Updated luasocket to 3.0rc1 (Fix #1217)

#61 Declined
  1. James Watkins-Harvey

Updated luasocket to 3.0rc1 (git commit 316a945)

  • Issues #1217: Embedded luasocket should be updated resolved

Comments (9)

  1. James Watkins-Harvey author

    @Bart van Strien Thanks, I will add the missing include, then update the pull request. You mention LUASOCKET_API, but what error did it fixed exactly? You had a compilation error?

    Note also that, based on @buckle2000 feedback, the pull request still appears not to work properly on Windows; it compiles fine, but the sample code in issue #1216 still wont execute as expected. I still haven't had the opportunity to debug the issue myself.

    1. Bart van Strien

      I got errors because LUASOCKET_API wasn't defined. That didn't happen when you built it, because you added a definition for it to your build file (the visibility attribute). Since it's (usually) define in luasocket.h, including that fixed it.

      As for issue #1216, since that also happens without this pull request, I don't think it should block it. Last I looked at it, the multicast code hadn't really changed much anyway, so I wouldn't have expected it to fix it.

      1. James Watkins-Harvey author

        LUASOCKET_API: Right, I remember that now, but wasn't there more files using that macro? Anyway, I used a build time macro definition because that is what the upstream's makefile did; I will recheck that just to make sure there was no special case to deal with (something along the line of "the macro must exceptionally be defined as x on platform z").

        Issue #1216: I agree, though I expected the update to fix that issue too, given that the problem was actually fixed in the upstream in that version. And this is actually what bothers me: the original issue (in the upstream project) was a matters of not linking to the correct library on the Windows platform's makefile. If the problem persist in love after this pull request, it could indicate that love's makefile is still incorrect, which means there could be more hidden issues lying out there. But this might also be me going paranoid. Your call.

        1. Bart van Strien

          No, luasocket.c and serial.c are the only files using that macro. As for its effect, it clearly works on linux, and according to #1281 windows. Since you modified the xcode project, I presume you also tested it on OSX?

          Issue #1216: That is a possibility, but I doubt it gets worse after merging this PR. We'll leave the issue open, of course.

          1. James Watkins-Harvey author

            Yes, I personally tested on OSX. Succeed to build on Windows (in a VirtualBox VM), but could not test (because VirtualBox doesn't offer an OpenGL 2.1+ compatible driver on Windows 10).

        2. James Watkins-Harvey author

          Ok, in upstream we have: LUASOCKET_API='__attribute__((visibility("default")))' on all unix-based platforms, and LUASOCKET_API='__declspec(dllexport)' on Windows. I get the feeling that the macro definition currently in luasocket.h, that is LUASOCKET_API=extern, might fail to properly set symbols visibility in shared libraries.

          I'm considering replacing the definition of macro LUASOCKET_API, in luasocket.h by the following:

          #ifndef LUASOCKET_API
          #if defined(WIN32) || defined(_WIN32)
          #       define LUASOCKET_API __declspec(dllexport)
          #       define LUASOCKET_API __attribute__((__visibility__("default")))

          Does that makes sense to you?

          1. Bart van Strien

            'visibility' doesn't have underscores, but otherwise that seems reasonable. It's not necessarily needed though, at least on linux, since the default visibility isn't hidden (yet, we might change that in the future). I'm also in favour of moving that logic from the builds systems, especially since it hides bugs such as the missing include.

  2. James Watkins-Harvey author

    @Alex Szpakowski Indeed. This is so because luasocket no longer expose its modules through global symbols, but only through the file's return value, conforming to "the lua 5.1+ way of doing things"... This will most certainly cause some existing code to break, even though everyone has been warned for some time that such changes were coming up.

    The upstream change can rather easily be "reversed", simply by copying the returned value to global symbol "socket" (and similarly for others), providing a backward compatible path in the mid-term.

    Otherwise, I can update my pull request to be applied against branch minor. Not familiar with mercurial though... what exactly would be a good approach to "rebase" a pull request on another branch?

    1. Alex Szpakowski

      what exactly would be a good approach to "rebase" a pull request on another branch?

      I'm not familiar enough with Bitbucket pull requests or Mercurial branch shenanigans to know either. :(

      At worst you could make a diff of the changes, make a new branch based on minor, apply the patch, and open a new pull request...