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.

Sign up for GitHub

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

Jump to bottom

singularity: fix defaultPath and reflect upstream changes #158486

Merged
merged 6 commits into from
Feb 8, 2023
Merged

singularity: fix defaultPath and reflect upstream changes #158486

merged 6 commits into from
Feb 8, 2023

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Feb 7, 2022 •
edited
Loading

Motivation for this change

Upstream changes:
singularity 3.8.7 -> singularity-legacy 3.8.7 / apptainer 1.0.3 / singularity (singularity-ce) 3.10.4

Build process:

  • Share between different sources.
  • Fix the sed regexp to make defaultPath patch work.
  • Add bin path of bash to the defaultPath to provide sh executable.
  • Set allowGoReference true to allow singularity to compile plugins when building container images.
  • Build with buildGoModule and pass vendorSha256 as a function argument to allow building from non-vendored source.
  • Set doCheck true.
  • Format with nixpkgs-fmt.
  • Add input parameter enableSuid that defaults to false. --with-suid or --without-suid will be passed to mconfig according to enableSuid the upstream's policy to enable / disable the SUID support by default.
    This parameter will be overwrite to false in the NixOS module programs.singularity.
  • Add man page output.

NixOS module programs.singularity:

  • Allow users to specify packages
  • Format with nixpkgs-fmt.

Singularity (the upstream) renamed themselves to Apptainer to distinguish themselves from a fork made by Sylabs Inc..

https://sylabs.io/2021/05/singularity-community-edition
https://apptainer.org/news/community-announcement-20211130

In this PR

  • singularity-legacy is from the original repo before the renaming.
  • apptainer is from the new repo after the renaming.
  • singularity-ce is from the fork of Sylabs Inc..

As apptainer is still at pre-released version 1.0.0-rc1 and the source and the revision of the previous singularity derivation is the same as that of singularity-legacy here,

singularity = singularity-legacy;

is chosen.

WIP: Update the release note. Done.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 7, 2022
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Feb 7, 2022 •
edited
Loading

As for the goPackagePath, the official one (the one explicitly written in each repo) is used. For example, the goPackagePath of singularity-legacy is github.com/hpcng/singularity and singularity-ce github.com/sylabs/singularity. The change of this variable should not be noticeable by most user and the toolchain, since Singularity/Apptainer is meant to be used as an application instead of a library. For those who use in as a library, this PR enables them to choose the goPackagePath corresponding to the source. Users can just override the derivation should they want to mix the source and the path, and the time to build is minimal.

By the way, should we use buildGoModule instead of buildGoPackage? The Nixpkgs Manue seems to recommend the former.

Update: Switched to buildGoModule.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Feb 7, 2022
@ofborg ofborg bot requested a review from jbedo February 7, 2022 16:49
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Feb 7, 2022
@ShamrockLee ShamrockLee force-pushed the singularity-apptainer branch 7 times, most recently from b441b33 to 6090b6b Compare February 8, 2022 02:54
@ShamrockLee ShamrockLee marked this pull request as ready for review February 8, 2022 03:29
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Feb 8, 2022 •
edited
Loading

Change the way how packages are called to preserve the override method.

@ofborg ofborg bot requested a review from kalbasit February 8, 2022 15:39
@ShamrockLee ShamrockLee mentioned this pull request Feb 9, 2022
Closed
@ShamrockLee ShamrockLee force-pushed the singularity-apptainer branch 2 times, most recently from 2839a78 to a0cdde4 Compare February 10, 2022 11:26
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Feb 10, 2022
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Feb 11, 2022 •
edited
Loading

  • Add input parameter withouSuid that defaults to true and pass --without-suid to mconfig to enable non-suid build to run containers.
    This parameter will be overwrite to false in the NixOS module programs.singularity.

  • Add the man page output.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 11, 2022
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Jan 1, 2023 •
edited
Loading

I'll downgrade Apptainer to 1.1.3 before apptainer/apptainer#958 gets fixed by apptainer/apptainer#967 , since it affects the behavior for Apptainer to find the configuration file.

@ShamrockLee ShamrockLee force-pushed the singularity-apptainer branch 3 times, most recently from 41deca7 to eb6d680 Compare January 1, 2023 22:35
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Jan 1, 2023 •
edited
Loading

Just add _apptainer-nixos-overriden-deafult and _singularity-nixos-overriden-default into all-packages.nix with a cool function pkgs.nixos.

Now NixOS users specifying programs.singularity.enable = true without touching options other than package also enjoy the binary cache.

Update: overrideAttrs with meta.description = ""; to make it less confusing when showing inside the nix search result.

@ShamrockLee ShamrockLee requested review from SuperSandro2000 and removed request for jbedo January 1, 2023 22:44
@ofborg ofborg bot requested a review from jbedo January 1, 2023 22:52
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/good-practice-to-make-not-for-install-packages-available-in-the-binary-cache/24407/1

@jbedo jbedo mentioned this pull request Jan 23, 2023
Closed
13 tasks
SuperSandro2000
SuperSandro2000 approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

last nits, other than LGTM

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/singularity/packages.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/singularity/generic.nix Outdated Show resolved Hide resolved
@ShamrockLee ShamrockLee requested review from SuperSandro2000 and removed request for jbedo January 28, 2023 17:34
@jbedo
Copy link
Contributor

jbedo commented Feb 7, 2023

This is great, I think we can merge it after resolving the conflicts.

@ShamrockLee
singularity-tool: format expression
b33d640
@ShamrockLee
nixos/modules/programs/singularity.nix: format expression
136bbe2
@ShamrockLee
all-packages.nix: move the line of singularity per alphabetical ordering
ef09cfe
@ShamrockLee
apptainer, singularity: fix defaultPath and reflect upstream changes
50788d2 
Upstream changes:
singularity 3.8.7 (the legacy) -> apptainer 1.1.3 (the renamed) / singularity 3.10.4 (Sylabs's fork)

Build process:
*   Share between different sources
*   Fix the sed regexp to make defaultPath patch work
*   allowGoReference is now true
*   Provied input parameter removeCompat (default to false)
    that removes the compatible "*singularity*" symbolic links
    and related autocompletion files when projectName != "singularity"
*   Change localstatedir to /var/lib
*   Format with nixpkgs-fmt
*   Fix the defaultPath patching
    and use it instead of the `<executable> path` config directive
    deprecated in Apptainer
*   Provide dependencies for new functionalities such as
    squashfuse (unprivileged squashfs mount)
*   Provide an attribute `defaultPathInputs` to override
    prefix of container runtime default PATH

NixOS module programs.singularity:
*   Allow users to specify packages
*   Place related directories to /var/lib
*   Format with nixpkgs-fmt

singularity-tools:
*   Allow users to specify packages
*   Place related directories to /var/lib when building images in VM
@ShamrockLee
apptainer, singularity: enable non-FHS --fakeroot support
71a8929 
This patch provides input arguments `newuidmapPath` and `newgidmapPath`
for apptainer and singularity to specify the path to the SUID-ed executables
newuidmap and newgidmap where they are not available from the FHS PATH.

As NixOS places those suided executables in a non-FHS position
(/run/wrapper/bin), this patch provides
programs.singularity.enableFakeroot option and implement with the above
input parameters.
@ShamrockLee
apptainer-overriden-nixos,singularity-overriden-nixos: init
d35f5c2
@ShamrockLee
Copy link
Contributor Author

Rebased onto the updated master branch and resolved the merge conflict.

@ofborg ofborg bot requested a review from jbedo February 8, 2023 10:35
@jbedo jbedo merged commit f2ab8c7 into NixOS:master Feb 8, 2023
@ShamrockLee ShamrockLee deleted the singularity-apptainer branch February 9, 2023 22:16
@ShamrockLee ShamrockLee mentioned this pull request May 9, 2023
Closed
@pitkling pitkling mentioned this pull request Jul 21, 2024
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SomeoneSerge SomeoneSerge SomeoneSerge requested changes

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@jbedo jbedo Awaiting requested review from jbedo

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ShamrockLee @SomeoneSerge @jbedo @SuperSandro2000 @GaetanLepage @nixos-discourse @bobby285271