[Libwebsockets] Recent buggy changes

Andy Green andy.green at linaro.org
Wed May 6 03:59:54 CEST 2015


On 6 May 2015 09:41, "Andrey Pokrovskiy" <wonder.mice at gmail.com> wrote:
>
> > 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 :)

Yeah... it's nice of you to agree you should have been asked to do your
patches better.

So who is going to do the work to fix this?  I know how to fix it by
reverting the changes and then putting anything critical back in
individually.  But it's clearly better to split the cmake includes if
someone is going to do it.

-Andy

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://libwebsockets.org/pipermail/libwebsockets/attachments/20150506/36a3a395/attachment-0001.html>


More information about the Libwebsockets mailing list