-
-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Haiku fixes #1289
Haiku fixes #1289
Conversation
Any comment? |
so, I don't know anything about haiku, just what I have read on wikipedia recently Why does premake need to link against 'network'? |
Probably because of cURL I suppose. |
Bootstrap.mak
Outdated
$(SILENT) rm -rf ./build | ||
$(SILENT) rm -rf ./obj | ||
mkdir -p build/bootstrap | ||
$(CC) -o build/bootstrap/premake_bootstrap -DPREMAKE_NO_BUILTIN_SCRIPTS -DLUA_USE_POSIX -DLUA_USE_DLOPEN -D_BSD_SOURCE -I"$(LUA_DIR)" -I"$(LUASHIM_DIR)" $? -lnetwork -lbsd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what symbols does -lnetwork provide that premake_bootstrap requires?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, none it seems. I just used the same libs as for the rest, but it seems you don't use cURL for bootstrap so it works without. I'll make a fixup.
@@ -197,6 +197,10 @@ | |||
defines { "LUA_USE_POSIX", "LUA_USE_DLOPEN" } | |||
links { "m" } | |||
|
|||
filter "system:haiku" | |||
defines { "LUA_USE_POSIX", "LUA_USE_DLOPEN", "_BSD_SOURCE" } | |||
links { "network", "bsd" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
links { "network" } probably needs to go in the contrib/curl/premake5.lua under a haiku filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm Solaris links with libsocket… besides, it seems you're building cURL as a static lib anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but if you build premake without those contribs, or if cURL ever gets made into a binmodule, this is not a requirement for building the executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for now cURL is used as a static lib, so you must link it here, since you can't link static libs to anything (it's just an archive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point, I had forgotten that detail about static libraries.
How about putting links { "network" }
inside a if not _OPTIONS["no-curl"] then ... end
check like on line 182?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't luasocket also need it?
Again, the Solaris libs also are linked without any test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're wildly inconsistent with these options. macOS probably only needs Security.framework
when linking cURL too. I'll open an issue for this anyway since the other systems aren't handled properly either, it's up to you @mmuman if you want this done correctly now or likely done wrong later. I'll link to this discussion in the issue, but without a CI option for Haiku we can't make any guarantees. (If there is an option I'm happy to look at it though!)
bsd is for getpass() btw. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you could squish this down to a single commit, it's only +20/-6.
@@ -197,6 +197,10 @@ | |||
defines { "LUA_USE_POSIX", "LUA_USE_DLOPEN" } | |||
links { "m" } | |||
|
|||
filter "system:haiku" | |||
defines { "LUA_USE_POSIX", "LUA_USE_DLOPEN", "_BSD_SOURCE" } | |||
links { "network", "bsd" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're wildly inconsistent with these options. macOS probably only needs Security.framework
when linking cURL too. I'll open an issue for this anyway since the other systems aren't handled properly either, it's up to you @mmuman if you want this done correctly now or likely done wrong later. I'll link to this discussion in the issue, but without a CI option for Haiku we can't make any guarantees. (If there is an option I'm happy to look at it though!)
Ok I'll squash all this when I get some time :-) |
- contrib/curl: use linux config for Haiku Like svr4 we don't have SIOCGIFADDR. - Haiku is POSIX enough - Add libs for Haiku Not tested yet - contrib: fix Haiku bootstrap - Fix os.getversion() for Haiku - Fix bootstrap on Haiku
Just rebased and squashed, builds here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still marked as a draft, are you happy for it to be merged?
Oh right, I think we're set. |
This fixes bootstrapping on Haiku.
I could probably squash this a bit.
Still some tests in error though.
Also the makefile in the tarball seems to have hardcoded libs and stuff, not sure how to fix this.