-
Notifications
You must be signed in to change notification settings - Fork 57
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
Set PATH before running conda #121
Conversation
8614a51
to
57ca293
Compare
src/Conda.jl
Outdated
@@ -103,6 +103,8 @@ function _set_conda_env(cmd, env::Environment=ROOTENV) | |||
env_var["PYTHONIOENCODING"]="UTF-8" | |||
env_var["CONDARC"] = conda_rc(env) | |||
env_var["CONDA_PREFIX"] = prefix(env) | |||
path_sep = Compat.Sys.iswindows() ? ';' : ':' | |||
env_var["PATH"] = bin_dir(env) * path_sep * ENV["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.
get(ENV, "PATH", "")
test/runtests.jl
Outdated
|
||
cmd = `python -c "import sys; print(sys.executable)"` | ||
path = rstrip(read(_set_conda_env(cmd, env), String)) | ||
@test startswith(normpath(path), normpath(bin_dir(env))) |
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.
startwith
or normpath(dirname(path)) == normpath(bin_dir(env))
?
test/runtests.jl
Outdated
@test isfile(pythonpath) | ||
|
||
cmd = Conda._set_conda_env(`$pythonpath -c "import zmq"`, env) | ||
@test_throws Exception run(cmd) |
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.
This won't work if pyzmq
is already installed — people don't necessarily run test Conda
on a clean installation.
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.
In particular, what happens if you run the tests twice in a row? Won't pyzmq
still be installed from the previous run?
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.
Oops, @test !success(cmd)
was just a leftover from the previous version.
To make ]test Conda
easier, how about doing
try
Conda.rm("--all", env)
catch
end
(or with something similar with cleaner handling of exception)?
I'm still unclear why it is necessary to set the |
I don't know, but from JuliaLang/IJulia.jl#739 (comment) and #119 (comment) (and that |
BTW, when I run
(the output is weird since I'm on linux) So it seems |
src/Conda.jl
Outdated
@@ -103,6 +103,12 @@ function _set_conda_env(cmd, env::Environment=ROOTENV) | |||
env_var["PYTHONIOENCODING"]="UTF-8" | |||
env_var["CONDARC"] = conda_rc(env) | |||
env_var["CONDA_PREFIX"] = prefix(env) | |||
path_sep = Compat.Sys.iswindows() ? ';' : ':' | |||
if haskey(ENV, "PATH") | |||
env_var["PATH"] = bin_dir(env) * path_sep * ENV["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.
#121 (comment): Good catch, but get(ENV, "PATH", "")
could put $PWD
into $PATH
?
So it turned out Windows searches |
But only (random?) half of the tests are fixed by this https://ci.appveyor.com/project/StevenGJohnson/conda-jl/build/1.0.121 So strange... |
Reading ContinuumIO/anaconda-issues#10082, it looks like it would be fixed on conda side. Maybe it's better to do |
I'm worried that this will also screw up PyCall… importing Python modules won't work if the Python modules depend on the |
Yeah, Conda.jl has to mutate But that's ugly so |
This reverts a subset of the patch in 7094e2a
|
||
Extra `ENV["PATH"]` required for conda environment `env`. | ||
""" | ||
function extra_path(env::Environment) |
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.
Somebody has to write a code to pass this to SetDllDirectory or AddDllDirectory in PyCall.__init__
: #125 (comment)
|
||
function _set_path(env_var, env::Environment) | ||
if Compat.Sys.iswindows() | ||
# Environment variables are case-insensitive. `Base.EnvDict` |
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.
Can you file a Julia issue for this? Seems like it should be fixed in Base.
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.
It's just because
julia> typeof(copy(ENV))
Dict{String,String}
To solve this, I think you need to add a case-insensitive in-memory dict to Base
.
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.
Seems like the easiest fix would be to replace copy(ENV)
with
env_var = Dict(uppercase(k)=>v for (k,v) in ENV)
(Otherwise the other code in _set_conda_env
would have to be fixed too.)
if Compat.Sys.iswindows() | ||
return join([ | ||
prefix(env), | ||
joinpath(prefix(env), "Library", "mingw-w64", "bin"), |
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 if it is 32-bit 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.
I don't know. I don't notice any branching in conda/activate.py
https://github.com/conda/conda/blob/4.6.0b0/conda/activate.py (But I'm not a Python interpreter)
If the folder does not exist, wouldn't it just be ignored? Also, it looks like win-32 and win-64 are different packages for binary libraries: https://anaconda.org/anaconda/pyzmq/files So I guess it wouldn't be installed in the first place?
# * https://github.com/ContinuumIO/anaconda-issues/issues/10082 | ||
# * https://github.com/conda/conda/issues/7789 | ||
res = "https://repo.continuum.io/miniconda/Miniconda$(MINICONDA_VERSION)-4.5.4-" | ||
end |
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.
Seems like this PR could be done before #125.
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.
Well..., the id of this PR is 121 (< #124)
Joking aside, this PR needs a coordination with PyCall to use SetDllDirectory/AddDllDirectory and for IJulia to tweak setup ENV["PATH"]
before launching Jupyter Notebook. So I still think it makes sense to release a quick fix with #124 first to help Windows users.
I don't know much about Windows so please feel free to close this PR and implement another solution. I re-opened this PR because I though I'd better cleanup my suboptimal solution.
Sorry I haven't looked at this for a while. CI is failing? |
I forgot the status of this PR :) But I remember thinking that new conda version may provide a better way to do this (via |
It looks like #170 supersedes this PR. Closing... |
close #125