Skip to content
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

Further fixes to $DESTDIR handling #1611

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Further fixes to $DESTDIR handling #1611

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 27 additions & 27 deletions Makefile.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ FLINT_SOLIB:=@FLINT_SOLIB@
prefix:=@prefix@
exec_prefix:=@exec_prefix@

INCLUDEDIR:=$(DESTDIR)@includedir@
LIBDIR:=$(DESTDIR)@libdir@
BINDIR:=$(DESTDIR)@bindir@
INCLUDEDIR:=@includedir@
LIBDIR:=@libdir@
BINDIR:=@bindir@
PKGCONFIGDIR:=$(LIBDIR)/pkgconfig

HOST_OS:=@host_os@
Expand Down Expand Up @@ -202,10 +202,10 @@ BUILD_DIRS += \
endif

INSTALL_DIRS := \
$(LIBDIR) $(INCLUDEDIR)/flint \
$(PKGCONFIGDIR)
$(DESTDIR)$(LIBDIR) $(DESTDIR)$(INCLUDEDIR)/flint \
$(DESTDIR)$(PKGCONFIGDIR)
ifneq ($(FLINT_DLLLIB),0)
INSTALL_DIRS += $(BINDIR)
INSTALL_DIRS += $(DESTDIR)$(BINDIR)
endif

################################################################################
Expand Down Expand Up @@ -722,41 +722,41 @@ distclean: clean
################################################################################

install: library | $(INSTALL_DIRS)
$(CP) flint.pc $(PKGCONFIGDIR)/flint.pc
$(CP) flint.pc $(DESTDIR)$(PKGCONFIGDIR)/flint.pc
ifneq ($(SHARED), 0)
ifneq ($(FLINT_DLLLIB),0)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB) $(BINDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_MAJOR) $(BINDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_FULL) $(BINDIR)
$(CP) $(FLINT_DIR)/$(FLINT_IMPLIB) $(LIBDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB) $(DESTDIR)$(BINDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_MAJOR) $(DESTDIR)$(BINDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_FULL) $(DESTDIR)$(BINDIR)
$(CP) $(FLINT_DIR)/$(FLINT_IMPLIB) $(DESTDIR)$(LIBDIR)
else
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB) $(LIBDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_MAJOR) $(LIBDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_FULL) $(LIBDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB) $(DESTDIR)$(LIBDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_MAJOR) $(DESTDIR)$(LIBDIR)
$(CP_A) $(FLINT_DIR)/$(FLINT_LIB_FULL) $(DESTDIR)$(LIBDIR)
endif
ifneq ($(FLINT_DYLIB),0)
install_name_tool -id $(LIBDIR)/$(FLINT_LIB_FULL) $(LIBDIR)/$(FLINT_LIB)
install_name_tool -id $(LIBDIR)/$(FLINT_LIB_FULL) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
install_name_tool -id $(LIBDIR)/$(FLINT_LIB_FULL) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB)
install_name_tool -id @libdir@/$(FLINT_LIB_FULL) $(LIBDIR)/$(FLINT_LIB)

What about just fixing this and leaving all the rest unchanged?

I worry that in the future some added line might forget to include $(DESTDIR).

Alternatively something like this:

Suggested change
install_name_tool -id $(LIBDIR)/$(FLINT_LIB_FULL) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB)
install_name_tool -id $(FLINT_DYLIB_ID) $(LIBDIR)/$(FLINT_LIB)

where this is added at the top:

FLINT_DYLIB_ID:=@libdir@/@FLINT_LIB_FULL@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it that way because I thought it more standard for things like $(BINDIR) to refer to final paths, with $(DESTDIR) only coming up in the installation target. But I'm happy with any of these options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, maybe you are right. Don't take my suggestion as a reference you probably know better than me.

In any case, I think $(DESTDIR) handling has been broken a few times in flint in the past, so whatever minimizes chances of getting it wrong is good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for the sake of it, I agree with @mezzarobba -- everyone handles it like this (i.e. DESTDIR really only affects make install). This is relevant in so far as that it means that if e.g. I were to edit the FLINT build system, then handling this differently would trip me up -- I'd not expect BINDIR to include DESTDIR.

That said, of course either approach can work.

endif
endif
ifneq ($(STATIC), 0)
$(CP) $(FLINT_DIR)/$(FLINT_LIB_STATIC) $(LIBDIR)
$(CP) $(FLINT_DIR)/$(FLINT_LIB_STATIC) $(DESTDIR)$(LIBDIR)
endif
$(CP) $(HEADERS) $(INCLUDEDIR)/flint
$(CP) $(HEADERS) $(DESTDIR)$(INCLUDEDIR)/flint

uninstall:
$(RM_F) $(PKGCONFIGDIR)/flint.pc
$(RM_F) $(DESTDIR)$(PKGCONFIGDIR)/flint.pc
ifneq ($(FLINT_DLLLIB),0)
$(RM_F) $(BINDIR)/$(FLINT_LIB)
$(RM_F) $(BINDIR)/$(FLINT_LIB_MAJOR)
$(RM_F) $(BINDIR)/$(FLINT_LIB_FULL)
$(RM_F) $(LIBDIR)/$(FLINT_IMPLIB)
$(RM_F) $(DESTDIR)$(BINDIR)/$(FLINT_LIB)
$(RM_F) $(DESTDIR)$(BINDIR)/$(FLINT_LIB_MAJOR)
$(RM_F) $(DESTDIR)$(BINDIR)/$(FLINT_LIB_FULL)
$(RM_F) $(DESTDIR)$(LIBDIR)/$(FLINT_IMPLIB)
else
$(RM_F) $(LIBDIR)/$(FLINT_LIB)
$(RM_F) $(LIBDIR)/$(FLINT_LIB_MAJOR)
$(RM_F) $(LIBDIR)/$(FLINT_LIB_FULL)
$(RM_F) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB)
$(RM_F) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB_MAJOR)
$(RM_F) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB_FULL)
endif
$(RM_F) $(LIBDIR)/$(FLINT_LIB_STATIC)
$(RM_RF) $(INCLUDEDIR)/flint
$(RM_F) $(DESTDIR)$(LIBDIR)/$(FLINT_LIB_STATIC)
$(RM_RF) $(DESTDIR)$(INCLUDEDIR)/flint

################################################################################
# maintainer stuff
Expand Down
Loading