Opened 5 years ago

Closed 5 years ago

#24106 closed enhancement (fixed)

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

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

Status badges

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 5 years ago by Erik Bray

Cc: Matthias Köppe Jeroen Demeyer jhpalimieri added
Component: PLEASE CHANGEbuild
Status: newneeds_review

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

comment:2 Changed 5 years ago by Erik Bray

Dependencies: #24017

Depends on #24017 for merge consistency.

comment:3 Changed 5 years ago by git

Commit: 5be15df378dadda50289eb7fd0158eda77583750aa61235596bc867ece2e5b2996d54be290157cc7

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

aa61235Fix typo in comment

comment:4 Changed 5 years ago by Erik Bray

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

comment:5 Changed 5 years ago by git

Commit: aa61235596bc867ece2e5b2996d54be290157cc74d0ad4ef0d35074d389421ef530253614d488398

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 5 years ago by Dima Pasechnik

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

Version 0, edited 5 years ago by Dima Pasechnik (next)

comment:7 Changed 5 years ago by Erik Bray

Patch, mainly.

comment:8 Changed 5 years ago by Jeroen Demeyer

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

comment:9 Changed 5 years ago by Jeroen Demeyer

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 5 years ago by Jeroen Demeyer

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

comment:11 Changed 5 years ago by Erik Bray

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

comment:12 Changed 5 years ago by Erik Bray

Milestone: sage-8.1sage-8.2

comment:13 Changed 5 years ago by Dima Pasechnik

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 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_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 5 years ago by Jeroen Demeyer

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 5 years ago by Erik Bray

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 5 years ago by git

Commit: 4d0ad4ef0d35074d389421ef530253614d488398c0cb165d227eee2c2c7bd4014acee542a2865051

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 5 years ago by Erik Bray

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 5 years ago by Dima Pasechnik

what are these small updates? are they already in?

comment:20 Changed 5 years ago by Erik Bray

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 5 years ago by Erik Bray

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 5 years ago by git

Commit: c0cb165d227eee2c2c7bd4014acee542a286505197ebe8e6bdeb6ed2db9fd0d2374107b7b5fd8039

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 5 years ago by Erik Bray

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 5 years ago by Erik Bray

Status: needs_workneeds_review

comment:25 Changed 5 years ago by Jeroen Demeyer

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

comment:26 in reply to:  25 Changed 5 years ago by Erik Bray

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 5 years ago by Jeroen Demeyer

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 5 years ago by Erik Bray

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 5 years ago by Erik Bray

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

comment:30 Changed 5 years ago by Erik Bray

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 Changed 5 years ago by Erik Bray

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 5 years ago by Dima Pasechnik

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 5 years ago by Jeroen Demeyer

Please do test!

comment:34 Changed 5 years ago by Erik Bray

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

comment:35 Changed 5 years ago by Dima Pasechnik

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

comment:36 in reply to:  35 Changed 5 years ago by Erik Bray

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 5 years ago by Jeroen Demeyer

Reviewers: Dima Pasechnik, Jeroen Demeyer
Status: needs_reviewpositive_review

comment:38 Changed 5 years ago by Volker Braun

Branch: u/embray/build/destdir-297ebe8e6bdeb6ed2db9fd0d2374107b7b5fd8039
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.