Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#21726 closed enhancement (fixed)

Support splitting spkg install into `spkg-build` and `spkg-install` (for SAGE_SUDO)

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-7.5
Component: build Keywords:
Cc: jdemeyer, fbissey, embray, vbraun, dimpase, tscrim Merged in:
Authors: Matthias Koeppe Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 48053d0 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

Here's a new attempt toward #21537 that is perhaps more elegant.

Let's change build/bin/sage-spkg as follows:

  • If spkg-build exists, call that, followed by $SAGE_SUDO spkg-install.
  • Otherwise, just call spkg-install (withOUT $SAGE_SUDO). (Such packages cannot be installed into a SAGE_ROOT that is only root-writable, unless they use $SAGE_SUDO themselves.)

The patch changes two example packages, to demonstrate this: 4ti2 and lcalc.

Before the patch: 

$ rm -Rf local/share/4ti2
$ mkdir local/share/4ti2
$ sudo chown root local/share/4ti2
$ ./sage -f 4ti2
[...]
[4ti2-1.6.7] mkdir: /Users/mkoeppe/s/sage/sage-rebasing/local/share/4ti2/doc: Permission denied
[4ti2-1.6.7] make[4]: *** [install-docDATA] Error 1
[...]
$ sudo chown root local/bin/ local/bin/lcalc
$ ./sage -f lcalc 
[...]
[lcalc-1.23.p14] cp -f lcalc /Users/mkoeppe/s/sage/sage-rebasing/local//bin/.
[lcalc-1.23.p14] cp: /Users/mkoeppe/s/sage/sage-rebasing/local//bin/./lcalc: Permission denied
[...]

Then, after the patch: 

$ export SAGE_SUDO="sudo -E"
$ ./sage -f 4ti2 lcalc  # succeeds

Other packages will be handled in follow-up tickets.

See also:

  • #21844: Update developer manual regarding spkg-build, spkg-install, SAGE_SUDO
  • Task ticket: #21537

Change History (19)

comment:1 Changed 6 years ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 6 years ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 6 years ago by mkoeppe

  • Branch set to u/mkoeppe/support_splitting_spkg_install_into__spkg_build__and__spkg_install___for_sage_sudo_

comment:4 Changed 6 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 48053d0af7bef79e41a581aa492e58a285c9feb6
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

​b5621b4If spkg-build exists, run it, followed by $SAGE_SUDO spkg-install
​b595edd4ti2: Call make install within $SUDO
​48053d0lcalc: Split out spkg-build from spkg-install

comment:5 Changed 6 years ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 6 years ago by mkoeppe

  • Cc vbraun dimpase added

comment:7 Changed 6 years ago by mkoeppe

  • Cc tscrim added

comment:8 follow-up: Changed 6 years ago by embray

So, if I understand correctly, the point of calling spkg-install without $SAGE_SUDO if no spkg-build exists, is that for backwards-compatibility such packages should be assumed to be "special" in some way, and have to handle $SAGE_SUDO on their own. Is that correct?

comment:9 follow-up: Changed 6 years ago by embray

What is this about? 

+    if [ ! -x spkg-build ]; then
+	echo >&2 "WARNING: spkg-build is not executable, making it executable"
+	chmod +x spkg-build
+    fi

Why not just require it to be executable in the first place?

comment:10 Changed 6 years ago by embray

LGTM otherwise. This looks like what I had in mind in #21537.

comment:11 in reply to: ↑ 8 Changed 6 years ago by mkoeppe

Replying to embray:

So, if I understand correctly, the point of calling spkg-install without $SAGE_SUDO if no spkg-build exists, is that for backwards-compatibility such packages should be assumed to be "special" in some way, and have to handle $SAGE_SUDO on their own. Is that correct?

That's right.

comment:12 in reply to: ↑ 9 Changed 6 years ago by mkoeppe

Replying to embray:

What is this about? 

+    if [ ! -x spkg-build ]; then
+	echo >&2 "WARNING: spkg-build is not executable, making it executable"
+	chmod +x spkg-build
+    fi

Why not just require it to be executable in the first place?

It's imitating existing code that does the same for spkg-install. I don't know why it does that. Hysterical raisins?

comment:13 Changed 6 years ago by embray

I see that now. Might as well keep it for now then :(

comment:14 Changed 6 years ago by embray

  • Status changed from needs_review to positive_review

comment:15 Changed 6 years ago by embray

  • Reviewers set to Erik Bray

comment:16 Changed 6 years ago by mkoeppe

Thanks!

comment:17 Changed 6 years ago by mkoeppe

  • Description modified (diff)

comment:18 Changed 6 years ago by vbraun

  • Branch changed from u/mkoeppe/support_splitting_spkg_install_into__spkg_build__and__spkg_install___for_sage_sudo_ to 48053d0af7bef79e41a581aa492e58a285c9feb6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 5 years ago by embray

  • Commit 48053d0af7bef79e41a581aa492e58a285c9feb6 deleted

Semi-related, see #22510.

Note: See TracTickets for help on using tickets.