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

Rust-analyzer uses lowercase letters for Windows drives #14683

Closed
jyn514 opened this issue Apr 29, 2023 · 6 comments · Fixed by #14689
Closed

Rust-analyzer uses lowercase letters for Windows drives #14683

jyn514 opened this issue Apr 29, 2023 · 6 comments · Fixed by #14689
Labels
C-bug Category: bug

Comments

@jyn514
Copy link
Member

jyn514 commented Apr 29, 2023

When I run x check in the rust-lang/rust repo, it unconditionally recompiles rustc_llvm on Windows only. CARGO_LOG=info shows the following:

[2023-04-29T10:37:19Z INFO  cargo::core::compiler::fingerprint] fingerprint dirty for rustc_llvm v0.0.0 (C:\Users\jyn\src\rust2\compiler\rustc_llvm)/RunCustomBuild/TargetInner { ..: custom_build_target("build-script-build", "C:\\Users\\jyn\\src\\rust2\\compiler\\rustc_llvm\\build.rs", Edition2021) }      
[2023-04-29T10:37:19Z INFO  cargo::core::compiler::fingerprint]     dirty: EnvVarChanged { name: "LLVM_CONFIG", old_value: Some("c:\\Users\\jyn\\src\\rust2\\build\\x86_64-pc-windows-msvc\\ci-llvm\\bin\\llvm-config.exe"), new_value: Some("C:\\Users\\jyn\\src\\rust2\\build\\x86_64-pc-windows-msvc\\ci-llvm\\bin\\llvm-config.exe") }

I think c:\ must be coming from rust-analyzer; running x check twice in a row doesn't recompile, only if I've saved in between. Disabling checkOnSave makes the problem go away.


some more detailed tracing:

LLVM_CONFIG is set here: https://github.com/rust-lang/rust/blob/8ce92daa854e329f4fb7ac75c28dbeea3f5bb125/src/bootstrap/compile.rs#L890
llvm_config comes from https://github.com/rust-lang/rust/blob/8ce92daa854e329f4fb7ac75c28dbeea3f5bb125/src/bootstrap/llvm.rs#L83
which in turn comes from https://github.com/rust-lang/rust/blob/214894275722249e434d24507771b6fef789df55/src/bootstrap/config.rs#L1319;
ci_llvm_root is just forwarding self.out https://github.com/rust-lang/rust/blob/214894275722249e434d24507771b6fef789df55/src/bootstrap/config.rs#L1516
which defaults to build and is set to an absolute path here: https://github.com/rust-lang/rust/blob/214894275722249e434d24507771b6fef789df55/src/bootstrap/util.rs#L469-L475

I don't know why GetFullPathName would return a different value between the bootstrap launched from powershell and the one launched by rust-analyzer.


rust-analyzer version: rust-analyzer version: 0.4.1494-standalone (370b72c 2023-04-28)

rustc version: HEAD is branched from rust-lang/rust@572c0d5; the beta compiler version is beta-2023-04-20.

relevant settings: various; bootstrap sets lots of things. .vscode/settings.json is as follows:

{
    "rust-analyzer.check.invocationLocation": "root",
    "rust-analyzer.check.invocationStrategy": "once",
    "rust-analyzer.check.overrideCommand": [
        "python",
        "x.py",
        "check",
        "--json-output"
    ],
    "rust-analyzer.linkedProjects": ["src/bootstrap/Cargo.toml", "Cargo.toml"],
    "rust-analyzer.rustfmt.overrideCommand": [
        "./build/host/rustfmt/bin/rustfmt",
        "--edition=2021"
    ],
    "rust-analyzer.procMacro.server": "./build/host/stage0/libexec/rust-analyzer-proc-macro-srv",
    "rust-analyzer.procMacro.enable": true,
    "rust-analyzer.cargo.buildScripts.enable": true,
    "rust-analyzer.cargo.buildScripts.invocationLocation": "root",
    "rust-analyzer.cargo.buildScripts.invocationStrategy": "once",
    "rust-analyzer.cargo.buildScripts.overrideCommand": [
        "python",
        "x.py",
        "check",
        "--json-output"
    ],
    "rust-analyzer.cargo.sysrootSrc": "./library",
    "rust-analyzer.rustc.source": "./Cargo.toml"
}
@jyn514 jyn514 added the C-bug Category: bug label Apr 29, 2023
@Veykril
Copy link
Member

Veykril commented Apr 29, 2023

A simple guess from my side would be that we set the current dir explicitly for most (if not all) commands and we only deal with paths in absolute forms, meaning we canonicalize paths where possible. It might be that canonicalizing returns lower case drive letters for some reason? One thing to check would be putting absolute paths instead of relative ones in the linkedProjects config

@Veykril
Copy link
Member

Veykril commented Apr 29, 2023

It's vscode sending us lowercase driver letters for the workspace roots

"InitializeParams":{
   "processId":15652,
   "clientInfo":{
      "name":"Visual Studio Code",
      "version":"1.76.2"
   },
   "locale":"en-us",
   "rootPath":"c:\\workspace\\rust\\rust-analyzer",
   "rootUri":"file:///c%3A/workspace/rust/rust-analyzer",
   "initializationOptions":{
      "..."
   },
   "trace":"off",
   "workspaceFolders":[
      {
         "uri":"file:///c%3A/workspace/rust/rust-analyzer",
         "name":"rust-analyzer"
      }
   ],
   "..."
}

@Veykril
Copy link
Member

Veykril commented Apr 29, 2023

So I'm fairly certain this is a wont fix on the VSCode side (judging from past discussions regarding their broken path caching in regards to letter casing) which means we probably want to fix this on the r-a side somehow.

Actually, can drive letters even be lowercase on windows? I assume we can just force them to be uppercase

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

One thing to check would be putting absolute paths instead of relative ones in the linkedProjects config

That did not help :(

Actually, can drive letters even be lowercase on windows? I assume we can just force them to be uppercase

👍 that seems like a good solution. We could also try doing that in bootstrap, but I imagine other people on windows have this problem too and just haven't complained.

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

"can be lowercase" is not really the right way to think about it though - NTFS is case-insensitive on Windows.

@Veykril
Copy link
Member

Veykril commented Apr 29, 2023

"can be lowercase" is not really the right way to think about it though - NTFS is case-insensitive on Windows.

Well it is case-insensitive in that the casing doesn't matter for checking paths etc. Casing is still very much a thing for displaying/fetching, so assuming the drive letter of a drive could be assigned in lowercase, fixing it by just uppercasing it wouldn't really fix the problem in theory (since we could end up with the reverse if VSCode always reported it as uppercase).
But that's nitpicking and not something to worry about.
I'd say we fix it in r-a since in theory there can be general built scripts observing this and causing a similar invalidation (build probing comes to mind).

bors added a commit that referenced this issue Apr 29, 2023
fix: Force InitializeParams windows path drives to uppercase

Should fix #14683
cc `@jyn514`
bors added a commit that referenced this issue Apr 29, 2023
fix: Force InitializeParams windows path drives to uppercase

Should fix #14683
cc `@jyn514`
@bors bors closed this as completed in 7bcb4c2 Apr 29, 2023
mataha added a commit to mataha/zoxide that referenced this issue Aug 16, 2023
When it comes to resolving paths on Windows, even though the underlying
API expects drive letter prefixes to be uppercase, some sources (e.g.
environment variables like `=C:`) won't normalize components, instead
returning the value as-is. While this wouldn't be a problem normally as
NTFS is case-insensitive on Windows, this introduces duplicates in the
database when adding new entries via `zoxide add`:

```batchfile prompt
> zoxide query --list
D:\
d:\
D:\coding
d:\coding
D:\coding\.cloned
d:\coding\.cloned
```

This is a cherry-pick from ajeetdsouza#567; see also rust-lang/rust-analyzer#14683.

Signed-off-by: mataha <mataha@users.noreply.github.com>
mataha added a commit to mataha/zoxide that referenced this issue Dec 15, 2023
When it comes to resolving paths on Windows, even though the underlying
API expects drive letter prefixes to be uppercase, some sources (e.g.
environment variables like `=C:`) won't normalize components, instead
returning the value as-is. While this wouldn't be a problem normally as
NTFS is case-insensitive on Windows, this introduces duplicates in the
database when adding new entries via `zoxide add`:

```batchfile prompt
> zoxide query --list
D:\
d:\
D:\coding
d:\coding
D:\coding\.cloned
d:\coding\.cloned
```

This is a cherry-pick from ajeetdsouza#567; see also rust-lang/rust-analyzer#14683.

Signed-off-by: mataha <mataha@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants