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

fix: Adapt shell init for mamba 2 #914

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jjerphan
Copy link

@jjerphan jjerphan commented Dec 16, 2024

Description

Fix conda-forge/miniforge#697.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

I am interested to have some guidance on the best way to test it.

Signed-off-by: Julien Jerphanion <[email protected]>
@conda-bot
Copy link
Contributor

We require contributors to sign our Contributor License Agreement and we don't have one on file for @jjerphan.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#1078), and ping the bot to refresh the PR.

@travishathaway
Copy link
Contributor

@conda-bot check

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Dec 16, 2024
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan marked this pull request as ready for review December 17, 2024 10:11
@jjerphan jjerphan requested a review from a team as a code owner December 17, 2024 10:11
@@ -671,11 +671,22 @@ if [ "$BATCH" = "0" ]; then
*) "$PREFIX/bin/python" -m conda init ;;
esac
if [ -f "$PREFIX/bin/mamba" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this code now, I think it might result into double initialization, right? Once with conda, once with mamba. Are those compatible or should they be exclusive?

Copy link
Author

Choose a reason for hiding this comment

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

I think conda's initialization is overridden by mamba's; so it works, but it might not be totally sane. I guess we can keep the same behavior when 1.x is used unchanged, and see potential adaptations for 2.x.

What do you think? Is there a way to test that this works with both versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to add cases for it in the examples/ directory. Maybe you can repurpose the Miniforge example (which might need to get synced up with current Miniforge scripts anyway), and then parametrize with some environment variables as we are doing in some other tests. Check for os.environ in the construct.yaml templates and also monkeypatch.setenv calls in tests/test_examples.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🆕 New
4 participants