hafen / rminiconda

Install and maintain isolated miniconda Python installations from R
Apache License 2.0
67 stars 4 forks source link

get_miniconda_path() should respect R_MINICONDA_PATH #6

Closed espinielli closed 5 years ago

espinielli commented 5 years ago

At work, IT allows us to install under C:\Users\<userid>\dev, so I installed miniconda via this package as follows:

rminiconda::install_miniconda(name = "pymodes", path = "C:/Users/cucu/dev/")

The above installs miniconda in the correct place (with complains):

> rminiconda::install_miniconda(name = "pymodes", path = "C:/Users/cucu/dev/")
Using path for conda installation:
  C:\Users\cucu\dev\pymodes
Downloading miniconda installer...
Source: https://repo.anaconda.com/miniconda/Miniconda3-latest-Windows-x86_64.exe
Destination: C:\Users\cucu\dev\pymodes
trying URL 'https://repo.anaconda.com/miniconda/Miniconda3-latest-Windows-x86_64.exe'
Content type 'application/octet-stream' length 53013808 bytes (50.6 MB)
downloaded 50.6 MB

By installing, you accept the Conda license:
  https://conda.io/en/latest/license.html
Installing isolated miniconda distribution...
Error: Installation was not successful.
In addition: Warning message:
In normalizePath(path.expand(path), winslash, mustWork) :
  path[1]="C:\Users\cucu\AppData\Roaming/rminiconda/pymodes/python.exe": The system cannot find the path specified

in fact after having set R_MINICONDA_PATH to point to C:/Users/cucu/dev the following is ok:

rminiconda::find_miniconda_python(
  "pymodes",
  path = Sys.getenv("R_MINICONDA_PATH"))

The problems come from all other functions in the package that take implicitly path = get_miniconda_path(), which is not looking into R_MINICONDA_PATH but rather in the APPDATA (for MS Windows) directory.

I think get_miniconda_path should check first in R_MINICONDA_PATH and then failing that in the (platform specific) default location.

espinielli commented 5 years ago

Something along the lines

get_miniconda_path <- function() {
  path <- Sys.getenv("R_MINICONDA_PATH")
  if (path != "") {
    path <- file.path(path)
    if (dir.exists(path)) return(path)
  }
  # continue with platform specific logic
    if (is_windows()) {
      ...
    }
    ...
}
...

[On a minor note IMHO get_miniconda_path should not create a "missing" directory ]

espinielli commented 5 years ago

Also in install_miniconda the call to test_miniconda(name) should use the path provided as argument too, so to make it work in case of custom installation (and without an R_MINICONDA_PATH being set up yet)

espinielli commented 5 years ago

I will try tp push a PR if there is interest. HTH and thanks for the package...already useful as is BTW!

hafen commented 5 years ago

PR would be great! I thought it was already handling the R_MINICONDA_PATH - I missed that.