JuliaPy / Conda.jl

Conda managing Julia binary dependencies
Other
172 stars 57 forks source link

Make ROOTENV always include MINICONDA_VERSION #181

Closed omus closed 4 years ago

omus commented 4 years ago

I encountered a scenario in which my Conda deps.jl file contained:

const ROOTENV = "/Users/omus/.julia/conda/3"
const MINICONDA_VERSION = "2"

I expect with this configuration that my ROOTENV included Miniconda 2 packages even though the ROOTENV appears to be for version 3. As this is confusing I updated the package to always create ROOTENV from a combination of the current CONDA_HOME and MINICONDA_VERSION. Doing this should avoid any confusion and additionally allow users to switch between Miniconda versions without having to delete the ROOTENV first.

isuruf commented 4 years ago

Wouldn't this break CONDA_JL_HOME functionality?

omus commented 4 years ago

Wouldn't this break CONDA_JL_HOME functionality?

Current iteration should not break that functionality unless the user specifies their CONDA_JL_HOME to be a directory in~/.julia/conda. If we that is also required to be supported I can also restrict this behaviour to the set of values possible for MINICONDA_VERSION but that will probably be more confusing.

stevengj commented 4 years ago

I would suggest something narrower in scope. Just add something like:

if tryparse(Int,basename(ROOTENV)) !== nothing && normpath(dirname(ROOTENV)) == normpath(condadir) && normpath(ROOTENV) != normpath(joinpath(condadir, MINICONDA_VERSION))
    error("....")
end

after the ROOTENV = ... line.

That is, give an error only if ROOTENV has the canonical form ~/.julia/conda/<number> and the number doesn't match the version. For custom user directories, do nothing.

I don't think we should silently change the path specified by the user. Fail noisily.

omus commented 4 years ago

That is, give an error only if ROOTENV has the canonical form ~/.julia/conda/ and the number doesn't match the version. For custom user directories, do nothing.

The solution you propose is problematic for users that just change CONDA_JL_VERSION:

julia> using Pkg

julia> print(read("deps/deps.jl", String))
const ROOTENV = "/Users/omus/.julia/conda/3"
const MINICONDA_VERSION = "3"

julia> withenv("CONDA_JL_VERSION" => "2") do
           Pkg.build()
       end
  Building Conda → `~/.julia/dev/Conda/deps/build.log`
 Resolving package versions...
┌ Error: Error building `Conda`:
│ ERROR: LoadError: ....
│ Stacktrace:
│  [1] error(::String) at ./error.jl:33
│  [2] top-level scope at /Users/omus/.julia/dev/Conda/deps/build.jl:29
│  [3] include at ./boot.jl:328 [inlined]
│  [4] include_relative(::Module, ::String) at ./loading.jl:1105
│  [5] include(::Module, ::String) at ./Base.jl:31
│  [6] include(::String) at ./client.jl:424
│  [7] top-level scope at none:5
│ in expression starting at /Users/omus/.julia/dev/Conda/deps/build.jl:28
└ @ Pkg.Operations ~/Development/Julia/1.3/usr/share/julia/stdlib/v1.3/Pkg/src/backwards_compatible_isolation.jl:649

I think we require the mutation for users just to be able to only specify a change to CONDA_JL_VERSION

omus commented 4 years ago

Tests are in a good state. Failure remaining appear unrelated to these changes.

omus commented 4 years ago

Feel free to squash and merge