-
Notifications
You must be signed in to change notification settings - Fork 328
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
Handle invalid conda-meta/history value #1659
Conversation
R/conda.R
Outdated
exe <- file.path(root, "Scripts", "conda.exe") | ||
if (file.exists(exe)) | ||
conda <- exe |
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.
Not sure if this is still necessary with the above handling for base conda envs
if (dir.exists(file.path(root, "condabin"))) {
# base conda env
conda <- if (is_windows())
file.path(root, "condabin/conda.bat")
else
file.path(root, "bin/conda")
}
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.
Lines 553 to 560 in 14d7327
} else { | |
# on Windows it's preferable to conda.bat located in the 'condabin' | |
# folder. if the user passed the path to a 'Scripts/conda.exe' we will | |
# try to find the 'conda.bat'. | |
altpath <- file.path(dirname(conda), "../condabin/conda.bat") | |
if (file.exists(altpath)) | |
return(normalizePath(altpath, winslash = "/", mustWork = TRUE)) | |
} |
conda.bat
replaces conda.exe
whenever conda_binary(".../conda.exe")
is used too
R/conda.R
Outdated
# that created it | ||
conda <- python_info_condaenv_find(root) | ||
# not base env | ||
conda <- find_conda() |
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.
Thank you for the PR! I noticed something in the implementation that might need clarification.
The purpose of get_python_conda_info()
is to locate the specific conda
used to create a given python
conda environment. Since it's common for users to have multiple versions of conda
installed, this function focuses on identifying the correct one for the environment.
On the other hand, find_conda()
is meant to find the user's preferred conda
executable based on expressed preferences in options()
or other heuristics.
It seems like using find_conda()
within get_python_conda_info()
may not be the right approach here. What do you think?
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 think my specific issue is when it hits
if(!file.exists(conda))
conda <- NA
so I guess in other locations if NA
is passed I can change it to use find_conda
then
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.
Oh I guess conda_binary
already handles that specific case here
Lines 531 to 539 in 4277e2d
conda_binary <- function(conda = "auto") { | |
# automatic lookup if requested | |
if (identical(conda, "auto") || isTRUE(is.na(conda))) { | |
conda <- find_conda() | |
if (is.null(conda)) | |
stop("Unable to find conda binary. Is Anaconda installed?", call. = FALSE) | |
conda <- conda[[1]] | |
} |
The only reason my error was occuring was because get_python_conda_info
used python_info_condaenv_find
directly which didn't handle invalid values from the history
4277e2d
to
d8d4e61
Compare
Hi @t-kalinowski could you take a look at this again? I've changed it so that Lines 1121 to 1123 in 4201011
|
Thank you for the PR! |
Resolves #1654
python_info_condaenv_find
was only being used byget_python_conda_info
andpython_info_condaenv
so I combined it withget_python_conda_info
and just repointedpython_info_condaenv
atget_python_conda_info
so it also gets the fix.