Ensure Windows Store takes precedence over Registry and fix Conda locators #23463
Conversation
DonJayamanne
added
no-changelog
DonJayamanne
force-pushed
the
addLoggingForWindowsStoreLC
branch
from
9822bfc to
84c1d17
Compare
DonJayamanne
changed the title
| python_run_command: Some(vec![env.executable.to_str().unwrap().to_string()]), | ||
| arch: None, | ||
| ..Default::default() |
There was a problem hiding this comment.
Had to do this in this PR, adding new fields was getting way too messy
| serde_json::from_str::<CondaMetaPackageStructure>(&contents).ok() | ||
| { | ||
| if let Some(channel) = js.channel { | ||
| if channel.ends_with("64") { |
There was a problem hiding this comment.
Detection of 64bit for conda envs
| @@ -202,6 +234,8 @@ fn get_conda_manager(path: &PathBuf) -> Option<EnvManager> { | |||
| executable_path: conda_exe, | |||
| version: Some(conda_pkg.version), | |||
| tool: EnvManagerType::Conda, | |||
| company: None, | |||
| company_display_name: None, | |||
There was a problem hiding this comment.
company names we get from registry
| // will be activated in python extension using first conda executable and -n base, | ||
| // I.e. base env of the first install will be activated instead of this. | ||
| // Hence lets always just give the path. | ||
| // name: Some("base".to_string()), |
There was a problem hiding this comment.
This is due to the fact that python extension supports only one conda exe today,
| if let Ok(metadata) = exe.metadata() { | ||
| if let Ok(ctime) = metadata.created() { | ||
| if let Ok(ctime) = ctime.duration_since(UNIX_EPOCH) { | ||
| env.creation_time = Some(ctime.as_millis()); |
There was a problem hiding this comment.
Adding this, else I found we get some weird behaviour
| let mut result = get_registry_pythons("PythonCore").unwrap_or_default(); | ||
| environments.append(&mut result); | ||
|
|
||
| let mut result = get_registry_pythons_anaconda(self.conda_locator) ; |
There was a problem hiding this comment.
No more hardcoding of PythonCore and CondaAnalytics,
the code is now generic and will also detect conda envs
| for file in std::fs::read_dir(apps_path).ok()?.filter_map(Result::ok) { | ||
| let path = file.path(); | ||
| trace!("Searching for Windows Store Python in {:?}", apps_path); | ||
| let folder_version_regex = |
There was a problem hiding this comment.
Fixes as per discussion, to search for python3.10.exe and the PythonSoftwareFoundation.Python3.10_.... folders instead of using file and dir exists.
| * @param env The environment to get the interpreter information for. | ||
| * @param priority The priority of the request. | ||
| */ | ||
| getMandatoryEnvironmentInfo( |
There was a problem hiding this comment.
This was required, else we end up resolving Python environments even when we have almost all of the info.
E.g. for windows store we know the version is 3.10.X, the typescript code extracts this today.
However we fully resolve this unnecessarily,
Now, added a different method to ensure we only resolve what we need to.
The old resolve will do a full resolve.
This now makes the discovery even faster, as we do not spawn envs at all, not unless we need to
| if (isEnvLackingPython) { | ||
| // TODO: Shouldn't this only apply to conda, how else can we have an environment and not have Python in it? | ||
| // If thats the case, then this should be gated on environment.kind === PythonEnvKind.Conda | ||
| if (isEnvLackingPython && environment.kind !== PythonEnvKind.MicrosoftStore) { |
There was a problem hiding this comment.
This looks wrong, but I do not want to fix this just yet.
E.g. I found that for MS store the exe is not in the same location as the sysPrefix, as a result we end up spawning the python process no matter what.
Thats a bug in the code, as in MS Store the exe is in a different location.
Also to my knowledge, this code should only apply to conda.
| } else { | ||
| const { ctime, mtime } = await getFileInfo(resolvedEnv.executable.filename); | ||
| resolvedEnv.executable.ctime = ctime; | ||
| resolvedEnv.executable.mtime = mtime; |
There was a problem hiding this comment.
More fixes to ensure we avoid more unnecessary I/O operations in node
No description provided.