Ready to Merge: Update/package singularity #11094
Conversation
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
|
And heads up (some future release) of Singularity will use go modules, which hypothetically should make the install easier! https://github.com/sylabs/singularity/pull/3184 |
|
ping - do you need anything else from me? |
|
Awesome! There's a new rocket emoji response! I'll save that one for after you test :) |
|
[edit: add note about warning when setting GOPATH] Here's an alternate implementation of this. It overrides the staging functionality and just moves the unpacked source tree into location that go expects to find it. A couple of other changes which your version will also need:
The This warns about # Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)
from spack import *
import os
import shutil
import tempfile
import llnl.util.tty as tty
class SingularityAlt(MakefilePackage):
'''Singularity is a container technology focused on building portable
encapsulated environments to support "Mobility of Compute" For older
versions of Singularity (pre 3.0) you should use singularity-legacy,
which has a different install base (Autotools).
'''
homepage = "https://www.sylabs.io/singularity/"
url = "https://github.com/sylabs/singularity/releases/download/v3.1.0/singularity-3.1.0.tar.gz"
git = "https://github.com/singularityware/singularity.git"
# ##########################################################################
# 3.1.1 Release Branch (GoLang)
version('develop', branch='master')
version('3.1.1', '158f58a79db5337e1d655ee0159b641e42ea7435')
depends_on('go')
depends_on('libuuid')
depends_on('libgpg-error')
depends_on('squashfs')
depends_on('git')
phases = ['configure', 'build', 'install']
# Where go traditionally thinls the source should be.
def singularity_dir(self):
return join_path(self.stage.path, 'src/github.com/sylabs/singularity')
# Unpack the tarball as usual, then move it to the traditional location
def do_stage(self, mirror_only=False):
super(SingularityAlt, self).do_stage(mirror_only)
shutil.move(self.stage.source_path, self.singularity_dir())
def configure(self, spec, prefix):
with working_dir(self.singularity_dir()):
configure = Executable('./mconfig --prefix=%s' % prefix)
configure()
def build(self, spec, prefix):
with working_dir(self.singularity_dir()):
make('-C', 'builddir', parallel=False)
def install(self, spec, prefix):
with working_dir(self.singularity_dir()):
make('install', '-C', 'builddir', parallel=False)
def setup_environment(self, spack_env, run_env):
# Point GOPATH at the top of the staging dir
spack_env.set('GOPATH', self.stage.path) |
|
What are you thinking about doing about the setuid bits? |
|
pps. I'm pretty sure that in my alt implementation above there's a way to tell the package, just once, that |
|
I played golf with this a bit, took advantage of # Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)
from spack import *
import shutil
class SingularityAlt(MakefilePackage):
'''Singularity is a container technology focused on building portable
encapsulated environments to support "Mobility of Compute" For older
versions of Singularity (pre 3.0) you should use singularity-legacy,
which has a different install base (Autotools).
'''
homepage = "https://www.sylabs.io/singularity/"
url = "https://github.com/sylabs/singularity/releases/download/v3.1.1/singularity-3.1.1.tar.gz"
git = "https://github.com/singularityware/singularity.git"
version('develop', branch='master')
version('3.1.1', '158f58a79db5337e1d655ee0159b641e42ea7435')
depends_on('go')
depends_on('libuuid')
depends_on('libgpg-error')
depends_on('squashfs', type='run')
depends_on('git', when='@develop') # mconfig uses it for version info
# Where go traditionally thinks the source should be.
# MakefilePackage uses this as working_dir in build/install steps
@property
def build_directory(self):
return join_path(self.stage.path, 'src/github.com/sylabs/singularity')
# Unpack the tarball as usual, then move it to the traditional location.
def do_stage(self, mirror_only=False):
super(SingularityAlt, self).do_stage(mirror_only)
shutil.move(self.stage.source_path, self.build_directory)
# Hijack the edit stage to run mconfig.
def edit(self, spec, prefix):
with working_dir(self.build_directory):
configure = Executable('./mconfig --prefix=%s' % prefix)
configure()
# Set these for use by MakefilePackage's default build/install methods.
build_targets = ['-C', 'builddir', 'parallel=False']
install_targets = ['install', '-C', 'builddir', 'parallel=False']
def setup_environment(self, spack_env, run_env):
# Point GOPATH at the top of the staging dir.
spack_env.prepend_path('GOPATH', self.stage.path) |
|
[edit: added a note about moving the run_before after the edit def'n] Thinking it might appear less magical, I tried replacing the E.g., commented out the @run_before('edit')
def move_src_tree(self):
shutil.move(self.stage.source_path, self.build_directory)I also tried moving this |
|
@hartzell thanks for looking at the PR! Is there a compelling reason to not use what I developed, with small tweaks to ensure dependencies are installed for tags other than develop? I tested (both older and new Singularity) and it works. I didn't look at setuid because (I don't recall) it was supported for the (now deprecated) previous version of Singularity. As a general rule, I don't add features until users ask for them. I saw in an email (but can't find the comment here) a question about why I didn't have the dependencies used for (non develop) install. The only reason is that I'm completely new at Spack, and didn't know to do this. Anyway, if you get something fully working I can offer to test. Otherwise, I think it would be logical to move forward with the code I wrote. |
|
I started playing with it because it felt icky to have to build outside of the normal place that Spack builds things (it goes to lengths to manage its directories, having a special case seemed like a problem waiting to happen). This, I think, is a genuine shortcoming of your approach. Perhaps someone (@tgamblin, @adamjstewart, @alalazo, @davydden ???) could chime in about whether doing the build in I think that my version is more idiomatic (though I'm likely not unbiased), leveraging the built-ins from You should try your builds in a minimal environment. There's no way to build [new versions of] singularity without Your version didn't work for me, out of the box. I don't have GOPATH defined in my environment. That said, environment changes should be made in a Patching over that, building fails because Adding that dependency still fails for 3.1.0 because you're using the Here's a diff that gets your approach to build both versions on CentOS 7 (ignore the bit where I rename the class so that it can reside alongside mine and the original in my screw-around-tree). [ghartzell@bifx1n03 spack]$ diff -u var/spack/repos/builtin/packages/singularity-van/package.py var/spack/repos/builtin/packages/singularity-van/package.py.orig
--- var/spack/repos/builtin/packages/singularity-van/package.py 2019-04-10 09:04:52.466773041 -0700
+++ var/spack/repos/builtin/packages/singularity-van/package.py.orig 2019-04-10 09:07:20.444913558 -0700
@@ -9,7 +9,7 @@
import tempfile
-class SingularityVan(Package):
+class Singularity(Package):
'''Singularity is a container technology focused on building portable
encapsulated environments to support "Mobility of Compute" For older
versions of Singularity (pre 3.0) you should use singularity-legacy,
@@ -17,20 +17,19 @@
'''
homepage = "https://www.sylabs.io/singularity/"
- url = "https://github.com/sylabs/singularity/releases/download/v3.1.0/singularity-3.1.0.tar.gz"
+ url = "https://github.com/sylabs/singularity/archive/v3.1.0.tar.gz"
git = "https://github.com/singularityware/singularity.git"
# ##########################################################################
# 3.1.0 Release Branch (GoLang)
version('develop', branch='master')
- version('3.1.0', 'd3a963ae85c527521434723b1cf8dda9594bf6c6')
+ version('3.1.0', 'f3f1388f7e904dbcc1f7934740b847a771965019')
- depends_on('go')
- depends_on('libuuid')
- # depends_on('libgpg-error', when='@develop')
- depends_on('squashfs', type='run')
- depends_on('git')
+ depends_on('go', when='@develop')
+ depends_on('libuuid', when='@develop')
+ depends_on('libgpg-error', when='@develop')
+ depends_on('squashfs', when='@develop')
phases = ['configure', 'build', 'install']
@@ -75,7 +74,7 @@
'''
gopath = os.path.join(tempfile.gettempdir(), 'go')
tmpgo = os.path.join(gopath, 'src', 'github.com', 'sylabs', 'singularity')
- os.environ['GOPATH'] = gopath
+ os.environ['GOPATH'] = gopath + ':' + os.environ['GOPATH']
# Create gopath
if not os.path.exists(gopath):You could also replace the calls to |
FWIW, the version in my initial comment is fully working, as far as i know. Its use of |
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
|
Haha, you called it "singularity-van..."
It looks like the formatting of your changes is doing the reverse - your changes are in red (and my original green). I just want to clarify / state that in case anyone else gets confused (I did). I just made some tweaks to address the GOPATH, .tar.gz referenced at url, and the dependencies. I tested for @develop and without, and didn't get any issues. I think there are two choices here. We can either have a "spack maintained" GOPATH that keeps the enormous numbers of dependencies for compile (this will get big very fast, and also be confusing about what was found in the user GOPATH vs. spack), and given cloning from master branches it will be hard to manage when several builds need the same repo) or just create this temporary install location, and then build from there, and cleanly remove. I chose the latter because I thought managing an internal GOPATH would be messy and annoying. Definitely would be okay to do otherwise, but be prepared that soon there will be go modules that don't need this. |
|
[edit: add question mark to first sentence :)] What are you worried about being on GOPATH? It appears that everything that Am I incorrect about what's in its If you were using |
|
"vendor" is expected to be in the present working directory of the build, and Singularity now (current version) has quite a bit of content in there. With modules this will change (and further complicate things) but for now it must be the case that the cloned repo is in GOPATH, and the PWD of the build has vendor in it to be found. You can try it other ways if you want, I spent quite a bit of time and this was what I got working. |
|
Do the current 3.x.y versions (ignoring future use of Go modules, for now) need anything that's not included in the If so, then there's still work to be done because Spack frowns on downloading things during the build/install step; it's a control-freak that likes to do it itself during its stage step, with digest values for repeatability and etc.... If not, then we're nearly in violent agreement, the major difference is this bit:
The big difference here is whether the build should happen within Spack's managed spaces (like all of its other builds do) or elsewhere. The other bits (inheriting from Go modules will eventually introduce another layer of complication. I, personally, hope that applications like Singularity will continue to vendor all of their dependencies (which modules will support) rather than downloading them on the fly. But....
I appreciate the amount of time it takes to get going with Spack. That's why I thought it was worth sharing a working idiomatic alternative rather than just commenting line by line. But, the most important question is, does the Singularity-Van play tunes like the Ice Cream truck? |
|
Singularity-Van serves ice cream and avocados, primarily :) |
|
FWIW,
|
|
Wait, there is no install with root? Singularity is extremely limited without that. That's not super logical to me - is there an active user of the (current) Singularity via Spack that can comment on this? |
|
Let's step back a second. The installation procedure is driven by the use cases of the users installing it, and this is different than I anticipated (I expected an install with root, Singularity doesn't function the same without it). Do you know of a set of users that can comment on how they have used the Spack installation of Singularity (with or without root?) We need to take these into consideration. |
I'm not familiar with singularity so apologies if this is a silly question, but are we talking about root permissions, or the root package in Spack? If we're talking about root permissions, then no, Spack currently doesn't support installing as root for any package, although I have an in-progress PR to support that at #9521 |
|
strange, let me swing back to the (one) issue that I know of where a user asked about Singularity - we should clarify this before moving forward. I'll reference the PR here. |
|
@scheibelp -- Singularity runs containers, like (this is going to give a bunch of people a big headache) Docker but in a way that's much more sensible in an HPC environment. More info here. I'm a big fan! In order to get its full benefits, there's a bit that needs to be If you install it by hand using their instructions, your last step would be something like It looks like #9521 would indeed fix things, at least for interactive builds (I wonder what happens with my Jenkins job?). @vsoch -- I've used the old package in the past, and my solution was to just sudo chmod 4755 $(spack location -i singularity)/path/to/files/that/needed/to/be/setuidI think that having the package in Spack is quite useful, even if people have to do this bit by hand. The alternatives seem to be to build it "by hand" or install it from RPMS that I dig up from somewhere. I try really hard to limit the set of repo's from which I pull RPMS and building it by hand requires either having the prereqs in the base system (I try to run a minimal base system) or link into some Spack tree (at which point...). |
|
@vsoch In my use case, I install in two passes, the first I install singularity's dependencies / everything else as a normally privileged user (which some packages like tar seem to require), then I install singularity as root. It's a pain, but I would consider it a shortcoming in spack's workflow, not the singularity package. |
Given this call: shutil.move(self.stage.source_path,
self.singularity_gopath_dir)I'm nearly certain that the call to
I'm not trying to argue with you, I'm trying to wrestle with the problem. But outside my own head that distinction's probably hard to see and less fun to be part of. Apologies if I'm sounds argumentative. Software that works mysteriously and patches that paper over race conditions (or whatever's going on here) drives me bonkers. That's my weakness (or, sometimes, strength). I can't recreate the problem (heck, your docker image Works For Me(tm)) so the only way I've been able to try to see what's going on is getting you to try things on your system for me. You've been great to team up with on this. That all said, I think we can ask for this to be merged (or perhaps close this and make a shorter, neater PR that refers back to this one). I'd suggest these two changes, use the local diff --git a/var/spack/repos/builtin/packages/singularity/package.py b/var/spack/repos/builtin/packages/singularity/package.py
index 6ad694521..4287773a7 100644
--- a/var/spack/repos/builtin/packages/singularity/package.py
+++ b/var/spack/repos/builtin/packages/singularity/package.py
@@ -55,7 +55,7 @@ class Singularity(MakefilePackage):
source_path = self.stage.source_path
if not os.path.exists(self.singularity_gopath_dir):
tty.debug("Moving {0} to {1}".format(
- self.stage.source_path, self.singularity_gopath_dir))
+ source_path, self.singularity_gopath_dir))
mkdirp(self.sylabs_gopath_dir)
shutil.move(source_path,
self.singularity_gopath_dir)
@@ -67,9 +67,7 @@ class Singularity(MakefilePackage):
# Hijack the edit stage to run mconfig.
def edit(self, spec, prefix):
- tty.info(os.getcwd())
with working_dir(self.build_directory):
- tty.info(os.getcwd())
configure = Executable('./mconfig --prefix=%s' % prefix)
configure() |
|
Those changes would be okay with me! I'm afk but feel free to push and then we can call it done, and pick this up in another issue. Thanks for you collaborative debugging on this, glad we found something that works for both of us. |
- Use the local `source_path` variable consistently instead of `self.stage.source_path` in the debug output, since that's what we're actually using in the code - Drop the info statements from the edit function, they're left over from debugging.
|
[edit, added OS info] Pushed. I tested by building both I'll leave it to you to think about opening up another clean PR using this branch or sticking with this. If you stick with this one, you might edit the title, adding something like "Ready to merge", to help catch the attention of the committers. Yay/Phew. |
|
Woot! I’m still out for my run - I’ll do-all-the-things mentioned above so we can update Singularity and still study this behavior (in another thread). Most likely tomorrow again, I’m pooped. |
|
@hartzell one quick question before closing this up - with an install I don't see any message to alert me to use the script as a final step. It's there: $ ls /tmp/spack/opt/spack/linux-ubuntu16.04-x86_64/gcc-5.4.0/singularity-3.1.1-3n7skikliv4pd6olbouxjtjsdl2j4vla/bin
run-singularity singularity spack_perms_fix.shbut if I didn't know to look for it, I wouldn't. Did you have in mind to tell the user about this? Are we relying on online documentation? |
|
Hey @hartzell - there isn't an issue with singularity / src if we remove the command to create the directory (it shouldn't be there). Did you have a reason for including it? I think it would fix finding two folders in there. |
Output during the build phase currently ends up in the build file. Known issue, mentioned above. @adamjstewart has a fix in this PR, awaiting feedback from @tgamblin. Once this is in place, output will be sent to the terminal, in a standout color. I also included a small blurb in the package info string ( |
|
Re: the
I'm fine w/ removing it, if someone, somewhere runs into a problem it won't be subtle.... |
|
Re^2: the If you only see the error involving 💥 |
|
I just ran through it again with the mkdir commented out, and I made you some create comforts asciinema recording evidence so you believe what happened! https://asciinema.org/a/ZNT6epLmEZ47dLgDKgqLS6wsb?speed=2 And it worked at the end! :D The last bit then, I think would be to remove this extra line, and then give a message to the user to execute that script? And yes, sometimes the error is right in front of our noses. The two directories were there at the same time leading to the wrong property for the self.stage.source_path because... we put it there. |
|
Gotta love asciinema and evidence. Data driven! I can't figure out any way to message the user (as mentioned a bit earlier). At this point I'd say LGTM, based on "Don't let perfect be the enemy of the good." or some similar aphorism. By way of solutions that I've explored.... I just had the cute idea that I could add "caveats" phase to the list of phases, to run after "install" and hook that to send the warning. Sadly, it seems to run in the subprocess also and the warnings still end up in the --- a/var/spack/repos/builtin/packages/singularity/package.py
+++ b/var/spack/repos/builtin/packages/singularity/package.py
@@ -40,6 +40,8 @@ class Singularity(MakefilePackage):
def gopath(self):
return join_path(self.stage.path)
+ phases = ['edit', 'build', 'install', 'caveats']
+
@property
def sylabs_gopath_dir(self):
return join_path(self.gopath, 'src/github.com/sylabs/')
@@ -126,10 +128,13 @@ class Singularity(MakefilePackage):
chmod = which('chmod')
chmod('555', script)
+ def caveats(self, spec, prefix):
+ pass
+
# Until tty output works better from build steps, this ends up in
# the build log. See https://github.com/spack/spack/pull/10412.
- @run_after('install')
- def caveats(self):
+ @run_after('caveats')
+ def perm_script_warnings(self):
tty.warn("""
For full functionality, you'll need to chown and chmod some files
after installing the package. This has security implications.Re-reading the relevant bit of code agrees with the empirical evidence. I also explored adding a new hook and using it, discussion in #11244. |
|
Ah, yes I remember those discussions from before! I'm in complete agreement - let's get this PR finished up, and then update with a message to the user when the protocol is implemented. |
vsoch
changed the title
|
@baberlevi this PR is ready to merge. We've addressed issues with installing Singularity 3.0, and remaining to do is to alert the user (with a message) about running a final script to change permissions. When a method is finished to allow for this, we'll open another PR to add the message. |
The message is already there. If/when #10412 is merged, the message will be sent to the console instead of (or in addition to, depending on #10412) ending up in the build.log (as it does now). We won't need to do anything. If #10412 doesn't merge, then we'll need to figure out what to do next. Here's an example of how it currently appears: |
|
Even better :) |
|
Any committers want to take a swing at this? |
|
Yes please ! ⚾ |
|
I mentioned this PR in https://groups.google.com/forum/#!topic/spack/Ps_pPbHbrvU |
|
I squashed this and hopefully captured the important points in the commit message. The logic added for the go build in Singularity likely ought to be generalized into a new build system. You mention
what is the general expected structure? Also, this creates a chmod script for the new Singularity, but I presume that |
|
Thanks!
I think that, with N =~ 3 (or so), we're getting close to having enough Go based projects in the tree to think about generalizing. The sticky bit, however, is that "it's complicated" and I'm (personally) inclined to wait and see what new packages need before we start pouring concrete. YMMV, however.... There are probably two major cases:
Given a particular structure, some projects are simple enough that they can just use a series of calls to the Independent of this, some projects "vendor" all of their dependencies, using a variety of different tools, though the build steps are probably similar. Vendoring and modules was not well defined in the early modules days and I'm not entire sure how it settled out. The singularity source tree includes all of it's prereqs, which makes it pretty simple. Left to their own devices, packages that do NOT vendor their prereqs will download them at build time, and we all know how we feel about that around here. Package authors will need to set up resources for them and figure out how their package is building and short-circuit it. The modules stuff is only just reaching maturity. The GOPATH stuff will go the way of the dodo, eventually, but it's unclear how long it'll hang around. |
Well, the legacy stuff is legacy, I'm not sure how much energy to put into it. Perhaps one of the current users could comment. The particular set of files to chown and chmod are different, but the concept is straight forward. I believe that it uses a different mechanism to hunt down the @current-singularity-users? I'd be inclined to let it be, but if there's demand. |
|
For Singularity Legacy, I agree with @hartzell - let's not solve problems that aren't being reported by users. If nobody is using / asking for the equivalent script, it's not optimizing use of our time.
Looks good to me :)
So - I want to suggest that we wait on this. Modules are new, yes, but they will still be ready soon enough to warrant not investing a huge amount of time into something that is guaranteed to be deprecated. For Singularity alone modules will be used in the next official release, and we can update to use them. I would even go as far to say that if someone wants to install something with GoLang that doesn't have a module, it would be better use of time to (develop for the future) and do PR to their repos to add modules. And hey, just a few days ago they announced the mirrors are up!. So -1 to add this weirdo GOPATH install business as something proper, +1 to testing Singularity with modules after next release, and then having "that thing we figure out" be the protocol. |
This pull request will close #9698, specifically we are allowing for installation of Singularity (versions 3.0 and up). Specifically:
singularity(3.0.0 and up) and 'singularity-legacy'.Here are some things that I learned that could help others in the future (and we can discuss).
opt/spack/linux-ubuntu16.04-x86_64/gcc-5.4.0/go-1.11.5-73pcics3bti7rafvlgjzmcgtaeogebxg. It has a src folder but this is not GOPATH.GOPATH Troubles
As I mentioned, GOPATH is not defined, but Singularity is expected to be found under
$GOPATH/src/github.com/sylabs/singularity. This led to most of the issues with install - the default location that the repo is dumped into doesn't match that. This means that we are able to compile, but when the time comes to build, the entire cloned repo is expected to be found under GOPATH. If we add the building directory to GOPATH, this actually means it's expected to find<build_dir>/src/github.com/sylabs/singularitywhich doesn't exist. If we try to change the build to some place other than there, we then lose having vendors/ in the present working directory and it breaks.The solution I came up with was to create a temporary GOPATH in /tmp, and in fact we move everything there. This mimics the correct path setup and allows for configure / make / make install without any issues.