[Libwebsockets] Recent buggy changes

Andy Green andy.green at linaro.org
Wed May 6 03:04:31 CEST 2015


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/ea9cd6db/attachment-0001.html>


More information about the Libwebsockets mailing list