Opened 3 years ago

Closed 3 years ago

#25045 closed defect (fixed)

Add DESTDIR support for mpfr

Reported by: embray Owned by:
Priority: major Milestone: sage-8.4
Component: build Keywords: destdir mpfr
Cc: Merged in:
Authors: Erik Bray Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: faec643 (Commits, GitHub, GitLab) Commit: faec6430e6e9eb433728282b821bab6f910557f2
Dependencies: #23733 Stopgaps:

Status badges

Description

Implements #24024 for mpfr.

This one is slightly non-trivial especially in how it uses multiple configure calls. However, my local experiments show that it's just fine to use sdh_configure there all the same (though it isn't strictly necessary either).

Change History (15)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Here, I see no reason to use sdh_configure. This is not the real ./configure run, it is only needed to check some defaults:

        ./configure --with-gmp="$SAGE_LOCAL" $SAGE_CONF_OPTS $MPFR_CONFIGURE) &>/dev/null;

You don't need

|| sdh_die << _EOF
Error configuring MPFR.
See above for the options passed to it, and the file
  $(pwd)/config.log
for details.

because sdh_configure can never fail (it either succeeds or exists the script).

comment:3 follow-up: Changed 3 years ago by embray

Do we care about keeping that error message at all then? (I'm not sure we do--the config.log file won't even exist unless built with SAGE_KEEP_BUILT_SPKGS=yes).

comment:4 in reply to: ↑ 3 Changed 3 years ago by jdemeyer

Replying to embray:

Do we care about keeping that error message at all then?

Ideally yes because the message saying to look at config.log is useful. But it should be put in sdh_configure, because it is generally useful for all packages using GNU configure. Of course, there should be a check that config.log exists.

the config.log file won't even exist unless built with SAGE_KEEP_BUILT_SPKGS=yes.

Directories of a failed build are kept anyway.

comment:5 follow-up: Changed 3 years ago by embray

Oh, I didn't know that. Or if I did I'd forgotten. I could move that message to sdh_configure. I agree it's useful in general, at least so long as we're talking standard autoconf.

The one wrinkle might be that in principle config.log can be somewhere other than $(pwd)/config.log. Sometimes it will be in some place like $(pwd)/config/config.log. I could either try to search for a config.log, or just assume $(pwd)/config.log and only print the message if it exists. For most cases that should probably suffice...

comment:6 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to embray:

Sometimes it will be in some place like $(pwd)/config/config.log.

Are you sure? As far as I know, this cannot be customized with GNU autoconf.

comment:7 Changed 3 years ago by embray

You know, I seem to recall seeing that before, but I wonder if that was some kind of AC_SUBDIRS thing going on with a trivial top-level directory or something. You're right that in general it can't be customized. So that makes things simpler.

comment:8 Changed 3 years ago by embray

  • Dependencies set to #23733

comment:9 Changed 3 years ago by git

  • Commit changed from c4e8f6f56081fb32b0d031c9058b6db6be8b7a9e to faec6430e6e9eb433728282b821bab6f910557f2

Branch pushed to git repo; I updated commit sha1. New commits:

b4ecda9trac 23733: deprecate SAGE64 and CFLAG64
905e4d4Stop supporting SAGE64 except in Numpy
0db62ebMerge branch 'u/jdemeyer/no-sage64' into u/embray/build/destdir-mpfr
70d5021For sdh_configure, add a generic message with the path to config.log (if it exists) so that it can be inspected for errors
faec643A little additional cleanup

comment:10 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:11 Changed 3 years ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:12 Changed 3 years ago by saraedum

  • Reviewers set to Julian Rüth

Looks good to me (though it does a bit more than what it says in the ticket description.) Feel free to set this to positive review if you think that this still works.

comment:13 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

comment:14 Changed 3 years ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

I believe this issue can reasonably be addressed for Sage 8.4.

comment:15 Changed 3 years ago by vbraun

  • Branch changed from u/embray/build/destdir-mpfr to faec6430e6e9eb433728282b821bab6f910557f2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.