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.

Sign up for GitHub

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

Jump to bottom

Introduce CONDA_JL_CONDA_EXE environment variable #216

Merged
merged 12 commits into from
Feb 8, 2022
Merged

Introduce CONDA_JL_CONDA_EXE environment variable #216

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3d42c9f
Introduce CONDA_JL_CONDA_EXE environment variable
mkitti Jan 15, 2022
53ed58c
Error and recommend rebuild if CONDA_EXE does not exist
mkitti Jan 15, 2022
6c244f2
Add Conda.create, use for testing
mkitti Jan 15, 2022
1399e71
Change default CONDA_EXE to match ROOTENV if version mismatch
mkitti Jan 15, 2022
bbb2653
Fix typos in tests
mkitti Jan 15, 2022
2162971
Use local variables instead of let
mkitti Jan 15, 2022
0756e35
The folder is Scripts not Script, fix Windows errors
mkitti Jan 15, 2022
c95e1ab
Check if CONDA_EXE exists CONDA_JL_CONDA_EXE is set during build
mkitti Jan 15, 2022
8eb7c9a
Check if CONDA_JL_CONDA_EXE exists & executable. Install only in ROOTENV
mkitti Jan 15, 2022
1cadc27
Update test/runtests.jl
tkf Jan 16, 2022
2c723b7
Add doc for `CONDA_JL_CONDA_EXE` to README.md
mkitti Jan 16, 2022
84c662c
Use Sys.isexecutable in deps.jl
mkitti Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "Conda"
uuid = "8f4d0f93-b110-5947-807f-2305c1781a2d"
version = "1.6"
version = "1.7"

[deps]
Downloads = "f43a241f-c20a-4ad4-852c-f6b1247861c6"
Expand Down
13 changes: 13 additions & 0 deletions deps/build.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ Using the Anaconda/defaults channel instead, which is free for non-commercial us
if !isdefined(@__MODULE__, :USE_MINIFORGE)
const USE_MINIFORGE = USE_MINIFORGE_DEFAULT
end
if !isdefined(@__MODULE__, :CONDA_EXE)
const CONDA_EXE = if Sys.iswindows()
p = joinpath(ROOTENV, "Script")
conda_bat = joinpath(p, "conda.bat")
isfile(conda_bat) ? conda_bat : joinpath(p, "conda.exe")
else
joinpath(ROOTENV, "bin", "conda")
end

end
end

MINICONDA_VERSION = get(ENV, "CONDA_JL_VERSION", DefaultDeps.MINICONDA_VERSION)
Expand Down Expand Up @@ -52,10 +62,13 @@ These will require rebuilding.
""")
end

CONDA_EXE = get(ENV, "CONDA_JL_CONDA_EXE", DefaultDeps.CONDA_EXE)

deps = """
const ROOTENV = "$(escape_string(ROOTENV))"
const MINICONDA_VERSION = "$(escape_string(MINICONDA_VERSION))"
const USE_MINIFORGE = $USE_MINIFORGE
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""

mkpath(condadir)
Expand Down
30 changes: 19 additions & 11 deletions src/Conda.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,18 @@ function python_dir(env::Environment)
end
const PYTHONDIR = python_dir(ROOTENV)

# note: the same conda program is used for all environments
const conda = if Sys.iswindows()
p = script_dir(ROOTENV)
conda_bat = joinpath(p, "conda.bat")
isfile(conda_bat) ? conda_bat : joinpath(p, "conda.exe")
else
joinpath(bin_dir(ROOTENV), "conda")
if ! @isdefined(CONDA_EXE)
# We have an oudated deps.jl file that does not define CONDA_EXE
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is impossible. If there's an outdated deps.jl that means there was a update of Conda.jl which means the package has to be built first.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible in a few situations such as using multiple Julia installations with the same JULIA_DEPOT_PATH and/or a modified JULIA_LOAD_PATH. Julia will only force a rebuild if it is aware that the version changed.

For example, I did this to myself in the course of testing the package by using pkg"dev Conda". In this case, the Conda package was checked out in ~/.julia/dev/Conda and the deps file at ~/.julia/dev/Conda/deps/deps.jlwas carried over between several versions as I moved between branches or I changed environments.

While we could just throw and error if we detect this condition, encouraging the user to rebuild, just doing the former calculation seems friendlier since we can just follow the old behavior here.

We see this issue with an outdated deps.jl file quite commonly with GR.jl and users will appear in support channels complaining that plotting is broken. The issues were reduced once we just started fixing the issues automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can remove it now, but my recommendation is to keep it for the moment and then remove it at a later time to smooth over the migration process.

const CONDA_EXE = if Sys.iswindows()
p = script_dir(ROOTENV)
conda_bat = joinpath(p, "conda.bat")
isfile(conda_bat) ? conda_bat : joinpath(p, "conda.exe")
else
joinpath(bin_dir(ROOTENV), "conda")
end
end
# note: the same conda program is used for all environments
const conda = CONDA_EXE

"Path to the condarc file"
function conda_rc(env::Environment)
Expand Down Expand Up @@ -191,20 +195,24 @@ _quiet() = get(ENV, "CI", "false") == "true" ? `-q` : ``
"Install miniconda if it hasn't been installed yet; _install_conda(true) installs Conda even if it has already been installed."
function _install_conda(env::Environment, force::Bool=false)
if force || !isfile(Conda.conda)
# This assumes that the conda executable will in some directory under
# CONDA_EXE_PREFIX.
# Ex: If CONDA_EXE="$HOME/miniforge3/bin/conda", then CONDA_EXE_PREFIX="$HOME/miniforge3"
CONDA_EXE_PREFIX = conda |> dirname |> dirname
Copy link
Member

Choose a reason for hiding this comment

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

I think this assumption is too strong. It's possible that people use a shim script or symlink. Can you use something like conda info --json or conda run python -c 'import sys; print(sys.executable)'?

Nitpicks: It's strange that a local variable is all caps. I also notice that src/Conda.jl does not use |> for unary function application. I have nothing against |> per se, but, I think it's good to follow the style of surrounding code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mkitti, why do we need a CONDA_EXE_PREFIX when PREFIX should work just as fine?

Copy link
Member Author

@mkitti mkitti Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

PREFIX is determined by ROOTENV. We cannot use this because we can no longer assume that CONDA_EXE is in ROOTENV.

Conda.jl/src/Conda.jl

Line 47 in f169018

const PREFIX = prefix(ROOTENV)

We reach this line of code when the conda executable does not exist at Conda.conda.
Thus, the objective here is to install the conda executable at the location specified by CONDA_JL_CONDA_EXE / CONDA_EXE / Conda.conda.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean when the user sets CONDA_JL_CONDA_EXE env variable to a non-existent conda executable? In that case, we should just error in deps.jl.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add a check at build time. What should we do at runtime when _install_conda is invoked?

Copy link
Collaborator

@isuruf isuruf Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We can add a check at build time.

Thanks

What should we do at runtime when _install_conda is invoked?

Nothing. We just use PREFIX since the if condition above is invoked only if CONDA_JL_CONDA_EXE is not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it only invoked if CONDA_JL_CONDA_EXE is not set? It might also be invoked if CONDA_EXE gets deleted between build time and run time.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot to mention another point: does Conda.jl still use PREFIX? If not, it's better to remove it or at least deprecate it.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an internal constant called PREFIX, but it does not use an environmental variable called PREFIX.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it only invoked if CONDA_JL_CONDA_EXE is not set? It might also be invoked if CONDA_EXE gets deleted between build time and run time.

True. I'm not sure how to handle it. If CONDA_EXE gets deleted between build time and run time, I'm not sure if we should install it there.

@info("Downloading miniconda installer ...")
if Sys.isunix()
installer = joinpath(PREFIX, "installer.sh")
installer = joinpath(CONDA_EXE_PREFIX, "installer.sh")
end
if Sys.iswindows()
installer = joinpath(PREFIX, "installer.exe")
installer = joinpath(CONDA_EXE_PREFIX, "installer.exe")
end
mkpath(PREFIX)
mkpath(CONDA_EXE_PREFIX)
Downloads.download(_installer_url(), installer)

@info("Installing miniconda ...")
if Sys.isunix()
chmod(installer, 33261) # 33261 corresponds to 755 mode of the 'chmod' program
run(`$installer -b -f -p $PREFIX`)
run(`$installer -b -f -p $CONDA_EXE_PREFIX`)
end
if Sys.iswindows()
run(Cmd(`$installer /S --no-shortcuts /NoRegistry=1 /AddToPath=0 /RegisterPython=0 /D=$PREFIX`, windows_verbatim=true))
Expand Down
106 changes: 71 additions & 35 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,36 +70,40 @@ Conda.rm_channel("foo", env)
Conda.add("zlib", env; channel=alt_channel)

@testset "Batch install and uninstall" begin
Conda.add(["affine", "ansi2html"], env)
installed = Conda._installed_packages(env)
@test "affine" ∈ installed
@test "ansi2html" ∈ installed

Conda.rm(["affine", "ansi2html"], env)
installed = Conda._installed_packages(env)
@test "affine" ∉ installed
@test "ansi2html" ∉ installed
mktempdir() do env
Conda.add(["affine", "ansi2html"], env)
installed = Conda._installed_packages(env)
@test "affine" ∈ installed
@test "ansi2html" ∈ installed

Conda.rm(["affine", "ansi2html"], env)
installed = Conda._installed_packages(env)
@test "affine" ∉ installed
@test "ansi2html" ∉ installed
end
end

# Run conda clean
Conda.clean(; debug=true)

@testset "Exporting and creating environments" begin
new_env = :test_conda_jl_2
Conda.add("curl", env)
Conda.export_list("conda-pkg.txt", env)

# Create a new environment
rm(Conda.prefix(new_env); force=true, recursive=true)
Conda.import_list(
IOBuffer(read("conda-pkg.txt")), new_env; channels=["foo", alt_channel, default_channel]
)

# Ensure that our new environment has our channels and package installed.
@test Conda.channels(new_env) == ["foo", alt_channel, default_channel]
installed = Conda._installed_packages(new_env)
@test "curl" ∈ installed
rm("conda-pkg.txt")
mktempdir() do env
new_env = :test_conda_jl_2
Conda.add("curl", env)
Conda.export_list("conda-pkg.txt", env)

# Create a new environment
rm(Conda.prefix(new_env); force=true, recursive=true)
Conda.import_list(
IOBuffer(read("conda-pkg.txt")), new_env; channels=["foo", alt_channel, default_channel]
)

# Ensure that our new environment has our channels and package installed.
@test Conda.channels(new_env) == ["foo", alt_channel, default_channel]
installed = Conda._installed_packages(new_env)
@test "curl" ∈ installed
rm("conda-pkg.txt")
end
end

@testset "Conda.pip_interop" begin
Expand Down Expand Up @@ -150,6 +154,17 @@ end
end
end

function default_conda_exe(ROOTENV)
@static if Sys.iswindows()
p = joinpath(ROOTENV, "Script")
tkf marked this conversation as resolved.
Show resolved Hide resolved
conda_bat = joinpath(p, "conda.bat")
isfile(conda_bat) ? conda_bat : joinpath(p, "conda.exe")
else
joinpath(ROOTENV, "bin", "conda")
end
end


if Sys.ARCH in [:x86, :i686]
CONDA_JL_USE_MINIFORGE_DEFAULT = "false"
else
Expand All @@ -162,13 +177,16 @@ end
@test !isfile(depsfile)
@test !isfile(joinpath(condadir, "deps.jl"))

withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => nothing) do
withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => nothing, "CONDA_JL_CONDA_EXE" => nothing) do
Pkg.build("Conda")
let ROOTENV=joinpath(condadir, "3"), CONDA_EXE=default_conda_exe(ROOTENV)
@test read(depsfile, String) == """
const ROOTENV = "$(escape_string(joinpath(condadir, "3")))"
const ROOTENV = "$(escape_string(ROOTENV))"
const MINICONDA_VERSION = "3"
const USE_MINIFORGE = $(CONDA_JL_USE_MINIFORGE_DEFAULT)
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""
end
end
end
end
Expand All @@ -179,41 +197,50 @@ end
@test !isfile(depsfile)
@test !isfile(joinpath(condadir, "deps.jl"))

withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => "1") do
withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => "1", "CONDA_JL_CONDA_EXE" => nothing) do
Pkg.build("Conda")
let ROOTENV=joinpath(condadir, "3"), CONDA_EXE=default_conda_exe(ROOTENV)
@test read(depsfile, String) == """
const ROOTENV = "$(escape_string(joinpath(condadir, "3")))"
const MINICONDA_VERSION = "3"
const USE_MINIFORGE = true
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""
end
end
end
preserve_build() do
# In order to test the defaults no depsfiles must be present
@test !isfile(depsfile)
@test !isfile(joinpath(condadir, "deps.jl"))

withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => "0") do
withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => "0", "CONDA_JL_CONDA_EXE" => nothing) do
Pkg.build("Conda")
let ROOTENV=joinpath(condadir, "3"), CONDA_EXE=default_conda_exe(ROOTENV)
@test read(depsfile, String) == """
const ROOTENV = "$(escape_string(joinpath(condadir, "3")))"
const ROOTENV = "$(escape_string(ROOTENV))"
const MINICONDA_VERSION = "3"
const USE_MINIFORGE = false
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""
end
end
end
end

@testset "custom home" begin
preserve_build() do
mktempdir() do dir
withenv("CONDA_JL_VERSION" => "3", "CONDA_JL_HOME" => dir, "CONDA_JL_USE_MINIFORGE" => nothing) do
withenv("CONDA_JL_VERSION" => "3", "CONDA_JL_HOME" => dir, "CONDA_JL_USE_MINIFORGE" => nothing, "CONDA_JL_CONDA_EXE" => nothing) do
Pkg.build("Conda")
let CONDA_EXE=default_conda_exe(ROOTENV)
@test read(depsfile, String) == """
const ROOTENV = "$(escape_string(dir))"
const MINICONDA_VERSION = "3"
const USE_MINIFORGE = $(CONDA_JL_USE_MINIFORGE_DEFAULT)
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""
end
end
end
end
Expand All @@ -222,29 +249,38 @@ end
@testset "version mismatch" begin
preserve_build() do
# Mismatch in written file
let ROOTENV=joinpath(condadir, "3"), CONDA_EXE=default_conda_exe(ROOTENV)
mkitti marked this conversation as resolved.
Show resolved Hide resolved
write(depsfile, """
const ROOTENV = "$(escape_string(joinpath(condadir, "3")))"
const ROOTENV = "$(escape_string(ROOTENV)))"
const MINICONDA_VERSION = "2"
const USE_MINIFORGE = $(CONDA_JL_USE_MINIFORGE_DEFAULT)
const CONDA_EXE = "$(escape_string(CONDA_EXE)))"
""")
end

withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => nothing) do
withenv("CONDA_JL_VERSION" => nothing, "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => nothing, "CONDA_JL_CONDA_EXE" => nothing) do
Pkg.build("Conda")
let ROOTENV=joinpath(condadir, "2"), CONDA_EXE=default_conda_exe(ROOTENV)
@test read(depsfile, String) == """
const ROOTENV = "$(escape_string(joinpath(condadir, "2")))"
const ROOTENV = "$(escape_string(ROOTENV))"
const MINICONDA_VERSION = "2"
const USE_MINIFORGE = $(CONDA_JL_USE_MINIFORGE_DEFAULT)
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""
end
end

# ROOTENV should be replaced since CONDA_JL_HOME wasn't explicitly set
withenv("CONDA_JL_VERSION" => "3", "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => nothing) do
withenv("CONDA_JL_VERSION" => "3", "CONDA_JL_HOME" => nothing, "CONDA_JL_USE_MINIFORGE" => nothing, "CONDA_JL_CONDA_EXE" => nothing) do
Pkg.build("Conda")
let ROOTENV=joinpath(condadir, "3"), CONDA_EXE=default_conda_exe(ROOTENV)
@test read(depsfile, String) == """
const ROOTENV = "$(escape_string(joinpath(condadir, "3")))"
const ROOTENV = "$(escape_string(ROOTENV)))"
const MINICONDA_VERSION = "3"
const USE_MINIFORGE = $(CONDA_JL_USE_MINIFORGE_DEFAULT)
const CONDA_EXE = "$(escape_string(CONDA_EXE))"
"""
end
end
end
end
Expand Down