Skip to content
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

Project root is calculated incorrectly if workspace contains a folder outside of the root that has a shorter name #579

Open
stan-stately opened this issue Aug 9, 2024 · 12 comments
Labels
needs-decision Undecided if this should be done

Comments

@stan-stately
Copy link

I've just hit a bug where the working directory that ruff is using to run the ruff server cmd is outside of my root workspace.

I checked out the repo and reproduced locally with the debugger. The issue is that my workspace looks like this:

{
  "folders": [
    {
      "name": "root",
      "path": ".",
    },
    {
      "name": "other",
      "name": "../other",
    }
]

Its in this block here:

let rootWorkspace = workspaces[0];
let root = undefined;
for (const w of workspaces) {
if (await fs.pathExists(w.uri.fsPath)) {
root = w.uri.fsPath;
rootWorkspace = w;
break;
}
}
for (const w of workspaces) {
if (root && root.length > w.uri.fsPath.length && (await fs.pathExists(w.uri.fsPath))) {
root = w.uri.fsPath;
rootWorkspace = w;
}
}

You assume that the shortest pathname must be used, but in my case my main workspace is ~/workspace and my secondary workspace is at ~/other so getProjectRoot() incorrectly selects ~/other because it is shorter.

I'm not sure how you can easily solve this because I don't see a clear way to get the true workspace root from the vscode API - i guess thats why you had to do this path thing in the first place

@dhruvmanila
Copy link
Member

For context, this was mainly adopted from the extension template: https://github.com/microsoft/vscode-python-tools-extension-template/blob/2017064cd981efde0239e3f6949295b8cde05bee/src/common/utilities.ts#L38-L67.

I think this a multi-root workspace for which there doesn't exists a single common root between the multiple workspaces. I guess the project root determined here is at a best-effort basis. Let me think about this more. I wonder if the "root" name in the .code-workspace is done by VS Code and if we could somehow extract that information instead.

@dhruvmanila dhruvmanila added the needs-decision Undecided if this should be done label Aug 9, 2024
@stan-stately
Copy link
Author

I think this a multi-root workspace for which there doesn't exists a single common root between the multiple workspaces.

yes thats correct. sorry the terminology is kinda confusing to me.

I wonder if the "root" name in the .code-workspace is done by VS Code and if we could somehow extract that information instead.

ideally you would be able get get the location of the .code-workspace file because that is the true root but I didn't see a way to access this.

@dhruvmanila
Copy link
Member

ideally you would be able get get the location of the .code-workspace file because that is the true root but I didn't see a way to access this.

Is there a reference to "true root" in the VS Code documentation for this? I don't see anything like that mentioned in https://code.visualstudio.com/docs/editor/multi-root-workspaces or https://github.com/microsoft/vscode/wiki/Adopting-Multi-Root-Workspace-APIs.

@jamesharris-garmin
Copy link

jamesharris-garmin commented Aug 9, 2024

So if you are in a multi-root workspace, shouldn't the ruff lsp adopt the settings relevant to the workspace the currently open file is handling? This is especially true from a formatting perspective. where my format shortcut in vscode gets the wrong values because it isn't detecting the nearest ruff.toml for the project the file is working in.

Nevermind. Turns out I wasn't on the latest version of the ruff extension.

@stan-stately
Copy link
Author

Is there a reference to "true root" in the VS Code documentation for this?

i couldn't find one, no.

@dhruvmanila
Copy link
Member

Is this causing any unexpected behavior from Ruff? If so, can you specify what that is and what's the expected behavior?

@stan-stately
Copy link
Author

i install ruff with mise. my .mise.toml is beside my .code-workspace. because the cwd is a parent of .mise.toml, mise is unable to resolve the correct ruff.

its not a huge problem for me, i just swapped to the ruff thats bundled with the extension.

you can close this if you want. i agree that its a really weird edge case and not worth too much time to look into, but it is a problem to not be able to guarantee the cwd that ruff is called from. there is probably other ways to solve this like allowing an explicit override of the working dir

@dhruvmanila
Copy link
Member

i install ruff with mise. my .mise.toml is beside my .code-workspace. because the cwd is a parent of .mise.toml, mise is unable to resolve the correct ruff.

So, are you saying that the problem here is that the Ruff VS Code extension is not picking up the ruff executable from wherever "mise" is installing? Is it available on PATH? You could possibly provide the path to ruff executable in ruff.path and can even use VS Code specific variables like ${workspaceFolder} or (in your case) ${workspaceFolder:root}, etc.

@dhruvmanila dhruvmanila added question Asking for support or clarification and removed needs-decision Undecided if this should be done labels Aug 12, 2024
@stan-stately
Copy link
Author

ruff is on the path as a wrapper/shim but mise throws an error if it can't find the mise.toml file in the CWD or parent dirs because it doesn't know what version is required.

the only real way to fix my problem is allow the user to set the CWD. I can make a repro if you'd like so you can see for yourself

@dhruvmanila
Copy link
Member

dhruvmanila commented Aug 13, 2024

ruff is on the path as a wrapper/shim but mise throws an error if it can't find the mise.toml file in the CWD or parent dirs because it doesn't know what version is required.

Ah, I see.

the only real way to fix my problem is allow the user to set the CWD.

This could be a possible solution. I think there's a VS Code API to do that.

I think one solution here, apart from using the bundled executable, is to provide the ruff.path. For your use-case, it would be more useful to update the workspace settings:

{
  "folders": [
	    {
	      "name": "root",
	      "path": ".",
	    },
	    {
	      "name": "other",
	      "name": "../other",
	    }
	],
	"settings": {
		"ruff.path": ["${workspaceFolder:root}/.venv/bin/ruff"]
	}
}

Update the remaining path after ${workspaceFolder:root} relative to the "root" directory.

I'll leave this issue open for now.

@dhruvmanila dhruvmanila added needs-decision Undecided if this should be done and removed question Asking for support or clarification labels Aug 13, 2024
@stan-stately
Copy link
Author

i tried. it still doesn't work because mise finds the executable but cant run it if there is no mise file. i think it generates a shim and puts it in the path as ruff. The working directory is really important here:

[~/Workspace/<my-project>] $ which ruff
/Users/stan-stately/.local/share/mise/shims/ruff
[~/Workspace/<my-project>] $ ruff version
ruff 0.5.5 (fc16d8d04 2024-07-25)
[~/Workspace/<my-project>] $ cd ..
[~/Workspace] $ ruff version
mise No version is set for shim: ruff
Set a global default version with one of the following:
mise use -g [email protected]
mise Run with --verbose or MISE_VERBOSE=1 for more information

@jamesharris-garmin
Copy link

For my involvement it turns out I was still using the black auto formatter by default and not the ruff one. That was my mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Undecided if this should be done
Projects
None yet
Development

No branches or pull requests

3 participants