Opened 5 years ago
Closed 4 years ago
#25044 closed enhancement (duplicate)
Add DESTDIR support for gap
Reported by:  Erik Bray  Owned by:  

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  build  Keywords:  destdir gap 
Cc:  Merged in:  
Authors:  Erik Bray  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/embray/build/destdirgap (Commits, GitHub, GitLab)  Commit:  ee25fe758ae28af6ca79ea49fbe3bfc2d3b0f65b 
Dependencies:  #24599, #25039, #23733  Stopgaps: 
Description
Implements #24024 for gap; this one is slightly nontrivial.
Change History (22)
comment:1 Changed 5 years ago by
Status:  new → needs_review 

comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
Hmm, for some reason I thought it wasn't a standard autoconf configure, but it looks like it is, actually. I don't know why it's being passed some PREFIX
argument. That doesn't seem to do anything.
comment:4 Changed 5 years ago by
Commit:  990b6adcdd3eac21f6243d1c739ca1194677e1b9 → 1af0242207918efa243cb405d69c89f6656b7d51 

Branch pushed to git repo; I updated commit sha1. New commits:
1af0242  Use sdh_configure; clean up some superfluous arguments to the configure script

comment:5 Changed 5 years ago by
Dependencies:  #24599, #25039 → #24599, #25039, #23733 

(already merges without conflict with #23733)
comment:6 Changed 5 years ago by
Status:  needs_review → needs_work 

This appears to have some kind of problem installing gap.sh
...
comment:7 Changed 5 years ago by
Commit:  1af0242207918efa243cb405d69c89f6656b7d51 → c14afaa0ec53cd34d91a65aa3d29ecc325ca4a44 

Branch pushed to git repo; I updated commit sha1. New commits:
b4ecda9  trac 23733: deprecate SAGE64 and CFLAG64

905e4d4  Stop supporting SAGE64 except in Numpy

4f85314  Merge branch 'u/jdemeyer/nosage64' into u/embray/build/destdirgap

c14afaa  turns out bin/gap.sh is a symlink to either gapdefault64.sh or gapdefault32.sh (only one or the other should exist). rather than add explicit support for dereferencing symlinks, for now just copy the real file

comment:8 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:9 Changed 5 years ago by
Milestone:  sage8.2 → sage8.3 

comment:10 Changed 4 years ago by
Status:  needs_review → needs_work 

Work issues:  → merge conflicts 
comment:11 Changed 4 years ago by
Commit:  c14afaa0ec53cd34d91a65aa3d29ecc325ca4a44 → 6d1493a8cc65b6a2fe869e20e1d92b7c52bd1b87 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b03b468  Update to use sagedisthelpers; add DESTDIR support where possible

19a00c4  update package version for buildbot

0a0ae72  Use sdh_configure; clean up some superfluous arguments to the configure script

6d1493a  turns out bin/gap.sh is a symlink to either gapdefault64.sh or gapdefault32.sh (only one or the other should exist). rather than add explicit support for dereferencing symlinks, for now just copy the real file

comment:12 Changed 4 years ago by
Status:  needs_work → needs_review 

Work issues:  merge conflicts 
I think it should be good now.
comment:13 Changed 4 years ago by
Milestone:  sage8.3 → sage8.4 

I believe this issue can reasonably be addressed for Sage 8.4.
comment:14 Changed 4 years ago by
Since SAGE_GAP
is no longer used in the script, it would be more clear to replace
SAGE_GAP="$SAGE_LOCAL/gap" INSTALL_DIR="$SAGE_GAP/$GAP_DIR"
by
INSTALL_DIR="$SAGE_LOCAL/gap/$GAP_DIR"
comment:15 followup: 17 Changed 4 years ago by
I would remove
[ f bin/gap.sh ]  sdh_die "Error building GAP ('gap.sh' not found)."
You have a commit message turns out bin/gap.sh is a symlink to either gapdefault64.sh or gapdefault32.sh (only one or the other should exist). rather than add explicit support for dereferencing symlinks, for now just copy the real file
. I would rather see something like that in a comment in the script, otherwise it's quite obscure what sdh_install T bin/gapdefault*.sh "$SAGE_LOCAL/bin/gap"
does.
Given that we have sagelegacyuninstall
, shouldn't this be moved there?
rm rf "$INSTALL_DIR" rm f "$SAGE_LOCAL/gap/latest" rm f "$SAGE_LOCAL/bin/gap"
(just a question, if there is a good reason to keep it there: fine for me)
comment:16 Changed 4 years ago by
Status:  needs_review → needs_work 

comment:17 Changed 4 years ago by
Replying to jdemeyer:
Given that we have
sagelegacyuninstall
, shouldn't this be moved there?rm rf "$INSTALL_DIR" rm f "$SAGE_LOCAL/gap/latest" rm f "$SAGE_LOCAL/bin/gap"(just a question, if there is a good reason to keep it there: fine for me)
Yes, this ticket just predates that being merged; as long as this is still open it makes sense to go ahead and move that stuff into an spkglegacyuninstall.
comment:18 Changed 4 years ago by
Commit:  6d1493a8cc65b6a2fe869e20e1d92b7c52bd1b87 → ee25fe758ae28af6ca79ea49fbe3bfc2d3b0f65b 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
25e9ae1  Update to use sagedisthelpers; add DESTDIR support where possible

392056a  update package version for buildbot

2da622a  Use sdh_configure; clean up some superfluous arguments to the configure script

b50ce9f  turns out bin/gap.sh is a symlink to either gapdefault64.sh or gapdefault32.sh (only one or the other should exist). rather than add explicit support for dereferencing symlinks, for now just copy the real file

448b6de  some review details

ee25fe7  move the old cleanup code to an spkglegacyuninstall

comment:21 Changed 4 years ago by
Milestone:  sage8.4 → sage8.5 

comment:22 Changed 4 years ago by
Milestone:  sage8.5 → sageduplicate/invalid/wontfix 

Resolution:  → duplicate 
Status:  needs_review → closed 
This is also implemented by #22626 which supersedes this.
Why not use
sdh_configure
here?