-
Notifications
You must be signed in to change notification settings - Fork 705
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
Statically link ncurses... #3545
Conversation
so it's not needed at runtime
Fixes an issue for easybuilders/easybuild-framework#1882 |
Test report by @boegel |
This breaks the build, requires another build dep? |
Yeah, libreadline...I'm on it |
Add libreadline dep to Lua for static build
Test report by @boegel |
Hmm, not sure what to do here, the |
I think using |
Test report by @boegel |
Test report by @boegel |
@@ -16,9 +16,10 @@ sources = [SOURCE_TAR_GZ] | |||
|
|||
configopts = [ | |||
# default build | |||
'--with-shared --enable-overwrite', | |||
'--with-shared --enable-overwrite --with-termlib', |
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.
@ocaisa this should be reflected in the sanity_check_paths
too?
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.
@ocaisa Why exactly was this required here? It makes this ncurses
easyconfig that uses dummy
different from the other ncurses
easyconfigs, which causes problems (see also easybuilders/easybuild-easyblocks#1088).
I'm not saying we should blindly revert this change, since the split is already taken into account elsewhere (for example in https://github.com/easybuilders/easybuild-easyconfigs/pull/4977/files#diff-03dfb6d80d1923383727d96e696cc53fR20), just wondering what the exact reason was for this.
cc @pforai
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.
I only had to do this to be able to build lua at the dummy
level, it was never an issue for me, it was an issue for the tests. (from what I remember)
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.
OK, thanks for clarifying.
Looking back at the test report at https://gist.github.com/boegel/5c3303ac90ceea87960a213cf33fc024, building Lua-5.1.4-8.eb
was indeed failing without the split:
gcc -o lua lua.o liblua.a -lm -Wl,-E -ldl -Wl,-Bstatic -lhistory -lreadline -ltinfo -lncurses -Wl,-Bdynamic
gcc -o luac luac.o print.o liblua.a -lm -Wl,-E -ldl -Wl,-Bstatic -lhistory -lreadline -ltinfo -lncurses -Wl,-Bdynamic
/usr/bin/ld: cannot find -ltinfo
/usr/bin/ld: cannot find -ltinfo
Maybe the proper fix would have been to not let Lua link with -ltinfo
though...
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.
@vanzod
Ok, I think I have a fix for this but some may consider it hackish.
Some systems do expect a separate tinfo by default (Ubuntu 16.04) and in that case if we don't provide libtinfo
from ncurses
, some configures (such as the Lua
one in the original PR) will pick it up from the OS. To get around this we can just do a soft link of libtinfo.so -> libncurses.so
after installing ncurses
which is what debian has done in the past.
I also found (what I think is) a problem with our libreadline
easyconfigs. I'm not sure of the purpose of the linking to ncurses
there, but if we are expecting ncurses to be force-linked in the final .so then we are not doing it right, the correct option ( for v6.3
) is
buildopts = "SHLIB_LIBS='-lncurses'"
(from http://www.linuxfromscratch.org/lfs/view/7.4/chapter06/readline.html)
With those two options I can revert the ncurses
build to be more like the rest. I'll open a PR and we can discuss further there.
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.
See #5067
Test report by @boegel |
Test report by @boegel |
Test report by @boegel |
lgtm |
Going in, thanks @ocaisa! |
It works but gives me a strange end result, I'm not sure it does what I think it does. For my case if I don't include the libreadline dep and I say to link statically, I get a |
Maybe not, just repeated and things are different, I must have made a mistake |
Looks OK on my end:
|
Yeah, you are right, I messed it up somehow |
…(see easybuilders#3545) creating a soft link 'libtinfo.so -> libncurses.so' and extending it to create soft links 'libtinfo.so.5.9 -> libncurses.so.5.9' and 'libtinfo.so.5 -> libncurses.so.5' as well.
so it's not needed at runtime