Skip to content

Ensure pyenv virtual envs are not skipped when in discovery experiment #15451

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.

Sign up for GitHub

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

Merged
karthiknadig merged 4 commits into from
Feb 19, 2021
Merged

Ensure pyenv virtual envs are not skipped when in discovery experiment #15451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
484c8e4
Ensure pyenv virtual envs are not skipped
karthiknadig Feb 18, 2021
6fdfb55
Add news
karthiknadig Feb 18, 2021
63d3de5
Clean up
karthiknadig Feb 18, 2021
69c71e8
Address comments
karthiknadig Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/client/pythonEnvironments/common/externalDependencies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,23 +97,26 @@ export async function getFileInfo(filePath: string): Promise<{ ctime: number; mt
}
}

export async function resolveSymbolicLink(filepath: string): Promise<string> {
const stats = await fsapi.lstat(filepath);
export async function resolveSymbolicLink(absPath: string): Promise<string> {
const stats = await fsapi.lstat(absPath);
if (stats.isSymbolicLink()) {
const link = await fsapi.readlink(filepath);
const link = await fsapi.readlink(absPath);
return resolveSymbolicLink(link);
}
return filepath;
return absPath;
}

export async function* getSubDirs(root: string): AsyncIterableIterator<string> {
export async function* getSubDirs(root: string, resolveSymlinks: boolean): AsyncIterableIterator<string> {
const dirContents = await fsapi.readdir(root);
const generators = dirContents.map((item) => {
async function* generator() {
const stat = await fsapi.lstat(path.join(root, item));
const fullPath = path.join(root, item);
const stat = await fsapi.lstat(fullPath);

if (stat.isDirectory()) {
yield item;
yield fullPath;
} else if (resolveSymlinks && stat.isSymbolicLink()) {
Copy link
Copy Markdown

@karrtikr karrtikr Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about symlinks which are not directories? It seems we'll be returning those as well :/

Copy link
Copy Markdown
Member Author

@karthiknadig karthiknadig Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this problem only started appearing since the last release? Is this only when in experiment? I can't seem to confirm that from the comments.

This is only in the experiment. I don't think it is affecting the old code. There is a different problem with the old code.

Is this only a special case with Pyenv, or should we fix this when listing subdirectories in other locators as well?

As far as I can tell this is a special case with Pyenvs. pyenv seems to optimize things this way.

I don't recall the old locators having a problem with listing such interpreters mentioned in the issue. We used the VSCode FS API in the old locators, vs.workspace.fs.readDirectory. Maybe that doesn't have this problem.

That is right vs.workspace.fs.readDirectory reads symlinks and resolves them for directories.

What about symlinks which are not directories?

Good catch. I will update the conde to handle that case.

Copy link
Copy Markdown

@karrtikr karrtikr Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha.

As far as I can tell this is a special case with Pyenvs. pyenv seems to optimize things this way.

We know vs.workspace.fs.readDirectory which reads symlinks and resolves them for directories works well for old virtual env locators that we had.
So I think we should also pass resolveSymlinks as true for other virtual env locators as well to be safe.

Copy link
Copy Markdown

@karrtikr karrtikr Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created a work item corresponding to the precautionary comment I made above.

yield await resolveSymbolicLink(fullPath);
}
}

Expand Down
14 changes: 7 additions & 7 deletions src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,15 +259,15 @@ export function parsePyenvVersion(str: string): Promise<IPyenvVersionStrings | u
async function* getPyenvEnvironments(): AsyncIterableIterator<PythonEnvInfo> {
const pyenvVersionDir = getPyenvVersionsDir();

const subDirs = getSubDirs(pyenvVersionDir);
const subDirs = getSubDirs(pyenvVersionDir, true);
for await (const subDir of subDirs) {
const envDir = path.join(pyenvVersionDir, subDir);
const interpreterPath = await getInterpreterPathFromDir(envDir);
const envDirName = path.basename(subDir);
const interpreterPath = await getInterpreterPathFromDir(subDir);

if (interpreterPath) {
// The sub-directory name sometimes can contain distro and python versions.
// here we attempt to extract the texts out of the name.
const versionStrings = await parsePyenvVersion(subDir);
const versionStrings = await parsePyenvVersion(envDirName);

// Here we look for near by files, or config files to see if we can get python version info
// without running python itself.
Expand All @@ -290,7 +290,7 @@ async function* getPyenvEnvironments(): AsyncIterableIterator<PythonEnvInfo> {
// `pyenv local|global <env-name>` or `pyenv shell <env-name>`
//
// For the display name we are going to treat these as `pyenv` environments.
const display = `${subDir}:pyenv`;
const display = `${envDirName}:pyenv`;

const org = versionStrings && versionStrings.distro ? versionStrings.distro : '';

Expand All @@ -299,14 +299,14 @@ async function* getPyenvEnvironments(): AsyncIterableIterator<PythonEnvInfo> {
const envInfo = buildEnvInfo({
kind: PythonEnvKind.Pyenv,
executable: interpreterPath,
location: envDir,
location: subDir,
version: pythonVersion,
source: [PythonEnvSource.Pyenv],
display,
org,
fileInfo,
});
envInfo.name = subDir;
envInfo.name = envDirName;

yield envInfo;
}
Expand Down