Introduce CONDA_JL_CONDA_EXE environment variable #216

mkitti · 2022-01-15T14:24:23Z

Here we introduce a new environment variable, CONDA_JL_CONDA_EXE which determines the location of the conda executable at build time. This in turn is saved in deps.jl as the constant CONDA_EXE. This in turn defines the constant Conda.conda. If CONDA_EXE does not exist due to an old deps.jl file, Conda.conda is defined in manner prior to this PR.

The default CONDA_EXE is defined by the following code based on ROOTENV.

        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

Conda.jl also has a method _install_conda which installs conda if the executable is not present. To determine the PREFIX for this installation the following code is used: CONDA_EXE_PREFIX = conda |> dirname |> dirname.

isuruf · 2022-01-15T15:18:10Z

src/Conda.jl

-else
-    joinpath(bin_dir(ROOTENV), "conda")
+if ! @isdefined(CONDA_EXE)
+    # We have an oudated deps.jl file that does not define CONDA_EXE


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.

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.

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.

mkitti · 2022-01-15T15:40:31Z

Could we run the CI to see if there are any current issues?

test/runtests.jl

codecov-commenter · 2022-01-15T22:08:11Z

Codecov Report

Merging #216 (84c662c) into master (f169018) will decrease coverage by 2.38%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   91.83%   89.44%   -2.39%     
==========================================
  Files           1        1              
  Lines         196      199       +3     
==========================================
- Hits          180      178       -2     
- Misses         16       21       +5

Impacted Files	Coverage Δ
src/Conda.jl	`89.44% <100.00%> (-2.39%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f169018...84c662c. Read the comment docs.

tkf · 2022-01-15T22:21:36Z

src/Conda.jl

+        # 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


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.

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

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.

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.

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

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.

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.

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.

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

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.

test/runtests.jl

tkf

LGTM! I think it's ready to merge after CONDA_JL_CONDA_EXE is documented in README and @stevengj looked at it.

mkitti · 2022-01-16T01:22:09Z

Great. Thanks for the help. It looks like this can also be used to use mamba in place of conda.

$ CONDA_JL_CONDA_EXE=~/mambaforge/bin/mamba julia

(@v1.7) pkg> build Conda
    Building Conda → `~/.julia/dev/Conda/deps/build.log`
Precompiling project...
  1 dependency successfully precompiled in 1 seconds (16 already precompiled)

julia> Conda.conda
"/home/mkitti/mambaforge/bin/mamba"

julia> Conda.list()
[ Info: Running `conda list` in root environment
# packages in environment at /home/mkitti/.julia/conda/3:

mkitti · 2022-01-21T18:31:00Z

@stevengj , would you like to review before we merge this?

mkitti · 2022-01-24T07:27:41Z

Would anyone object to merging by the end of this week if we do not hearing anything else?

stevengj · 2022-01-24T13:00:18Z

deps/build.jl

+if haskey(ENV, "CONDA_JL_CONDA_EXE")
+    # Check to see if CONDA_EXE is an executable file
+    if isfile(CONDA_EXE)
+        if uperm(CONDA_EXE) & 0x01 > 0


Sys.isexecutable(CONDA_EXE)?

Thanks. I made this change in 84c662c

mkitti · 2022-02-07T06:11:17Z

Thank you for the reviews. Can we merge this?

mkitti · 2022-02-08T04:23:51Z

Thanks again for studying this pull request. I would like to merge this tomorrow if I there are no further concerns.

Introduce CONDA_JL_CONDA_EXE environment variable

3d42c9f

mkitti mentioned this pull request Jan 15, 2022

condabin instead of bin #214

Closed

isuruf reviewed Jan 15, 2022

View reviewed changes

mkitti added 2 commits January 15, 2022 12:14

Error and recommend rebuild if CONDA_EXE does not exist

53ed58c

Add Conda.create, use for testing

6c244f2

mkitti mentioned this pull request Jan 15, 2022

Conda-Forge Julia interop JuliaPy/CondaPkg.jl#19

Closed

mkitti added 2 commits January 15, 2022 14:43

Change default CONDA_EXE to match ROOTENV if version mismatch

1399e71

Fix typos in tests

bbb2653

tkf reviewed Jan 15, 2022

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

tkf reviewed Jan 15, 2022

View reviewed changes

mkitti added 4 commits January 15, 2022 18:12

Use local variables instead of let

2162971

The folder is Scripts not Script, fix Windows errors

0756e35

Check if CONDA_EXE exists CONDA_JL_CONDA_EXE is set during build

c95e1ab

Check if CONDA_JL_CONDA_EXE exists & executable. Install only in ROOTENV

8eb7c9a

tkf reviewed Jan 16, 2022

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

Update test/runtests.jl

1cadc27

tkf reviewed Jan 16, 2022

View reviewed changes

Add doc for CONDA_JL_CONDA_EXE to README.md

2c723b7

tkf approved these changes Jan 16, 2022

View reviewed changes

This was referenced Jan 16, 2022

integration with Conda.jl? conda-forge/julia-feedstock#161

Open

Bump to version 1328, remove Vararg testing patch, degit patches, Conda.jl conda-forge/julia-feedstock#185

Merged

Adding PyJulia and PySR conda-forge/staged-recipes#17605

Merged

stevengj reviewed Jan 24, 2022

View reviewed changes

Use Sys.isexecutable in deps.jl

84c662c

isuruf merged commit 8f71332 into JuliaPy:master Feb 8, 2022

mkitti mentioned this pull request Feb 13, 2022

Empty build update for testing conda-forge/pyjulia-feedstock#1

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CONDA_JL_CONDA_EXE environment variable #216

Introduce CONDA_JL_CONDA_EXE environment variable #216

mkitti commented Jan 15, 2022 •

edited

Loading

isuruf Jan 15, 2022

mkitti Jan 15, 2022

mkitti Jan 15, 2022

mkitti commented Jan 15, 2022

codecov-commenter commented Jan 15, 2022 •

edited

Loading

tkf Jan 15, 2022

isuruf Jan 15, 2022

mkitti Jan 15, 2022 •

edited

Loading

isuruf Jan 15, 2022

mkitti Jan 15, 2022

isuruf Jan 15, 2022 •

edited

Loading

mkitti Jan 15, 2022

tkf Jan 15, 2022

mkitti Jan 15, 2022

isuruf Jan 15, 2022

tkf left a comment

mkitti commented Jan 16, 2022

mkitti commented Jan 21, 2022

mkitti commented Jan 24, 2022

stevengj Jan 24, 2022

mkitti Jan 25, 2022

mkitti commented Feb 7, 2022

mkitti commented Feb 8, 2022

Introduce CONDA_JL_CONDA_EXE environment variable #216

Introduce CONDA_JL_CONDA_EXE environment variable #216

Conversation

mkitti commented Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Jan 15, 2022

codecov-commenter commented Jan 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isuruf Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf left a comment

Choose a reason for hiding this comment

mkitti commented Jan 16, 2022

mkitti commented Jan 21, 2022

mkitti commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Feb 7, 2022

mkitti commented Feb 8, 2022

mkitti commented Jan 15, 2022 •

edited

Loading

codecov-commenter commented Jan 15, 2022 •

edited

Loading

mkitti Jan 15, 2022 •

edited

Loading

isuruf Jan 15, 2022 •

edited

Loading