Opened 3 years ago

Closed 2 years ago

#24106 closed enhancement (fixed)

Install packages in temporary root before copying to $SAGE_LOCAL (simplified)

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: build Keywords:
Cc: mkoeppe, jdemeyer, jhpalimieri Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 97ebe8e (Commits) Commit: 97ebe8e6bdeb6ed2db9fd0d2374107b7b5fd8039
Dependencies: #24017 Stopgaps:

Description

This is a stripped-down version of the patch in #22509, demonstrating the bare minimum functionality required to support staged installation via DESTDIR. It does not add support for this to all packages as #22509 does. That will require additional followups.

In particular, this supports passing a SAGE_DESTDIR variable to spkg build scripts. In the case of packages installed with make install, this is passed to make as DESTDIR=$SAGE_DESTDIR. Not all packages that use make support this (but most do). For those that don't this is a no-op. The sage-spkg command then takes advantage of this by setting $SAGE_DESTDIR to a standard value before running the package build/install scripts. It then uses the temporary install directory to generate a manifest for the package. Currently this can be seen in action with the patch package (the only package currently using sdh_make_install). With this patch applied:

$ ./sage -f patch
...
$ cat local/var/lib/sage/installed/patch-2.7.5
{
    "package_name": "patch",
    "package_version": "2.7.5",
    "install_date": "Wed Oct 25 14:32:16 UTC 2017",
    "system_uname": "Linux sage-docker 3.13.0-123-generic #172-Ubuntu SMP Mon Jun 26 18:04:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux",
    "sage_version": "SageMath version 8.1.beta7, Release Date: 2017-10-03",
    "test_result": "",
    "files": [
        "bin/patch",
        "share/man/man1/patch.1"
    ]
}

The installed files are listed relative to $SAGE_LOCAL, and does not include empty directories (in a git-like manner). This is because directories are not typically owned by a single package, only files. In the rare case where an empty directory needs to be owned by a specific package, a placeholder file should be added to that directory (this is typical in some Linux distributions as well).

The same could be done easily for Python packages by having sdh_pip_install add --root=${SAGE_DESTDIR}, but currently that breaks a tiny set of Python packages whose build scripts do not fully support this kind of staged installation, so their case will be addressed separately.

Change History (38)

comment:1 Changed 3 years ago by embray

  • Cc mkoeppe jdemeyer jhpalimieri added
  • Component changed from PLEASE CHANGE to build
  • Status changed from new to needs_review

CC'd people who have already shown interest in this work.

comment:2 Changed 3 years ago by embray

  • Dependencies set to #24017

Depends on #24017 for merge consistency.

comment:3 Changed 3 years ago by git

  • Commit changed from 5be15df378dadda50289eb7fd0158eda77583750 to aa61235596bc867ece2e5b2996d54be290157cc7

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

aa61235Fix typo in comment

comment:4 Changed 3 years ago by embray

The patchbot failure is due to #24092 so I'm going to merge that *sigh*

comment:5 Changed 3 years ago by git

  • Commit changed from aa61235596bc867ece2e5b2996d54be290157cc7 to 4d0ad4ef0d35074d389421ef530253614d488398

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

6dd69fdAdd pip to brial's dependencies, replace the use of PIP_INSTALL as per #24014.
4d0ad4eMerge branch 'u/fbissey/brial_pip' into u/embray/build/destdir-2

comment:6 Changed 3 years ago by dimpase

What is the proof of concept package to test with this ticket? It's not clear to me.

Last edited 3 years ago by dimpase (previous) (diff)

comment:7 Changed 3 years ago by embray

Patch, mainly.

comment:8 Changed 3 years ago by jdemeyer

I'm getting a 500 Internal Server Error when clicking the branch on this ticket.

comment:9 Changed 3 years ago by jdemeyer

What is $(pwd) in this case?

export SAGE_DESTDIR="$(pwd)/inst"

Can we be more explicit and write

export SAGE_DESTDIR="${SAGE_BUILD_DIR}/${PKG_NAME}/inst"

comment:10 Changed 3 years ago by jdemeyer

I'd rather wait until #24025 has been merged in a beta before fully reviewing this.

comment:11 Changed 3 years ago by embray

Well the "milestone" for #24025 is set to 8.1, but that is apparently meaningless...

comment:12 Changed 3 years ago by embray

  • Milestone changed from sage-8.1 to sage-8.2

comment:13 follow-up: Changed 3 years ago by dimpase

with ccache installed, I get a problem with this ticket; it tries to rebuild ccache, and at the very end of the installation of ccache I get

Copying package files from temporary location /home/dima/Sage/sage-dev/local/var/tmp/sage/build/ccache-3.2.2.p0/inst to /home/dima/Sage/sage-dev/local
cp: cannot create regular file '/home/dima/Sage/sage-dev/local/bin/ccache': Text file busy
************************************************************************
Error copying files for ccache.
************************************************************************

So probably one needs to somehow uninstall ccache beforehand...

comment:14 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. As mentioned in 9, it would be nice to more explicit here:
    export SAGE_DESTDIR="$(pwd)/inst"
    
  1. It could possibly be a lot more efficient (and also simpler) to move instead of copy the files from SAGE_DESTDIR to SAGE_LOCAL. Is there any good reason to keep the files in SAGE_DESTDIR?

comment:15 in reply to: ↑ 13 Changed 2 years ago by jdemeyer

Replying to dimpase:

cp: cannot create regular file '/home/dima/Sage/sage-dev/local/bin/ccache': Text file busy

This is also fixed by moving instead of copying (on my Linux box at least, but that might be POSIX-specified behaviour)

comment:16 Changed 2 years ago by embray

On occasion I've found it useful for debugging purposes to be able to inspect the contents of the destdir. That said, as long as the system is working correctly, the results can just as easily be inspected directly in $SAGE_LOCAL, so I wouldn't be opposed to giving move a try.

comment:17 Changed 2 years ago by git

  • Commit changed from 4d0ad4ef0d35074d389421ef530253614d488398 to c0cb165d227eee2c2c7bd4014acee542a2865051

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

ce7e0a4Add the ability to temporarily install to an alternate installation root
c0cb165Fix typo in comment

comment:18 Changed 2 years ago by embray

Turns out this is also broken until and unless at least a few packages get some small updates to their spkg-installs to support this mode of installation.

comment:19 Changed 2 years ago by dimpase

what are these small updates? are they already in?

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

No, I'll push them to this branch once I know what they are (i.e. after the build completes successfully). I think it's small enough for now that it's okay to roll into this.

comment:21 in reply to: ↑ 20 Changed 2 years ago by embray

Replying to embray:

No, I'll push them to this branch once I know what they are (i.e. after the build completes successfully). I think it's small enough for now that it's okay to roll into this.

If it turns out to be more I'll split it out. This is mostly a matter of some packages that were updated in #24025 that perform a few additional actions on files in $SAGE_LOCAL that really should be working on $SAGE_DESTDIR$SAGE_LOCAL.

comment:22 Changed 2 years ago by git

  • Commit changed from c0cb165d227eee2c2c7bd4014acee542a2865051 to 97ebe8e6bdeb6ed2db9fd0d2374107b7b5fd8039

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

6bc7332Make $SAGE_DESTDIR explicitly relative to $SAGE_BUILD_DIR, and not whatever
ed41210Move files from SAGE_DESTDIR into SAGE_LOCAL instead of copying.
91dfc13ecl: create post-build links in SAGE_DESTDIR instead of directly in SAGE_LOCAL
edf5898mpfi: perform this sanity check relative to $SAGE_DESTDIR
1398d1dpatch: On Cygwin, ensure the .manifest file is installed relative to $SAGE_DESTDIR
97ebe8ereadline: these post-build steps should be performed relative to $SAGE_DESTDIR

comment:23 Changed 2 years ago by embray

I added the aforementioned tweaks. It's likely others are needed, but these were the minimal fixes I needed for a successful build from scratch.

The one change that's a bit sketchy is the removal of the "sanity check" from patch's spkg-install. I really don't think it's needed at all, but if we really wanted to keep it it would make more sense as a post-install step.

#22509 has a feature of allowing packages to include a spkg-postinst script that is run on the package after it's already been fully copied into $SAGE_LOCAL. I left it out of this ticket to keep things simple, but I could add it back in, as it doesn't add very much complexity at all.

comment:24 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:25 follow-up: Changed 2 years ago by jdemeyer

Looks good on first sight. Am I right that this affects only packages using sdh_make_install?

comment:26 in reply to: ↑ 25 Changed 2 years ago by embray

Replying to jdemeyer:

Looks good on first sight. Am I right that this affects only packages using sdh_make_install?

Right now, yes. Obviously long-term it will affect all packages (as is the case for the original #22509), but for this ticket those are the only packages that are affected.

comment:27 Changed 2 years ago by jdemeyer

Is blocking internet access to unknown ports "a thing" in France? I'm in Besançon right now and both the university network as well as my hotel network block SSH access (on a non-standard port) to a testing machine that I use for testing build-related tickets such as this one. I'd rather not reinstall Sage from scratch on my laptop, so could you please double-check that make distclean; make ptest passes with this ticket applied?

comment:28 Changed 2 years ago by embray

I don't think it's "a thing" in France any more than it is at any other random network. I've also had problems at my university with very tight firewalls.

comment:29 Changed 2 years ago by embray

I feel like I just tested that that works yesterday, but I'll do it one more time....

comment:30 Changed 2 years ago by embray

The recent build failure of this on the Cygwin patchbot appears to be unrelated, though it's not entirely clear to me what happened. I have a feeling there are still some unresolved race conditions involved in running DLL rebase.

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

Confirmed, build from scratch worked, at least on Linux. I'm going to try on OSX too. I now have (supposedly) access to an OSX machine to test on, though I haven't actually tried building Sage on it yet...

comment:32 in reply to: ↑ 31 Changed 2 years ago by dimpase

Replying to embray:

Confirmed, build from scratch worked, at least on Linux. I'm going to try on OSX too. I now have (supposedly) access to an OSX machine to test on, though I haven't actually tried building Sage on it yet...

I am using an OSX machine at LRI, trac.lri.fr, to run tests, etc. If it's the same as you mean to use, it's probably better not to try doing it at the same time...

If you like I can test this branch there, as I'm planning to try the new beta from scratch anyway.

comment:33 Changed 2 years ago by jdemeyer

Please do test!

comment:34 Changed 2 years ago by embray

Worked fine for me on Cygwin locally--whatever went wrong with the patchbot is probably an unrelated fluke.

comment:35 follow-up: Changed 2 years ago by dimpase

it builds on OSX just fine. Any other tests to run?

comment:36 in reply to: ↑ 35 Changed 2 years ago by embray

Replying to dimpase:

it builds on OSX just fine.

Thanks for checking that!

Any other tests to run?

Not as far as I'm concerned. It might be nice to get a patch bot to say it's good. I think I'm going to reconfigure the docker patchbot to run "unsafe" tickets.

comment:37 Changed 2 years ago by jdemeyer

  • Reviewers set to Dima Pasechnik, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:38 Changed 2 years ago by vbraun

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