Skip to content

BUILD: set install_name for libgap on macOS #5769

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.

Sign up for GitHub

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

Merged
fingolfin merged 4 commits into from
Aug 27, 2024
Merged

BUILD: set install_name for libgap on macOS #5769

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e35d827
BUILD: set install_name for libgap on macOS
fingolfin Jul 29, 2024
983fb2f
Ensure install_name_tool is only called when targetting macOS
fingolfin Aug 27, 2024
51b1174
fixup
fingolfin Aug 27, 2024
d5e63d3
Set -headerpad_max_install_names
fingolfin Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Makefile.rules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,9 +448,6 @@ else ifneq (,$(findstring darwin,$(host_os)))
LINK_SHLIB_FLAGS += -current_version $(LIBGAP_CURRENT_VER)
LINK_SHLIB_FLAGS += -Wl,-single_module

# TODO: set install_name, at least for installed version of the lib?
#LINK_SHLIB_FLAGS += -install_name $(libdir)/$(LIBGAP_FULL)

GAP_CPPFLAGS += -DPIC
GAP_CFLAGS += -fno-common
GAP_CXXFLAGS += -fno-common
Expand Down Expand Up @@ -516,6 +513,7 @@ build/main.c: src/main.c
# 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) $@

@fingolfin fingolfin Aug 27, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be changed so it only runs on macOS

@mkoeppe mkoeppe Aug 27, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fingolfin fingolfin Aug 27, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


endif

Expand Down Expand Up @@ -687,6 +685,7 @@ install-libgap: $(LIBGAP_FULL) libgap.pc
$(INSTALL) -d -m 0755 $(DESTDIR)$(libdir)
$(INSTALL) -m 0644 $(LIBGAP_FULL) $(DESTDIR)$(libdir)
ln -sf $(LIBGAP_FULL) $(DESTDIR)$(libdir)/libgap$(SHLIB_EXT)
install_name_tool -id $(libdir)/$(LIBGAP_FULL) $(DESTDIR)$(libdir)/$(LIBGAP_FULL)
Comment thread
fingolfin marked this conversation as resolved.
Outdated
$(INSTALL) -d -m 0755 $(DESTDIR)$(libdir)/pkgconfig
$(INSTALL) -m 0644 libgap.pc $(DESTDIR)$(libdir)/pkgconfig

Expand Down