Further fixes to $DESTDIR handling #1611
Conversation
Change the definition of $BINDIR & co so that, in the case of staged installations, they contain the final (binary) installation directory, not the packaging directory. The only tangible effect should be that, on MacOS, install_name_tool is passed the correct library path. See sagemath/sage#35848 (comment)
| 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) |
There was a problem hiding this comment.
| 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:
| 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@
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I like this. Thanks! I will let @fredrik-johansson take care of the merging into anything other than |
Upgrade to flint3. Current Sage versions are not compatible with flint ≥ 3, and, though the diff is not huge, there are enough changes that versions including this PR will be incompatible with flint < 3. Fixes sagemath#20003. Closes sagemath#35993 as no longer relevant. Related PRs in upstream projects: * Singular/Singular#1177 * flintlib/flint#1408 * flintlib/flint#1489 * flintlib/flint#1492 * flintlib/flint#1611 * algebraic-solving/msolve#76 * flatsurf/e-antic#264 Additional changes still needed for optional packages to work: * sagemath#36677 * upgrade e-antic * possibly more Planned follow-ups: * sagemath#36449 * sagemath#36433 URL: sagemath#35848 Reported by: Marc Mezzarobba Reviewer(s): Vincent Delecroix
Upgrade to flint3. Current Sage versions are not compatible with flint ≥ 3, and, though the diff is not huge, there are enough changes that versions including this PR will be incompatible with flint < 3. Fixes sagemath#20003. Closes sagemath#35993 as no longer relevant. Related PRs in upstream projects: * Singular/Singular#1177 * flintlib/flint#1408 * flintlib/flint#1489 * flintlib/flint#1492 * flintlib/flint#1611 * algebraic-solving/msolve#76 * flatsurf/e-antic#264 Additional changes still needed for optional packages to work: * sagemath#36677 * upgrade e-antic * possibly more Planned follow-ups: * sagemath#36449 * sagemath#36433 URL: sagemath#35848 Reported by: Marc Mezzarobba Reviewer(s): Vincent Delecroix
Change the definition of $BINDIR & co so that, in the case of staged
installations, they contain the final (binary) installation directory,
not the packaging directory.
The only tangible effect should be that, on MacOS, install_name_tool is
passed the correct library path. See
sagemath/sage#35848 (comment)