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 conda executable not found (conda 4.4+) #79

Closed
wants to merge 1 commit into from

Conversation

benbovy
Copy link

@benbovy benbovy commented May 21, 2018

Fixes #73

Try first getting path to conda executable from $CONDA_EXE.

xref: jupyterlab/jupyterlab#4598

@coveralls
Copy link

coveralls commented May 21, 2018

Coverage Status

Coverage increased (+0.4%) to 48.364% when pulling de9f8ac on benbovy:conda44-fix into f7b4680 on Anaconda-Platform:master.

@tjd2002
Copy link

tjd2002 commented May 25, 2018

Thanks @benbovy this allows me to use nb_conda under latest anaconda (4.5.3)!

It seems this repo needs a little attention to catch up to the changes in 4.4. Are there any maintainers around? @damianavila or @bollwyvl perhaps ?

@tjd2002
Copy link

tjd2002 commented May 26, 2018

Looks like this will not be compatible with older installs of conda that do not export $CONDA_EXE. Is this a problem? See #80 for a PR that includes a backwards-compatible fix (among other changes)

@mcg1969
Copy link
Collaborator

mcg1969 commented May 26, 2018

To be honest, #80 is getting too unwieldy, trying to do too many things at once. It's fine with me if we try to separate concerns here. I would propose that you bring in my back-compatible logic (or equivalent) into your construction of CONDA_EXE, but that this PR be treated/reviewed/merged separately.

@benbovy
Copy link
Author

benbovy commented May 26, 2018

Looks like this will not be compatible with older installs of conda that do not export $CONDA_EXE. Is this a problem? See #80 for a PR that includes a backwards-compatible fix (among other changes)

In this PR, if $CONDA_EXE doen't exists then it fails back to the older behavior, i.e., trying to get conda from PATH, so I think it is still backward compatible.

@benbovy
Copy link
Author

benbovy commented May 26, 2018

That said, #80 seems to test for a few more things, notably on windows platforms which I don't use, so it's probably better addressed there.

@mcg1969
Copy link
Collaborator

mcg1969 commented May 27, 2018

I agree now that the approach in #80 can at least be simplified. For #82, I've opted to pull this in directly. If #80 needs something more, it will get it ;-)

@tjd2002
Copy link

tjd2002 commented May 27, 2018

As to whether the special-casing in #80 is necessary,

At https://github.com/Anaconda-Platform/anaconda-nb-extensions/issues/168#issuecomment-392280875, Kale Franz wrote:

I would wait to target any change toward conda 4.6+. [...] The big differences here are the existence of a new conda init command to ensure your shell is configured correctly, and then the guaranteed presence of a CONDA_EXE environment variable (and CONDA_BAT as appropriate).

(This: conda/conda#7206 appears to be the PR that makes these changes in 4.6)

So I agree with @mcg1969 that the approach used here looks good.

I think it is preferable to consider this separately from #82, since it is a simple fix for an issue that is happening in the wild right now (for users trying out the new-style install).

@mcg1969
Copy link
Collaborator

mcg1969 commented May 27, 2018

Good point, I would definitely support merging this first.

@mcg1969
Copy link
Collaborator

mcg1969 commented May 28, 2018

Based on discussions with @tjd2002, this is still included in #82, so this can be closed (and #73) if #82 is merged.

@benbovy
Copy link
Author

benbovy commented May 29, 2018

OK, I leave this PR as is for now. Let me know when I can close this (or for maintainers, feel free to close it or merge it depending on what's best to do).

@mcg1969
Copy link
Collaborator

mcg1969 commented May 30, 2018

I'm still waiting on some internal decision making and @damianavila in particular for advice on how to proceed. Stay tuned. Won't be too much longer, I don't think.

@mcg1969
Copy link
Collaborator

mcg1969 commented May 30, 2018

Addressed by #82.

@mcg1969 mcg1969 closed this May 30, 2018
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.

4 participants