-
Notifications
You must be signed in to change notification settings - Fork 1.3k
#1882: Use "conda activate" after 4.4.0. #2717
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
#1882: Use "conda activate" after 4.4.0. #2717
Conversation
| ]; | ||
| } | ||
|
|
||
| private async getPowershellCommands( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about conda version > 4.4.0 on Powershell or Command Prompt?
Don't we need to make any changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Else, please create a separate issue for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware of any changes relative to powershell. I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to consider all shells.
We have a case statement in the class, and we need to ensure all are satisfied.
I.e. works on:
- Windows + Linux + Mac
- Powershell, Powershell core, bash, tcsh, zsh, fsh, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, activation on powershell has not changed. When I get a chance, I'll verify on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything worked fine when activating a selected env on 4.4.0+.
| // the conda instructions such that "conda" is on their $PATH. | ||
| return [ | ||
| `& cmd /k "activate ${envInfo.name.toCommandArgument().replace(/"/g, '""')} & ${powershellExe}"` | ||
| `${conda.fileToCommandArgument()} activate ${envName.toCommandArgument()}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure you have tested this PR.
This is not the solution what was discussed, this is what failed for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution that was discussed was (documented it in the issue): #1882 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, and @brettcannon expressed reservations about working around the "init" step under 4.4.0+. Let's get together today and iron this out.
| // we accommodate the two ways distinctly. | ||
| if (version === '4.4.0' || compareVersion(version, '4.4.0') > 0) { | ||
| // Note that this requires the user to have already followed | ||
| // the conda instructions such that "conda" is on their $PATH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the conda instructions such that "conda" is on their $PATH.
This is what we would like to avoid. In fact conda will NOT be in their PATH.
Thats why we introduced a setting for condaPath in settings.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked with @brettcannon about this. He does not want the extension to initialize conda (under 4.4.0+) if the user has not followed conda's instructions; the error message given by the conda command (if it is absolute or otherwise on $PATH) will explain what the user needs to do.
Do you mean we should help users if conda is installed but they did not follow conda's instructions to put it on their $PATH? In that case they will get a relatively unhelpful error message: conda: command not found. FWIW, the same goes for pre-4.4.0 (i.e. source activate): -bash: activate: No such file or directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the same goes for pre-4.4.0
Pre-4.4.0, conda (root env) was in the current path, hence this worked.
(fixes #1882)
Any new/changed dependencies inpackage.jsonare pinned (e.g."1.2.3", not"^1.2.3"for the specified version)package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)