buildcache push always --allow-root#38878
Conversation
spackbot-app
bot
added
binary-packages
commands
core
Without --allow-root spack cannot push binaries that contain paths in binaries. This flag is almost always needed, so I don't see the point of requiring users to spell it out. Even without --allow-root, rpaths would still have to be patched, so the flag is not there to guarantee binaries are not modified on install. So, this PR makes --allow-root the default, and drops the code required for it.
haampie
force-pushed
the
buildcache/make-allow-root-a-noop
branch
from
8c5615b to
52da80a
Compare
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
Basically LGTM. Maybe remove something more.
lib/spack/spack/cmd/buildcache.py
Outdated
| print("Relocatable nodes") | ||
| print("--------------------------------") | ||
| print(spec.tree(status_fn=spack.relocate.is_relocatable)) | ||
| print(spec.tree(status_fn=lambda s: True)) |
There was a problem hiding this comment.
Wondering if we can completely remove the:
spack buildcache preview command
It doesn't seem to be synced with other commands, and it's not env aware...
There was a problem hiding this comment.
Ok, this was introduced in #9199 We can safely remove it if we get rid of --allow-root.
There was a problem hiding this comment.
Then I'll just remove the output of the command, but leave the deprecation message, so it doesn't error if someone had it in their ci script or so.
|
Hi @gartung, since you've worked on this before, can you share your opinions? Two parts:
Dropping the old behavior also allows us to drop the dependency on |
|
I am OK with this as long as it passes regression tests. It it meant to catch compiled in paths that some older programs use to locate config files, etc. |
…tion message -- dont remove it entirely, since that may pbreak ci for people who may use this command (if any)
…on (spack#38878) Without --allow-root spack cannot push binaries that contain paths in binaries. This flag is almost always needed, so there is no point of requiring users to spell it out. Even without --allow-root, rpaths would still have to be patched, so the flag is not there to guarantee binaries are not modified on install. This commit makes --allow-root the default, and drops the code required for it. It also deprecates `spack buildcache preview`, since the command made sense only with --allow-root. As a side effect, Spack no longer depends on binutils for relocation
…on (spack#38878) Without --allow-root spack cannot push binaries that contain paths in binaries. This flag is almost always needed, so there is no point of requiring users to spell it out. Even without --allow-root, rpaths would still have to be patched, so the flag is not there to guarantee binaries are not modified on install. This commit makes --allow-root the default, and drops the code required for it. It also deprecates `spack buildcache preview`, since the command made sense only with --allow-root. As a side effect, Spack no longer depends on binutils for relocation
3 participants
closes #7237
closes #11620
Without --allow-root spack cannot push binaries that contain paths in
binaries. This flag is almost always needed, so I don't see the point of
requiring users to spell it out. Even without --allow-root, rpaths would
still have to be patched, so the flag is not there to guarantee binaries
are not modified on install, and in fact relocation is still in dangerous
territory, because patchelf will have to rewrite the dynamic section,
which hasn't been without issues.
So, this PR makes --allow-root the default, and drops the code required
for it.
This also allows us to drop binutils
stringsas a dependency.