fixing link so that /environment is honored

[GodloveD]

Description of the Pull Request (PR):

Fixes backward compatibility with /environment file

This fixes or addresses the following GitHub issues:

• Ref: /environment not honored in Singularity 2.3 #799

Checkoff for all PRs:

• ✅ I have read the Guidelines for Contributing, and this PR conforms to the stated requirements.
• ✅ I have tested this PR locally with a make test
• ✅ This PR is NOT against the project's master branch
• ✅ I have added myself as a contributor to the Author's file
• ✅ This PR is ready for review and/or merge

Attn: @singularityware-admin

[vsoch]

I think /environment is a symlink? It gets generated from the contents of the singularity metadata env folder (/.singularity.d/env/*)

[vsoch]

I agree we should support older images that have the /environment, but I don't think we should be encouraging people to echo things / write into /environment directly. This is why we added %environment. Do you think it is reasonable to support older images with /environment, but for images created with ability to use %environment we shouldn't encourage the old hacky way?

[GodloveD]

I think that /environment was supposed to be symlinked but instead the thing getting symlinked is /.singularity.d/environment. That's not really maintaining backward compatibility or going with the new scheme either.

There are also things that cannot be done without catting to an /environment file. You may not know what env vars need to be written out to the /environment until after the %setup section has run some things. So there is no way you can just put it in the %environment section.

On a separate note, I have no idea why the travis build failed. ???

[vsoch]

I would advocate for supporting a way to define environment variables during post (that cannot be determined) and still not supporting users echoing into /environment.

[GodloveD]

I think the current method is to echo to /.singularity.d/env/90-environment.sh. That seems much worse and less intuitive than echoing to /environment. Popping the /environment link back to the top level will also mirror the current behavior of /singularity so it will be more consistent.

[vsoch]

I think we moved the file into the metadata folder because it affords a distinct barrier between "container" and "metadata." To have "some in and some out" would be confusing. Perhaps a workaround would be to allow the user to echo to the same path, but via an environment variable set during bootstrap? Eg,

echo "PANCAKES=yesplease" >> $SINGULARITY_ENVIRONMENT

and that way, we can preserve the clean / distinct -ness of the metadata folder, while still making it an intuitive workflow.

[GodloveD]

I should probably say that this is not my idea. I made this PR at @gmkurtzer request following a meeting we had yesterday. So he should probably weigh in.

I don't really see any reason to leave /singularity where it is but to move /environment to the /.singularity.d directory. Seems like it should be one or the other.

Maybe it was a bad idea to pollute the root directory with those files in the first place, but what's done is done. It's a big headache for users if the locations keep changing.

[GodloveD]

And I think that adding an env var that we can echo to will only complicate matters even more. I think changes like this that impact the user interface should not be made lightly.

[vsoch]

ok @gmkurtzer do you want to weigh in here?

[gmkurtzer]

I think those ideas are great @vsoch! Keep in mind too that the %environment scriptlet will do the same thing, but it won't handle dynamically created info like you are talking about. So yes, let's define the environment variable to the environment file such that this can be done portably. Shall we add that feature to this PR or create another? Up to you @GodloveD!

Thanks!

[vsoch]

let's define the environment variable to the environment file such that this can be done portably

cool, so you mean this idea:

echo "PANCAKES=yesplease" >> $SINGULARITY_ENVIRONMENT

I think this makes most sense to support the way we are doing things now, while still allowing an easy workflow for the user to do it dynamically. @GodloveD what are your concerns with this approach?

[gmkurtzer]

I think the $ENVIRONMENT_FILE should point to a new environment file, so %environment doesn't accidently get overwritten by a single >.

@GodloveD, I'm curious too what this would complicate. Can you elaborate?

Thanks!

[vsoch]

oh +1, that makes sense. Too many 🥕 🥕 >>>

[GodloveD]

I think it would complicate matters from the user perspective by adding a 3rd method for setting up the environment. In other words we tell the user "echo to /environment. No wait don't do that anymore now you should use %environment (unless you can't in which case you should echo to /.singularity.d/environmet). No wait now you should echo to an env var instead." This is basically part of the user interface. I think we need to pick a method and stick to it. Since we started with echoing to /environment however flawed that is I think we should still support it.

I'm not saying it complicates the code, I'm saying it complicates the user experience. We should consider how much responsibility we are putting on the user to keep up with every change we make because they have to alter their workflow to keep up.

[vsoch]

Echoing to environment was never "best use" only as "this is the workaround for now." In my mind it's very clear to have these two options:

1. define environment under %environment for runtime, predictable things
2. echo to $SINGULARITY_ENVIRONMENT to do the same thing for dynamic variables.

The others that you mention are simply not options we advocate. They were, and still are, hacks.

[gmkurtzer]

Echo'ing to /environment was the only use case for everything =< 2.2, so for legacy purposes we need to support that (I've heard from multiple people that this broke their existing workflow). The %environment section is new, and the better direction and I love the echo to $SINGULARITY_ENVIRONMENT idea as well (I'd like that redirection to goto a new .env file, perhaps 91-redirects or something like that). @GodloveD, can you add that feature to this PR please?

Thank you all, this is great stuff!

[vsoch]

Just a note from the peanut gallery - the choice of emoticons for commenting on issue comments is just abysmal! I can either laugh, thumbs up or down, throw you a party, act confused, or send love. Not enough emotion choices!!

[gmkurtzer]

It's also not great that I don't get a notification about it either! ... Unless I go back and look, I have no idea you emoted on it. lol

[GodloveD]

@gmkurtzer I'm not sure if this is exactly what you had a in mind, but have a look. This basically just adds an extra command to the %post section to set the variable $SINGULARITY_ENVIRONMENT. User's can echo to it and viola! It creates /.singularity.d/env/91-environment.sh with anything new.

[vsoch]

If we are echoing to a file in the metadata folder, why are we changing it back to still have /environment outside of the metadata folder?

[GodloveD]

I'm enabling both methods. /environment for backward compatibility with older definition files and SINGULARITY_ENVIRONMENT as you suggested for the new way moving forward. This way users can have time to transition to the new method without breaking their current workflow.

[vsoch]

As long as we aren't advocating and continuing support for users echoing to /environment, but to the variable $SINGULARITY_ENVIRONMENT instead, I think that is what we want. By linking to the file and sourcing it we support older images, however what we don't want to do is (continue) advocating for users to echo directly to /environment. That was a hack. it's funny, after all this learning I understand why docker doesn't have sections, they have commands that can appear anywhere when needed. When they say:

ENV FOO BAR

it dually gets defined and also added to their "runtime" list. With our method, we can't easily do that. And funny that, if you think about it, we are now supporting both an environment section and our equivalent of ENV with SINGULARITY_ENVIRONMENT but it's far less elegant.

[gmkurtzer]

I would recommend just adding this to the shell code in deffile-sections.sh instead of the C code.

[GodloveD]

OK. I'll give it a shot. Note that SINGULARITY_ENVIRONMENT is already a
(rather important) variable. If I add it to deffile-sections.sh is it
going to get added to the registry? What kind of havoc would that cause?

[GodloveD]

OK. I'll give it a shot. Note that SINGULARITY_ENVIRONMENT is already a (rather important) variable. If I add it to `deffile-sections.sh` is it going to get added to the registry? What kind of havoc would that cause?

…

On Thu, Jul 20, 2017 at 1:14 PM, Gregory M. Kurtzer < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/bootstrap.c <#800 (comment)> : > @@ -83,6 +83,7 @@ int main(int argc, char **argv) { envar_set("SINGULARITY_DOCKER_USERNAME", singularity_registry_get("DOCKER_USERNAME"), 1); envar_set("SINGULARITY_CACHEDIR", singularity_registry_get("CACHEDIR"), 1); envar_set("SINGULARITY_version", singularity_registry_get("VERSION"), 1); + envar_set("SINGULARITY_ENVIRONMENT", "/.singularity.d/env/91-environment.sh", 1); I would recommend just adding this to the shell code in deffile-sections.sh instead of the C code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#800 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHUUXLbtCtAIlBamTW_n2mWqWyKclrY_ks5sP4rvgaJpZM4OTjsz> .

[gmkurtzer]

Oh, perhaps I missed something. Why does it need to be in the registry? I don't think it would cause any issues, but perhaps I am missing something.

Thoughts?

[GodloveD]

No it totally doesn't need to be in the registry! I was just afraid it might be automatically added to the registry if I set it in deffile-sections.sh. That would be bad. I'll try to set it there and if it works I'll change the PR.

[gmkurtzer]

Ahhh, I understand now. Yeah, it won't get into the registry if we set it in the deffile-sections.sh. Just set it near the top so it is available for %post.

Thanks!

[GodloveD]

OK. Hopefully this will do the trick (and do it properly). So basically now if you bootstrap with a definition file like this:

BootStrap: docker
From: ubuntu:latest

%post
    echo "export BASE='this is rogue 2'" >> $SINGULARITY_ENVIRONMENT

You can do things like this:

$ singularity shell test.img
Singularity: Invoking an interactive shell within container...

Singularity test.img:~> echo $BASE
this is rogue 2

[gmkurtzer]

Looks perfect to me! Merging, thanks!