Embed luasocket, add mobdebug and --debugger command line option #1020
Conversation
Add mobdebug lua script to allow debugging premake with ZeroBrane Studio Add --debugger command line option to enable debugging
…defined as __attribute__((visibility(\"default\"))) on all platforms except Windows
There was a problem hiding this comment.
Your commit titles could be a little more succinct, but that's nitpicking, and it otherwise looks good to me. @samsinsane, what do you think? Any objection to merging another library?
|
There's just one thing I wasn't sure of which is putting mobdebug.lua at the root of the premake folder. I couldn't get it to work any other way, but that's just my inexperience with lua I guess. I can spend a bit more time trying to figure it out. How does updating branches work? If I update the luasocket branch does the PR 'reset' waiting for approval again? Regarding the commit messages I can try summarizing a bit more :) |
There was a problem hiding this comment.
Hmm, as I mentioned in another PR, I'm more in favour of pushing contribs to submodules. Given the complexities of doing so (GitHub Releases don't produce source archives with submodule source) I think this is fine. There's just so much noise in the PR, commit tree, contribution statistics and what not, but it's not the end of the world more of a mild inconvenience of software development. (GitLab sometimes has nice graphs for some statistics, but GitHub doesn't have them - probably because they provide absolutely no value and just look nice lol)
I've approved it, but the rename is a little confusing and I'd prefer it wasn't done if it's not required. Kind of nitpicky though, so happy to merge as-is.
contrib/luasocket/premake5.lua
Outdated
| "src/wsocket.*", | ||
| } | ||
|
|
||
| defines { "LUASOCKET_API=__attribute__((visibility(\"default\")))" } |
There was a problem hiding this comment.
Generally, files should end with an empty line - specifically an empty line, not a blank line with indentation. That's what this two red emojis are all about here. Apparently some tools in the "*nix" world assume text files end with an empty line, and searching for LUASOCKET_API would not flag this file due to that.
There was a problem hiding this comment.
Right, I'll fix this up with a newline then
src/host/buffered_io.c
Outdated
| @@ -9,7 +9,7 @@ | |||
| #include "premake.h" | |||
| #include "buffered_io.h" | |||
|
|
|||
| void buffer_init(Buffer* b) | |||
| void buffer_initialize(Buffer* b) | |||
There was a problem hiding this comment.
Not sure why this was renamed?
There was a problem hiding this comment.
luasocket has a function with the same name, and C doesn't allow function aliases as I figured out (I'm used to C++ so that took me by surprise) so in the interest of minimal changes I renamed it.
I figured that was an issue with the debugger itself. I don't have any objection to this myself, and it can always be moved later if there is a need/way. |
|
This might be a bit more complicated, and extra work at this point, but I did create the "lua shim" for this purpose, so that LuaRocks can easily be ported for the premake lua interpreter. Quite honestly, I would prefer it if the "lua socket" library was compiled as a "binary module", you'll find an example of that here: it would make loading the .dll more optional, and not impact the premake5.exe with the added embedded code, and you wouldn't have to rename those buffer methods either. Anyway, since I'm a little out of the loop, I'm not going to request changes here, but I figured I'd at least give you all my preferred approach.... and it's always something that can be changed later on as well. |
|
What do you think of @tvandijck's suggestions, @redorav? Could you take a crack at splitting it out into a module? |
|
@starkos @tvandijck Sorry for not replying earlier, it's being a busy week and weekend, I can take a look at the suggested solution of adding it as binary module, I'll first have to understand what they are and how they work. I thought defining it out would be optional enough for people who don't want to add the library, but I can certainly understand wanting to integrate it differently. |
|
@tvandijck I've managed to build luasocket as a dll and exported the main function called luaopen_socket_core, but when it comes to calling require(), I can't figure out how to do the equivalent of
that I used when building as a static library. It seems like it will look for a dll of the same name and tries to match the function somehow but all variations of the name always return an error. I have been able to call require('example') with the example.dll in the same directory, so I must be almost there. EDIT: Ah, never mind, I just realized I have to load the luashim before loading any libs, which means creating my own luaopen_luasocket function instead of using luaopen_socket_core directly from the luasocket code. It's all working now :) |
|
@starkos Feel free to review this when you have a bit of time. I managed to get it working and didn't spend too much time on it. The solution ends up being pretty much the same except with a dll instead of a binary, which is fine. I'll have to update the wiki steps I've written to make it clear but should be ok. One thing I didn't manage was outputting a good error message when luasocket isn't found. When calling require("luasocket") if the call fails I can't retrieve the return value to output something with it. Is there any multiplatform functionality in premake I can use to determine whether a given dynamic library exists alongside premake? |
|
I like it this way for sure... thank you for taking the time to make this change. |
Embed luasocket as a static library
Add mobdebug lua script to allow debugging premake with ZeroBrane Studio
Add --debugger command line option to enable debugging