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

Micromamba: Substitute env vars in .condarc #1423

Merged
merged 13 commits into from
Feb 2, 2022

Conversation

jonashaag
Copy link
Contributor

New version of #1416 with only ${var} support (and $$ support)

@jonashaag
Copy link
Contributor Author

@wolfv this should be ready to go with one minor todo where my C++ skills weren't good enough. How do you make a function that can return a std::string or "None"? I guess you make it return a pointer, but then where do you allocate the std::string? Does std::string x allocate the string on the stack or on the heap? If on the heap, how is it deallocated? What's the "modern" approach to heap allocation in C++?

@wolfv
Copy link
Member

wolfv commented Feb 1, 2022

@jonashaag there are two options -- either to use std::optional<std::string> or std::unique_ptr<std::string>. Both will destroy the string when they go out of scope.

@wolfv
Copy link
Member

wolfv commented Feb 1, 2022

@jonashaag can you have a look to see if this is what you had in mind?

Now when the env var exists, but is empty, it will add an empty string ("").

@jonashaag
Copy link
Contributor Author

@wolfv I was about to push my changes at this very moment :D

@wolfv
Copy link
Member

wolfv commented Feb 1, 2022

Nooooooooo! Sorry :/

@jonashaag
Copy link
Contributor Author

https://github.com/jonashaag/mamba/tree/envsubst2-2

Can you review this instead?

@jonashaag
Copy link
Contributor Author

But I'm happy to see that you made essentially the same changes :) Are you ok with the change in behaviour that empty env vars are now not considered absent anymore? (You also seem to have made that decision in your changes)

@wolfv
Copy link
Member

wolfv commented Feb 1, 2022

I am happy with your version, too. Do you want to force push to this branch to erase my changes and use yours?
I just read them and it's really super similar!

Sorry again, I should've coordinated :)

@jonashaag
Copy link
Contributor Author

No worries. Force pushed.

@jonashaag
Copy link
Contributor Author

@wolfv I don't understand the remaining Windows test failure. It seems that Micromamba thinks that ${foo} is missing (not only empty!) and thus doesn't replace it. But the env::get code should be taking proper care of empty vs missing.

@wolfv
Copy link
Member

wolfv commented Feb 1, 2022

I think
it's a Windows limitation. Setting empty strings to environment variables seems unsupported

So IMHO we can skip that test on Windows.

Screenshot 2022-02-01 at 21 43 12

@jonashaag
Copy link
Contributor Author

Oh wow, well that's unfortunate but then we can simplify some of the code and skip that test.

@@ -14,19 +16,21 @@ def config(*args):
cmd = [umamba, "config"] + [arg for arg in args if arg]
res = subprocess.check_output(cmd)
if "--json" in args:
j = json.loads(res)
j = yaml.load(res, yaml.FullLoader)
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've filed a bug for this already, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@wolfv wolfv merged commit 9e5f8fd into mamba-org:master Feb 2, 2022
@wolfv
Copy link
Member

wolfv commented Feb 2, 2022

Thanks for tackling this!

tsibley added a commit to tsibley/mamba that referenced this pull request Oct 7, 2022
The empty string is found at the start of the string, pos = 0, and
replaced over and over again.  If the replacement string is also empty,
the size of the data string will not change.  If the replacement string
is not empty, it will be repeatedly prepended to the data string.

While most callers are careful to ensure the search string is not empty,
PosixActivator::update_prompt() lost this property in "Micromamba:
Substitute env vars in .condarc (mamba-org#1423)" (9e5f8fd).  Rather than restore
the check in the caller, fix the root of the issue in replace_all()
itself.

I ran into this bug as a Micromamba user in a particular set of
circumstances where the CONDA_PROMPT_MODIFIER env var was the empty
string.¹  It manifested to me the user as a `micromamba create`
invocation hanging at the linking phase for any package with a post-link
script (such as openmpi).  The generated post-link script wrapper
invoked, indirectly, `micromamba shell --shell bash activate <path>`
which was hanging during update_prompt().

¹ nextstrain/cli#223 (comment)
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.

2 participants