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: init 2.2 #20408

Merged
merged 1 commit into from
Nov 14, 2016
Merged

singularity: init 2.2 #20408

merged 1 commit into from
Nov 14, 2016

Conversation

jbedo
Copy link
Contributor

@jbedo jbedo commented Nov 14, 2016

Motivation for this change

Singularity is a container system similar to docker for mobile compute. It has a strong focus on HPC use. I have added a build expression for Singularity and also some support for building Singularity images. The image building interface is inspired by dockerTools.buildImage.

I have tested images produced by my buildImage function both on my NixOS install using the Singularity expression in this pull request. I have also tested the images running on my HPC cluster on a standard Linux setup.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Copy link
Member

@rasendubi rasendubi left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, otherwise looks good.

I'm not familiar with virtualization, so I haven't reviewed build-support/singularity well.

@@ -0,0 +1,32 @@
{ stdenv
, sudo
Copy link
Member

Choose a reason for hiding this comment

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

Not used

./autogen.sh
'';

meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to maintain the package?

@@ -281,6 +281,8 @@ in
nixBufferBuilders = import ../build-support/emacs/buffer.nix { inherit (pkgs) lib writeText; inherit (emacsPackagesNg) inherit-local; };

pathsFromGraph = ../build-support/kernel/paths-from-graph.pl;

singularityTools = callPackage ../build-support/singularity { };
Copy link
Member

Choose a reason for hiding this comment

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

singularity-tools is preferred spelling. (We're moving from camelCase to this-one for package names.)

Copy link
Member

Choose a reason for hiding this comment

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

Might be good to rename to build-support/singularity-tools, so attribute path matches the file name.

cd disk

# Run root script
${if runAsRoot != null then ''
Copy link
Member

Choose a reason for hiding this comment

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

You can use lib.optionalString here:

''
  ${lib.optionalString (runAsRoot != null) ''
    string contents
  ''}
''

@jbedo
Copy link
Contributor Author

jbedo commented Nov 14, 2016

Thanks for the feedback! I have made changes to address your comments. I'm happy to be the maintainer, so I have added myself as the maintainers list.

Copy link
Member

@rasendubi rasendubi left a comment

Choose a reason for hiding this comment

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

Looks good to me ;)


buildInputs = [ autoconf automake libtool ];

preConfigure = ''
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use the autoreconfHook instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed to autoreconfHook and retested, works perfectly :).

@@ -60,6 +60,7 @@
bcarrell = "Brandon Carrell <brandoncarrell@gmail.com>";
bcdarwin = "Ben Darwin <bcdarwin@gmail.com>";
bdimcheff = "Brandon Dimcheff <brandon@dimcheff.com>";
bedoj = "Justin Bedő <cu@cua0.org>";
Copy link
Member

Choose a reason for hiding this comment

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

@jbedo, could you rename to jbedo, so it matches your GitHub nick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry muscle memory, fixed!

name = "singularity-${version}";
version = "2.2";

src = fetchFromGitHub {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realise that was preferable to fetchFromGitHub; I'll make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done & retested :)

@rasendubi
Copy link
Member

rasendubi commented Nov 14, 2016

@FRidh, @jbedo, fetchFromGitHub is in fact preferred and does the same thing:

let # fix syntax highlight
  fetchFromGitHub = {
    owner, repo, rev, name ? "${repo}-${rev}-src",
    ... # For hash agility
  }@args: fetchzip ({
    inherit name;
    url = "https://github.com/${owner}/${repo}/archive/${rev}.tar.gz";
    meta.homepage = "https://github.com/${owner}/${repo}/";
  } // removeAttrs args [ "owner" "repo" "rev" ]) // { inherit rev; };

@FRidh
Copy link
Member

FRidh commented Nov 14, 2016

There is always something new to learn with Nix(pkgs). I was under the impression it was a wrapper around fetchgit. Thanks for showing.

@jbedo
Copy link
Contributor Author

jbedo commented Nov 14, 2016

Ok, reverted back to fetchFromGitHub :).

@rasendubi rasendubi merged commit 7f51891 into NixOS:master Nov 14, 2016
@rasendubi
Copy link
Member

Thanks for your contribution! Sorry for so many review rounds ;)

@jbedo jbedo deleted the singularity branch November 15, 2016 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants