Opened 2 years ago

Closed 23 months ago

#24644 closed defect (fixed)

Use $SAGE_SUDO when copying files from SAGE_DESTDIR to SAGE_LOCAL

Reported by: embray Owned by:
Priority: major Milestone: sage-8.3
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: 5a23d19 (Commits) Commit: 5a23d19e72159e4231e1b1a6391b7e9ec04cd1c6
Dependencies: Stopgaps:

Description

Minor followup to #24106, which didn't support the SAGE_SUDO variable properly.

Change History (16)

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 45127cba14768900e1da138252664ba8cc71024c to 56b9905bc57b4f038653c17e8105a846a75d044e

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

56b9905Don't use SAGE_SUDO in these helpers if SAGE_DESTDIR is set

comment:3 Changed 2 years ago by embray

  • Status changed from needs_review to needs_work

comment:4 Changed 2 years ago by git

  • Commit changed from 56b9905bc57b4f038653c17e8105a846a75d044e to 5a23d19e72159e4231e1b1a6391b7e9ec04cd1c6

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

5a23d19Don't use SAGE_SUDO in these helpers if SAGE_DESTDIR is set

comment:5 Changed 2 years ago by embray

(I was doing some JavaScript earlier and got some wires crossed)

comment:6 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:7 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_info

If this is really meant for installations as "root", then the installation in DESTDIR should already be done under the "sudo" user. So I think that this patch actually reverses existing correct behaviour.

=> Proposal: close as wontfix.

comment:8 Changed 2 years ago by jdemeyer

No, I was wrong: we should keep SAGE_SUDO for the DESTDIR installation and then also add it for moving the files to SAGE_LOCAL.

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:9 Changed 2 years ago by embray

I'm not sure about that. In the staged installation, the install to DESTDIR step should be treated as a "build" step, and doesn't need to have root privileges to run. I don't know what makes you think it should be done as root.

On Debian, fakeroot is provided and I believe install to the DESTDIR is done using fakeroot. But in Debian's case that's because it's being used to build binary tarballs, and you want the files in the resulting tarball to be owned by "root" (but fakeroot allows you to do this without actually having to gain root privileges on the system you're building on). For Sage's purposes this isn't necessary because we're not building binary tarballs--at least for now it's just a step needed for easily building a file manifest.

So only the actual install into SAGE_LOCAL should be done with elevated privileges, if any.

comment:10 follow-up: Changed 2 years ago by embray

Well, it looks like maybe there are some other problems with root installs. For example, if you try to run something like ./configure --prefix=/opt/sage where /opt/sage is owned by root, it will fail because the configure script tries to create some directories under $SAGE_LOCAL.

But if you run sudo ./configure (which really, you shouldn't have to in the first place) it fails because of AC_CHECK_ROOT.

comment:11 in reply to: ↑ 10 Changed 2 years ago by mkoeppe

Replying to embray:

Well, it looks like maybe there are some other problems with root installs. For example, if you try to run something like ./configure --prefix=/opt/sage where /opt/sage is owned by root, it will fail because the configure script tries to create some directories under $SAGE_LOCAL.

This is addressed by #21532.

comment:12 Changed 2 years ago by embray

  • Status changed from needs_info to needs_review

comment:13 Changed 2 years ago by embray

Latest patchbot shows one failure due to the banner issue (#25056) but otherwise ths should be good I think. I think this approach really makes the most sense for the time being.

comment:14 Changed 2 years ago by saraedum

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

comment:15 Changed 2 years ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:16 Changed 23 months ago by vbraun

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