[Libwebsockets] Recent buggy changes

Andrey Pokrovskiy wonder.mice at gmail.com
Wed May 6 03:41:20 CEST 2015


> Oh, it's not part of this single patchbomb then?
> commit f1b125442be619c43bff45791a3f6fd5726099b5

No, that was a separate patch:
https://github.com/warmcat/libwebsockets/commit/41802c7a98a1ff559fba9e7b1165fd9a8e35e99d

The patch you are talking about goes afterwards and removes
CMAKE_BUILD define from the CMakaLists.txt since it's no longer
needed.

But I agree that you should ask to split that patch :)

On Tue, May 5, 2015 at 6:04 PM, Andy Green <andy.green at linaro.org> wrote:
>
> On 6 May 2015 08:33, "Andrey Pokrovskiy" <wonder.mice at gmail.com> wrote:
>>
>> > Otherwise I think we were better off how it was.
>> > I assume user code prior to this with CMAKE_BUILD will just continue to
>> > work fine.
>>
>> To use the previous version of the library correctly CMAKE_BUILD
>> _must_ be defined. It it worked without CMAKE_BUILD defined you just
>> got lucky.
>>
>> If CMAKE_BUILD _must_ be defined, then it doesn't make sense to check
>> whether it defined or not.
>
> What I mean is that code already exists out there.  So whatever we do if it
> meets CMAKE_BUILD defined and include the current filepath, it should
> continue to act well.
>
>> > I think the root cause of the current situation is I accepted some
>> > patchbombs I should have asked to be split up.
>>
>> It already was a _separate_ patch :)
>
> Oh, it's not part of this single patchbomb then?
>
> commit f1b125442be619c43bff45791a3f6fd5726099b5
> Author: wonder-mice <wonder.mice at gmail.com>
> Date:   Wed Apr 22 10:52:13 2015 -0700
>
>     Subject: [PATCH] Multiple changes in the build process
>
>     * Default CMAKE_BUILD_TYPE to Release
>     * Use CMAKE_BUILD_TYPE to define _DEBUG (NDEBUG is more standard though,
>       but cmake defines it)
>     * Drop LWS_WITHOUT_DEBUG (use CMAKE_BUILD_TYPE for that)
>     * Drop LWS_NO_EXTERNAL_POLL (was not used)
>     * Drop CMAKE_BUILD (CMake is the only build system now)  <<<<------
>     * Add LWS_WITH_STATIC and LWS_WITH_SHARED to choose what version(s) to
>       build
>     * Add LWS_XXX_LIBRARIES and LWS_XXX_INCLUDE_DIRS for each library
>       dependency (zlib, openssl, libev, cyassl)
>     * Support setting of XXX_LIBRARIES, XXX_LIBRARIES and XXX_INCLUDE_DIRS
>       by parent project (when included with add_subdirectory())
>     * Rename LWS_USE_EXTERNAL_ZLIB to LWS_USE_BUNDLED_ZLIB and default it to
>       NO (since it's Windows only)
>     * Default LWS_WITHOUT_DAEMONIZE to NO (since network lib shouldn't know
>       how to do it anyway)
>     * Rename config.h.cmake to lws_config.h.in
>     * Rename shared library to websockets_shared so linker will not be
>       confused
>     * Fix inline keyword detection in clang
>     * Explicitly set MACOSX_RPATH to YES on MacOS
>
> Yes it's a *single patch*, that's the issue: it should have been 14 patches.
>
> -Andy
>
>>
>> On Tue, May 5, 2015 at 5:20 PM, Andy Green <andy.green at linaro.org> wrote:
>> >
>> > On 6 May 2015 07:40, "Andrey Pokrovskiy" <wonder.mice at gmail.com> wrote:
>> >>
>> >> You have to ALWAYS include lws_config.h in the beginning of
>> >> libwebsockets.h because that's how libwebsockets.h knows about options
>> >> with which the library was compiled. If you look into libwebsockets.h
>> >> you will see it uses defines from lws_config.h, such as
>> >> LWS_NO_EXTENSIONS, LWS_OPENSSL_SUPPORT, LWS_USE_LIBEV.
>> >>
>> >> Before that patch it was basically broken and to use it correctly you
>> >> have to define CMAKE_BUILD in your project manually to workaround
>> >> that.
>> >>
>> >> But I agree that it's completely inappropriate to have such generic
>> >> defines in library public header. I even added a comment for that:
>> >>
>> >> /* That's a bad idea since it will leak all internal defines outside */
>> >> #include "lws_config.h"
>> >
>> > Yeah.  It's better if you don't say, "it's a bad idea to stick my hand
>> > in
>> > the fire" and then stick your hand in the fire as if that absolved
>> > anyone
>> > from feeling the pain though.
>> >
>> >> I suppose the right solution is to split lws_config.h on two - public
>> >> (LWS_xxx things) and private (all other things). Or just prefix
>> >> everything with "LWS_". But any solution probably will have some
>> >> "compatibility" issues, so chances are things will remain the way they
>> >> are.
>> >
>> > I think the root cause of the current situation is I accepted some
>> > patchbombs I should have asked to be split up.
>> >
>> >> On Tue, May 5, 2015 at 1:19 PM, Roger Light <andy.green at linaro.org>
>> >> wrote:
>> >> > Hi there,
>> >> >
>> >> > I believe that a recent changeset introduced some undesirable changes
>> >> > to libwebsockets. I'm not sure what the best solution is, hence this
>> >> > email.
>> >> >
>> >> > The change is:
>> >> >
>> >> >
>> >> >
>> >> > https://github.com/warmcat/libwebsockets/commit/04da2ccd1e8c5b582c4e2a77ee53f929ae8f22a0
>> >> >
>> >> > "Always include lws_config.h since now we have only CMake build"
>> >> >
>> >> > This means that libwebsockets.h always #includes lws_config.h,
>> >> > whereas
>> >> > before it was only included if CMAKE_BUILD was defined - this means
>> >> > only when the library itself was being built in practice. From what I
>> >> > can see, lws_config.h includes a mix of information that is useful to
>> >> > people using lws - such as whether it was compiled with cyassl
>> >> > support
>> >> > - and lots of things that aren't really of any interest.
>> >> >
>> >> > One of these things is the "#define VERSION" macro. This means that
>> >> > lws is claiming ownership of the VERSION macro, which seems a bit
>> >> > impolite for a library to do :) The shame of it is that the changeset
>> >> > acknowledges that the change is going to cause problems.
>> >> >
>> >> > I think that these is a lot of leftover cruft from autoconf days, but
>> >> > I don't know everything that can be cut. I think that if it is
>> >> > possible to have lws_config.h for public information about the lws
>> >> > build, and lws_config_private.h for things that lws needs for its own
>> >> > build that would be a reasonable solution.
>> >
>> > It sounds good to me if someone will implement it.  Otherwise I think we
>> > were better off how it was.
>> >
>> > I assume user code prior to this with CMAKE_BUILD will just continue to
>> > work
>> > fine.
>> >
>> > -Andy
>> >
>> >> > Any thoughts?
>> >> >
>> >> > Cheers,
>> >> >
>> >> > Roger
>> >> > _______________________________________________
>> >> > Libwebsockets mailing list
>> >> > Libwebsockets at ml.libwebsockets.org
>> >> > http://ml.libwebsockets.org/mailman/listinfo/libwebsockets



More information about the Libwebsockets mailing list