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: | 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, GitHub, GitLab) | 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 5 years ago by
- Cc mkoeppe jdemeyer jhpalimieri added
- Component changed from PLEASE CHANGE to build
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
- Dependencies set to #24017
Depends on #24017 for merge consistency.
comment:3 Changed 5 years ago by
- Commit changed from 5be15df378dadda50289eb7fd0158eda77583750 to aa61235596bc867ece2e5b2996d54be290157cc7
Branch pushed to git repo; I updated commit sha1. New commits:
aa61235 | Fix typo in comment
|
comment:4 Changed 5 years ago by
The patchbot failure is due to #24092 so I'm going to merge that *sigh*
comment:5 Changed 5 years ago by
- Commit changed from aa61235596bc867ece2e5b2996d54be290157cc7 to 4d0ad4ef0d35074d389421ef530253614d488398
comment:6 Changed 5 years ago by
What is the proof of concept package to test with this ticket? It's not clear to me.
comment:7 Changed 5 years ago by
Patch, mainly.
comment:8 Changed 5 years ago by
I'm getting a 500 Internal Server Error when clicking the branch on this ticket.
comment:9 Changed 5 years ago by
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
I'd rather wait until #24025 has been merged in a beta before fully reviewing this.
comment:11 Changed 5 years ago by
Well the "milestone" for #24025 is set to 8.1, but that is apparently meaningless...
comment:12 Changed 5 years ago by
- Milestone changed from sage-8.1 to sage-8.2
comment:13 follow-up: ↓ 15 Changed 5 years ago by
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
- Status changed from needs_review to needs_work
- As mentioned in 9, it would be nice to more explicit here:
export SAGE_DESTDIR="$(pwd)/inst"
- It could possibly be a lot more efficient (and also simpler) to move instead of copy the files from
SAGE_DESTDIR
toSAGE_LOCAL
. Is there any good reason to keep the files inSAGE_DESTDIR
?
comment:15 in reply to: ↑ 13 Changed 5 years ago by
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
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
- Commit changed from 4d0ad4ef0d35074d389421ef530253614d488398 to c0cb165d227eee2c2c7bd4014acee542a2865051
comment:18 Changed 5 years ago by
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
what are these small updates? are they already in?
comment:20 follow-up: ↓ 21 Changed 5 years ago by
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
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
- Commit changed from c0cb165d227eee2c2c7bd4014acee542a2865051 to 97ebe8e6bdeb6ed2db9fd0d2374107b7b5fd8039
Branch pushed to git repo; I updated commit sha1. New commits:
6bc7332 | Make $SAGE_DESTDIR explicitly relative to $SAGE_BUILD_DIR, and not whatever
|
ed41210 | Move files from SAGE_DESTDIR into SAGE_LOCAL instead of copying.
|
91dfc13 | ecl: create post-build links in SAGE_DESTDIR instead of directly in SAGE_LOCAL
|
edf5898 | mpfi: perform this sanity check relative to $SAGE_DESTDIR
|
1398d1d | patch: On Cygwin, ensure the .manifest file is installed relative to $SAGE_DESTDIR
|
97ebe8e | readline: these post-build steps should be performed relative to $SAGE_DESTDIR
|
comment:23 Changed 5 years ago by
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
- Status changed from needs_work to needs_review
comment:25 follow-up: ↓ 26 Changed 5 years ago by
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
comment:27 Changed 5 years ago by
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
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
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
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: ↓ 32 Changed 5 years ago by
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
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
Please do test!
comment:34 Changed 5 years ago by
Worked fine for me on Cygwin locally--whatever went wrong with the patchbot is probably an unrelated fluke.
comment:35 follow-up: ↓ 36 Changed 5 years ago by
it builds on OSX just fine. Any other tests to run?
comment:36 in reply to: ↑ 35 Changed 5 years ago by
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
- Reviewers set to Dima Pasechnik, Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:38 Changed 5 years ago by
- Branch changed from u/embray/build/destdir-2 to 97ebe8e6bdeb6ed2db9fd0d2374107b7b5fd8039
- Resolution set to fixed
- Status changed from positive_review to closed
CC'd people who have already shown interest in this work.