Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

singularity-tools: miscellaneous fixes #332437

Merged
merged 8 commits into from
Aug 10, 2024

Conversation

ShamrockLee
Copy link
Contributor

Description of changes

This pull request introduces fixes targeting the singularity-tools build helper set.

Two backward-incompatible changes include:

  • Removing the storeDir argument from the override interface of singularity-tools.
    • In the current implementation, storeDir is used interchangeably with builtins.storeDir. Besides, storeDir will be naturally reflected by builtins.storeDir. We could only configure the store path inside the container, which is not covered by the current implementation.
  • Remove two build helpers, shellScript and mkLayer.
    • These two build helpers are no longer involved in image-building.

Split from #268199

Cc: @SomeoneSerge

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable: (tested apptainer.tests.image-hello-cowsay and singularity.tests.image-hello-cowsay)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/) (tested the execution of the produced images.)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog labels Aug 5, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Aug 5, 2024
ShamrockLee and others added 3 commits August 9, 2024 09:07
Deprecate singularity-tools.mkLayer and singularity-tools.shellScript,
for they are no longer related to image building.

Use writers.writeBash instead of singularity-tools.shellScript.
@@ -105,14 +122,14 @@ rec {

# Create runScript and link shell
if [ ! -e bin/sh ]; then
ln -s ${runtimeShell} bin/sh
ln -s ${lib.getExe bashInteractive} bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is runtimeShell wrong and bashInteractive right? I mean, seems to make sense, but I fail to spell it out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtimeShell is provided by runtimeShellPackage (i.e., bash). Current implementation links runtimeShell to container's /bin/sh, which is then executed by runscript.

When calling apptainer run or singularity run, it executes runscript and starts a (supposedly) interactive session. Due to the lack of line-editing functionality of the non-interacive bash, control characters, including arrow keys, are not handled correctly.

This change solves this problem by linking to /bin/sh bashInteractive instead, the way NixOS does by default.

You can see the difference by running

apptainer run "$(nix-build -A apptainer.tests.image-hello-cowsay)"

Before and after this change.

@SomeoneSerge SomeoneSerge merged commit a3a5fb4 into NixOS:master Aug 10, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants