Containerize: accommodate nested or pre-existing spack-env paths #41558
Conversation
The current `mkdir {{ paths.environment }}` will generate an error if:
* `{{ paths.environment }}` already exists, or
* `{{ paths.environment }}` is nested in non-existing dirs.
Adding `-p` to the command will make this robust to both possibilities.
|
Isn't a pre-existing directory more likely an indication of unexpected behavior? I'd be worried that this will silently overwrite a |
|
On the other hand, how likely it is that a |
|
One use case that I can imagine is that someone uses a base image that is itself the outcome of a spack containerize run. This could fit in a strategy of layering containers: env1 installs core dependencies, env2 uses env1 image as base and installs dependent packages (reusing what's in /opt/software), env3 uses env1 as well to install other dependent packages (also reusing /opt/software), etc. This would cause the env1 spack.yaml to be overwritten by the env2 and env3 spack.yamls. Yes, maybe this is a bit contrived, and maybe the 'damage' is minimal in this case. |
Yeah, if you don't strong oppose to this solution, I would be for merging this PR. |
@wdconinc Just out of curiosity, in this scenario, which spack.yaml would be the right one to keep? In any case, if we believe it is important to protect against silently overwriting an existing spack.yaml file, we could do something like: At least that would preserve the behavior of throwing an error during build, if the environment dir pre-exists and contains a spack.yaml file. |
I would just want spack to error out instead of picking one over the other in that case :-) Maybe |
Oh, nice. I didn't know about that bash option. So then how about (edited to correct path): (btw I think it's odd that the If you both agre with the above, I will update my PR and rebase with develop. |
|
Sounds good to me. |
Set noclobber bash option when writing manifest
|
I don't have the ability to add a reviewer to the PR. Could one of you maybe add yourself (and approve it if you think it's ready?) |
|
Thanks @odoublewen ! |
) The current `mkdir {{ paths.environment }}` will generate an error if: * `{{ paths.environment }}` already exists, or * `{{ paths.environment }}` is nested in non-existing dirs. Adding `-p` to the command will make this robust to both possibilities. Set noclobber bash option when writing manifest.
…ck#41558) The current `mkdir {{ paths.environment }}` will generate an error if: * `{{ paths.environment }}` already exists, or * `{{ paths.environment }}` is nested in non-existing dirs. Adding `-p` to the command will make this robust to both possibilities. Set noclobber bash option when writing manifest.
…ck#41558) The current `mkdir {{ paths.environment }}` will generate an error if: * `{{ paths.environment }}` already exists, or * `{{ paths.environment }}` is nested in non-existing dirs. Adding `-p` to the command will make this robust to both possibilities. Set noclobber bash option when writing manifest.
) The current `mkdir {{ paths.environment }}` will generate an error if: * `{{ paths.environment }}` already exists, or * `{{ paths.environment }}` is nested in non-existing dirs. Adding `-p` to the command will make this robust to both possibilities. Set noclobber bash option when writing manifest.
…ck#41558) The current `mkdir {{ paths.environment }}` will generate an error if: * `{{ paths.environment }}` already exists, or * `{{ paths.environment }}` is nested in non-existing dirs. Adding `-p` to the command will make this robust to both possibilities. Set noclobber bash option when writing manifest.
fixes #41656
The current
mkdir {{ paths.environment }}will generate an error if:{{ paths.environment }}already exists, or{{ paths.environment }}is nested in non-existing dirs.Adding
-pto the command will make this robust to both possibilities.Why would
{{ paths.environment }}pre-exist? Sometimes I need to copy a custom spack package repo into my spack environment, to provide spack packages for proprietary software. I like the custom repo to stay relative to thespack.yamlfile for portability (i.e. so that thespack.yamlfile can work both inside and outside of the containerize command.)cc: @hartzell