Opened 2 years ago

Closed 19 months 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) 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 2 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 2 years ago by git

  • Commit changed from efecbb56eb5e9743f63e0b24a639ae4edddf5c8b to fcd6e5d28644c3f24b01d719b6c422e251701c56

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a591ffbAdded the ability for packages to have a post-install script called spkg-postinst.
599668cThe postinst script should probably be run with SAGE_SUDO if it is set
ac34ecbAdd a paragraph to the docs about spkg-postinst
6519c3fUse $SAGE_SUDO, in case it's set, when moving files from the $SAGE_DESTDIR to $SAGE_LOCAL
454b75bDon't use SAGE_SUDO in these helpers if SAGE_DESTDIR is set
fcd6e5dAdd support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation

comment:3 Changed 2 years ago by git

  • Commit changed from fcd6e5d28644c3f24b01d719b6c422e251701c56 to fd1719e051417e956af06a6fe4e51a09171dd366

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fd1719eAdd support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation

comment:4 Changed 22 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:5 Changed 22 months ago by vdelecroix

  • Status changed from needs_review to needs_work

merge failure on 8.3.b0

comment:6 Changed 22 months ago by git

  • Commit changed from fd1719e051417e956af06a6fe4e51a09171dd366 to d03844b7b3897d837b5370c3e972900cbacc966b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d03844bAdd support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation

comment:7 Changed 22 months ago by embray

  • Dependencies #24645 deleted
  • Status changed from needs_work to needs_review

Rebased since its dependency #24645 has been satisfied.

comment:8 follow-up: Changed 22 months ago by jdemeyer

  • Status changed from needs_review to needs_work

What's up with build/pkgs/widgetsnbextension/spkg-postinst? That should just be removed.

comment:9 follow-up: Changed 22 months ago by jdemeyer

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 in reply to: ↑ 8 ; follow-up: Changed 22 months ago by embray

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 in reply to: ↑ 9 ; follow-up: Changed 22 months ago by embray

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 in reply to: ↑ 11 Changed 22 months ago by jdemeyer

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 in reply to: ↑ 10 Changed 22 months ago by jdemeyer

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 22 months ago by embray

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 22 months ago by git

  • Commit changed from d03844b7b3897d837b5370c3e972900cbacc966b to 00bd3f3814f22b5d43c8402821f3df893ac380b9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6bbff40Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
00bd3f3try installing the symlink anyways and don't poke around SAGE_LOCAL

comment:16 Changed 22 months ago by embray

  • Status changed from needs_work to needs_review

Ok, I dropped the spkg-postinst for widgetsnbextension and updated the sagenb install. I think this is better now.

comment:17 Changed 22 months ago by jdemeyer

For sagenb, you still removed the check for a working symlink. Why?

comment:18 Changed 22 months ago by embray

The symlink wouldn't resolve because this is being installed in DESTDIR.

comment:19 Changed 22 months ago by jdemeyer

The changes to Sphinx conflict with #24935. It is possible to just revert those?

comment:20 Changed 22 months ago by embray

Unfortunately they're necessary for the package to install correctly. You could merge this into #24935.

comment:21 Changed 21 months ago by saraedum

  • Status changed from needs_review to needs_work

there are merge conflicts

comment:22 Changed 21 months ago by git

  • Commit changed from 00bd3f3814f22b5d43c8402821f3df893ac380b9 to 208d754a61850cccb34b6a27afc3553fa7dc3bbb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5cb0a24Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
208d754try installing the symlink anyways and don't poke around SAGE_LOCAL

comment:23 Changed 21 months ago by embray

  • Status changed from needs_work to needs_review

I'm surprised this hasn't been merged yet. It really ought to be.

comment:24 Changed 20 months ago by saraedum

  • Work issues set to merge conflicts

comment:25 Changed 20 months ago by git

  • Commit changed from 208d754a61850cccb34b6a27afc3553fa7dc3bbb to 449a6a29ff85becfefa9b915a9aa0e56a3ea409f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

49aa08fAdd support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
449a6a2try installing the symlink anyways and don't poke around SAGE_LOCAL

comment:26 Changed 20 months ago by embray

Here we go again...

comment:27 Changed 20 months ago by embray

  • Work issues merge conflicts deleted

comment:28 Changed 20 months ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to positive_review

comment:29 Changed 20 months ago by vbraun

  • Status changed from positive_review to needs_work

See patchbot

comment:30 Changed 20 months ago by embray

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 20 months ago by git

  • Commit changed from 449a6a29ff85becfefa9b915a9aa0e56a3ea409f to 4d2050677eb89a12bd864f100b601ea3dc586887

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b1412f5Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
4d20506try installing the symlink anyways and don't poke around SAGE_LOCAL

comment:32 Changed 20 months ago by embray

  • Status changed from needs_work to positive_review

The spkg-postinst for Sphinx was simply no longer necessary so I removed it.

comment:33 Changed 19 months 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:34 follow-up: Changed 19 months ago by vbraun

  • Status changed from positive_review to 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 in reply to: ↑ 34 Changed 19 months ago by embray

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 19 months ago by embray

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 19 months ago by git

  • Commit changed from 4d2050677eb89a12bd864f100b601ea3dc586887 to ff1ccd3c132a4bd27a36a1c56ecaf171ad3b7a5d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7643e66Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
32f5bbetry installing the symlink anyways and don't poke around SAGE_LOCAL
ff1ccd3bump package version on sagenb since its spkg-install changes are slightly non-trivial

comment:38 Changed 19 months ago by embray

  • Status changed from needs_work to positive_review

New commits:

7643e66Add support for SAGE_DESTDIR to sdh_pip_install, thus making most Python packages work with staged installation
32f5bbetry installing the symlink anyways and don't poke around SAGE_LOCAL
ff1ccd3bump package version on sagenb since its spkg-install changes are slightly non-trivial

comment:39 Changed 19 months ago by vbraun

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