Opened 5 years ago
Closed 5 years ago
#24646 closed enhancement (fixed)
Add support for staged installations in packages that use sdh_pip_install
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.4 |
Component: | build | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Julian Rüth |
Report Upstream: | N/A | Work issues: | |
Branch: | ff1ccd3 (Commits, GitHub, GitLab) | Commit: | ff1ccd3c132a4bd27a36a1c56ecaf171ad3b7a5d |
Dependencies: | Stopgaps: |
Description
This implements #22509 for any Python packages that are installed with sdh_pip_install
. Passing --root=
to pip is essentially the same as using DESTDIR
with make install
.
This required a few additional changes to a small handful of packages for them to work with staged installation.
Change History (39)
comment:1 Changed 5 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 5 years ago by
Commit: | efecbb56eb5e9743f63e0b24a639ae4edddf5c8b → fcd6e5d28644c3f24b01d719b6c422e251701c56 |
---|
comment:3 Changed 5 years ago by
Commit: | fcd6e5d28644c3f24b01d719b6c422e251701c56 → fd1719e051417e956af06a6fe4e51a09171dd366 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fd1719e | Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
|
comment:4 Changed 5 years ago by
Milestone: | sage-8.2 → sage-8.3 |
---|
comment:6 Changed 5 years ago by
Commit: | fd1719e051417e956af06a6fe4e51a09171dd366 → d03844b7b3897d837b5370c3e972900cbacc966b |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d03844b | Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
|
comment:7 Changed 5 years ago by
Dependencies: | #24645 |
---|---|
Status: | needs_work → needs_review |
Rebased since its dependency #24645 has been satisfied.
comment:8 follow-up: 10 Changed 5 years ago by
Status: | needs_review → needs_work |
---|
What's up with build/pkgs/widgetsnbextension/spkg-postinst
? That should just be removed.
comment:9 follow-up: 11 Changed 5 years ago by
For sagenb
, why not keep
if [ ! -d mathjax ]; then echo >&2 "Error: Cannot symlink mathjax into the SageNB data directory." exit 1 fi
It seems like a useful sanity check.
comment:10 follow-up: 13 Changed 5 years ago by
Replying to jdemeyer:
What's up with
build/pkgs/widgetsnbextension/spkg-postinst
?
Let me check whether or not that was intended to be in this branch.
That should just be removed.
I don't see how you can assert that without knowing what it is.
comment:11 follow-up: 12 Changed 5 years ago by
Replying to jdemeyer:
For
sagenb
, why not keep
You can see just above that it checks that the directory being symlinked exists before making the symlink.
comment:12 Changed 5 years ago by
Replying to embray:
You can see just above that it checks that the directory being symlinked exists before making the symlink.
It's not at all obvious to me that the check before is equivalent to the ln -s
succeeding. The check after is simpler and therefore safer.
comment:13 Changed 5 years ago by
Replying to embray:
I don't see how you can assert that without knowing what it is.
I know what it is because I wrote the original. The code which is in that spkg-postinst
script used to be in spkg-install
but it's no longer needed because of Jupyter improvements. At some point, you must have had a merge/rebase conflict with spkg-install
and this left-over spkg-postinst
script is fallout from that.
comment:14 Changed 5 years ago by
I see, you even removed the jupyter nbextension
call a while ago (but probably after I first wrote this). I believe you then, that it's no longer needed.
comment:15 Changed 5 years ago by
Commit: | d03844b7b3897d837b5370c3e972900cbacc966b → 00bd3f3814f22b5d43c8402821f3df893ac380b9 |
---|
comment:16 Changed 5 years ago by
Status: | needs_work → needs_review |
---|
Ok, I dropped the spkg-postinst
for widgetsnbextension
and updated the sagenb install. I think this is better now.
comment:17 Changed 5 years ago by
For sagenb
, you still removed the check for a working symlink. Why?
comment:18 Changed 5 years ago by
The symlink wouldn't resolve because this is being installed in DESTDIR.
comment:19 Changed 5 years ago by
The changes to Sphinx conflict with #24935. It is possible to just revert those?
comment:20 Changed 5 years ago by
Unfortunately they're necessary for the package to install correctly. You could merge this into #24935.
comment:22 Changed 5 years ago by
Commit: | 00bd3f3814f22b5d43c8402821f3df893ac380b9 → 208d754a61850cccb34b6a27afc3553fa7dc3bbb |
---|
comment:23 Changed 5 years ago by
Status: | needs_work → needs_review |
---|
I'm surprised this hasn't been merged yet. It really ought to be.
comment:24 Changed 5 years ago by
Work issues: | → merge conflicts |
---|
comment:25 Changed 5 years ago by
Commit: | 208d754a61850cccb34b6a27afc3553fa7dc3bbb → 449a6a29ff85becfefa9b915a9aa0e56a3ea409f |
---|
comment:27 Changed 5 years ago by
Work issues: | merge conflicts |
---|
comment:28 Changed 5 years ago by
Reviewers: | → Julian Rüth |
---|---|
Status: | needs_review → positive_review |
comment:30 Changed 5 years ago by
Apparently I had asked Jeroen to make this a prerequisite to #24935 (after all, this ticket was opened first and had no reason to wait so long to be merged). But he forgot about that or ignored it (and I likewise forgot about it).
comment:31 Changed 5 years ago by
Commit: | 449a6a29ff85becfefa9b915a9aa0e56a3ea409f → 4d2050677eb89a12bd864f100b601ea3dc586887 |
---|
comment:32 Changed 5 years ago by
Status: | needs_work → positive_review |
---|
The spkg-postinst
for Sphinx was simply no longer necessary so I removed it.
comment:33 Changed 5 years ago by
Milestone: | sage-8.3 → sage-8.4 |
---|
I believe this issue can reasonably be addressed for Sage 8.4.
comment:34 follow-up: 35 Changed 5 years ago by
Status: | positive_review → needs_work |
---|
You obviously didn't try to build Sage with the ticket
No record that 'sagenb' was ever installed; skipping uninstall ./spkg-install: line 33: syntax error near unexpected token `fi' ./spkg-install: line 33: `fi'
comment:35 Changed 5 years ago by
Replying to vbraun:
You obviously didn't try to build Sage with the ticket
That's a bit flippant. Of course I built Sage with this ticket. It's been opened for 6 months and has had to be rebased many times. Perhaps there was a small merge error along the way. But that's not as if it was never tested...
comment:36 Changed 5 years ago by
Also, the ticket doesn't bump the package versions of the affected packages, so you would only see the issue if rebuilding from scratch. The "obviously" is pretty unnecessary.
comment:37 Changed 5 years ago by
Commit: | 4d2050677eb89a12bd864f100b601ea3dc586887 → ff1ccd3c132a4bd27a36a1c56ecaf171ad3b7a5d |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7643e66 | Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
|
32f5bbe | try installing the symlink anyways and don't poke around SAGE_LOCAL
|
ff1ccd3 | bump package version on sagenb since its spkg-install changes are slightly non-trivial
|
comment:38 Changed 5 years ago by
Status: | needs_work → positive_review |
---|
New commits:
7643e66 | Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
|
32f5bbe | try installing the symlink anyways and don't poke around SAGE_LOCAL
|
ff1ccd3 | bump package version on sagenb since its spkg-install changes are slightly non-trivial
|
comment:39 Changed 5 years ago by
Branch: | u/embray/build/destdir-pip → ff1ccd3c132a4bd27a36a1c56ecaf171ad3b7a5d |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Added the ability for packages to have a post-install script called spkg-postinst.
The postinst script should probably be run with SAGE_SUDO if it is set
Add a paragraph to the docs about spkg-postinst
Use $SAGE_SUDO, in case it's set, when moving files from the $SAGE_DESTDIR to $SAGE_LOCAL
Don't use SAGE_SUDO in these helpers if SAGE_DESTDIR is set
Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation