BUILD: set install_name for libgap on macOS#5769
BUILD: set install_name for libgap on macOS#5769fingolfin merged 4 commits intogap-system:masterfrom
Conversation
fingolfin
force-pushed
the
mh/install_name
branch
from
78f2349 to
b43d2e9
Compare
|
@ChrisJefferson any thoughts on this? |
... but only for the installed version of the library
fingolfin
force-pushed
the
mh/install_name
branch
from
b43d2e9 to
e35d827
Compare
Makefile.rules
Outdated
| # build rule for the gap executable used by the `install-bin` target | ||
| build/gap-install: libgap$(SHLIB_EXT) cnf/GAP-LDFLAGS cnf/GAP-LIBS cnf/GAP-OBJS build/obj/build/main.c.o | ||
| $(QUIET_LINK)$(LINK) $(GAP_LDFLAGS) build/obj/build/main.c.o $(GAP_LIBS) -L${abs_builddir} -lgap -o $@ | ||
| @install_name_tool -change $(LIBGAP_FULL) $(libdir)/$(LIBGAP_FULL) $@ |
There was a problem hiding this comment.
This needs to be changed so it only runs on macOS
There was a problem hiding this comment.
I'll add that relying on install_name_tool -change is brittle unless you use the ld option -headerpad_max_install_names at the time of linking. It's typically better to set the correct install name already in the linker command.
There was a problem hiding this comment.
Thank you, you are of course 100% right.
However, setting it in the linker command would require linking two copies of the library: the one we do now and then a second copy that gets installed. That wouldn't be completely unreasonable (we already build a second gap executable for installation), but it would really only be necessary for macOS, overall it was just easier to hack it together this way. It seems to work reasonably well.
Thanks also for reminding me about -headerpad_max_install_names, I had forgotten about that, will add it later/tomorrow to this PR.
fingolfin
added
the
release notes: not needed
|
|
||
| # TODO: set install_name, at least for installed version of the lib? | ||
| #LINK_SHLIB_FLAGS += -install_name $(libdir)/$(LIBGAP_FULL) | ||
| LINK_SHLIB_FLAGS += -headerpad_max_install_names |
There was a problem hiding this comment.
Have you forgotten -Wl, in front of -headerpad_max_install_names ? Because we have now sagemath/sage#40106 ...
3 participants
... but only for the installed version of the library
Alternative to PR #5736
Closes #5736
@mkoeppe I wonder if this also work for you?
Note that I am not yet removing
-single_module, as I still need to figure out when exactly it became default (if you happen to have any info on this, please let me know). But presumably this is "just" about silencing a warning, so hopefully OK to delay its removal a bit more.