singularity-tools: miscellaneous fixes (2nd round) #333843
Conversation
|
I found some issues inside this PR and will hold it back until they are resolved. |
Place the VM disk image in a local directory "disk-image" instead of "$out", so that we don't have to delete it to reserve "$out" for the container image.
ShamrockLee
force-pushed
the
singularity-tools-fixes2
branch
from
c58e32b
to
90eb773
Compare
|
Problem solved. It's now ready for review. |
ofborg
bot
added
10.rebuild-darwin: 0
| cp -ar $f ./$f | ||
| done | ||
| while IFS= read -r f; do | ||
| cp -r "$f" "./$f" |
There was a problem hiding this comment.
I wonder why -a is not necessary. I'm seeing that if I do
$ touch a
$ chmod +x a
$ cp a b
$ stat b...bs still has the +x bit, but I don't know why
There was a problem hiding this comment.
I'm seeing that if I do
$ touch a $ chmod +x a $ cp a b $ stat b...
bs still has the +x bit, but I don't know why
For cp, -a (--archive) implies --preserve all, which means preserving all attributes, including mode, ownership, timestamps, links (hard links), context (SELinux or SMACK security context), xattr. cp preserves the file mode by default but not the file ownership.
It requires root privileges to create a file owned by root:root, which is why it requires root privileges to cp -ra from a store object.
I wonder why
-ais not necessary.
The initial purpose of preserving ownership might be to please Nix when using Nix inside the container. However, the container Nix store is inherently broken from Nix's perspective, as it doesn't include the database.
Cc: @jbedo, the original author of singularity-tools.
There was a problem hiding this comment.
It requires root privileges to create a file owned by root:root
It doesn't? In particular, this works:
with import <nixpkgs> { };
runCommand "abc" { } ''
cp -ar ${hello}/ $out/
''
There was a problem hiding this comment.
It requires root privileges to create a file owned by root:root
It doesn't? In particular, this works:
with import <nixpkgs> { }; runCommand "abc" { } '' cp -ar ${hello}/ $out/ ''
Oops! I remember that it wasn't when I first worked on it.
| for c in ${lib.escapeShellArgs contents} ; do | ||
| for f in "$c"/bin/* ; do | ||
| if [ ! -e "bin/$(basename "$f")" ] ; then | ||
| ln -s "$f" bin/ |
There was a problem hiding this comment.
May not be in scope for the PR but it's maybe worth linking
- Avoid symlinking to /bin when building singularity containers (or warn on clashes) #93122
- singularity-tools: Check for /bin/sh existence before symlink #93082
...from here
There was a problem hiding this comment.
I agree with using PATH instead of /bin symlinking. I used it in the definition-based build provided by #224636. Here is our previous discussion on this topic: https://github.com/NixOS/nixpkgs/pull/224636/files#r1160279847
May not be in scope for the PR
I'm unsure if people (we) have reached a consensus about this change, so I didn't include it in this PR.
String-interpolation converts path objects inside `contents` into store paths to ensure they are properly included in the result image. See tests.trivial-builders.references for the necessity of string-interpolation. Quote each string-interpolated content member to accomodates spaces inside.
ShamrockLee
force-pushed
the
singularity-tools-fixes2
branch
from
90eb773
to
37ce47c
Compare
Don't preserve store content ownership to prepare for unprivileged-build workflow.
ShamrockLee
force-pushed
the
singularity-tools-fixes2
branch
from
37ce47c
to
c2eb0aa
Compare
|
I don't see why the "Check pkgs/by-name pkgs-by-name-check" CI check fails. This pull request has nothing to do with
|
There was a problem hiding this comment.
I don't see why the "Check pkgs/by-name pkgs-by-name-check" CI check fails.
Not sure, I restarted the job and it seems fine
There was a problem hiding this comment.
It's GitHub; they had a widespread outage.
There was a problem hiding this comment.
Huh, I thought that was a day ago
There was a problem hiding this comment.
https://www.githubstatus.com/incidents/mrpx4trfk45z they had another one in this area
Description of changes
When rebasing #268199, I found some more minor issues. Here's what I do in this round of fixes:
bashInteractiveandrunScriptFileintoclosureInfoand the resulting container image.singularity-toolsextensible instead of usingrec. This will be useful for singularity-tools: refactor and addrunImageInLinuxVM; apptainer.passthru.tests.exec-image-in-linux-vm: init #268199.contentsbefore copying (instead oftoString) to ensure that path objects get appropriately included in the resulting image.Thank you, @SomeoneSerge, for being so patient in reviewing everything.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.