Skip to content

Conversation

@ericsnowcurrently
Copy link

(fixes #1882)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Unit tests & system/integration tests are added/updated
  • Any new/changed dependencies in package.json are pinned (e.g. "1.2.3", not "^1.2.3" for the specified version)
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)

];
}

private async getPowershellCommands(

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?

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.

Copy link
Author

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.

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..

Copy link
Author

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.

Copy link
Author

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()}`

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.

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)

Copy link
Author

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.

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.

Copy link
Author

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.

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.

@ericsnowcurrently ericsnowcurrently merged commit 9c04f3b into microsoft:master Oct 2, 2018
@ericsnowcurrently ericsnowcurrently deleted the fix-1882-conda-activation branch October 2, 2018 17:24
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Activating a conda environment using conda instead of source

2 participants