Skip to content

Introduce canonicalization library for Windows subst drives - #22129

Merged
jketema merged 7 commits into
github:mainfrom
jketema:jketema/subst-fix
Jul 28, 2026
Merged

Introduce canonicalization library for Windows subst drives#22129
jketema merged 7 commits into
github:mainfrom
jketema:jketema/subst-fix

Conversation

@jketema

@jketema jketema commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

See internal PR for a full explanation.

@github-actions github-actions Bot added C# JS Java Python Go Ruby Rust Pull requests that update Rust code labels Jul 6, 2026
@jketema jketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jul 6, 2026
@jketema
jketema force-pushed the jketema/subst-fix branch from ff4372b to 8b2ab6e Compare July 6, 2026 17:09
@github-actions github-actions Bot added the Kotlin label Jul 7, 2026
@jketema jketema changed the title Update expected test results after CLI/extractor changes Introduce canonicalization library for Windows subst drives Jul 7, 2026
@jketema
jketema force-pushed the jketema/subst-fix branch 4 times, most recently from ef25e44 to 5d4429c Compare July 7, 2026 15:23
@jketema jketema changed the title Introduce canonicalization library for Windows subst drives Introduce canonicalization library for Windows subst drives Jul 10, 2026
@jketema
jketema force-pushed the jketema/subst-fix branch from 5d4429c to b13df1f Compare July 20, 2026 12:50
@jketema
jketema marked this pull request as ready for review July 21, 2026 11:43
Copilot AI review requested due to automatic review settings July 21, 2026 11:43
@jketema
jketema requested review from a team as code owners July 21, 2026 11:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds Windows subst drive canonicalization to Go and Java/Kotlin extraction paths.

Changes:

  • Loads and invokes canonicalize.dll from Go and Java.
  • Adds the Go Windows-system dependency and Bazel wiring.
  • Updates cross-language integration expectations for canonical paths.
Show a summary per file
File Description
MODULE.bazel Exposes the Go Windows dependency.
go/extractor/extractor.go Resolves subst paths during normalization.
go/extractor/util/subst_windows.go Implements Windows DLL integration.
go/extractor/util/subst_other.go Adds the non-Windows no-op.
go/extractor/util/BUILD.bazel Builds platform-specific resolver sources.
go/extractor/go.mod Adds x/sys.
go/extractor/go.sum Records dependency checksums.
java/kotlin-extractor/src/main/java/com/semmle/util/files/SubstResolver.java Adds Java native resolution.
java/kotlin-extractor/src/main/java/com/semmle/util/files/FileUtil.java Applies resolution during canonicalization.
go/ql/integration-tests/subst/file.expected Updates Go expected paths.
java/ql/integration-tests/java/subst/file.expected Updates Java/Kotlin expected paths.
javascript/ql/integration-tests/subst/file.expected Updates JavaScript expected paths.
python/ql/integration-tests/subst/file.expected Updates Python expected paths.
ruby/ql/integration-tests/subst/file.expected Updates Ruby expected paths.
rust/ql/integration-tests/subst/file.expected Updates Rust expected paths.
csharp/ql/integration-tests/windows/subst/file.expected Updates C# expected paths.

Review details

  • Files reviewed: 15/16 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread go/extractor/util/subst_windows.go
Comment thread java/kotlin-extractor/src/main/java/com/semmle/util/files/SubstResolver.java Outdated
Comment thread java/kotlin-extractor/src/main/java/com/semmle/util/files/SubstResolver.java Outdated
Comment thread java/kotlin-extractor/src/main/java/com/semmle/util/files/SubstResolver.java Outdated
Comment thread go/extractor/util/subst_windows.go
geoffw0
geoffw0 previously approved these changes Jul 23, 2026

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, subject to the internal PR getting approval, tests passing etc.

👍

@jketema

jketema commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@geoffw0 could you re-approve? Note that the tests here cannot pass, because of the dependency on the internal PR.

@geoffw0

geoffw0 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

It's not the tests that slowed me down, it's the force push. I'll check it over now and re-approve...

@jketema

jketema commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

My test remark was a comment on:

LGTM, subject to the internal PR getting approval, tests passing etc.

Apologies for the force push, but it's just the easiest way to resolve submodule conflicts.

The only thing new after the force push is 726705b, which is in line with the internal changes.

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@geoffw0

geoffw0 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Yep, I see the internal PR is now approved and has passing tests. 👍

@jketema
jketema merged commit 9d1a8a7 into github:main Jul 28, 2026
100 of 115 checks passed
@jketema
jketema deleted the jketema/subst-fix branch July 28, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# depends on internal PR This PR should only be merged in sync with an internal Semmle PR Go Java JS Kotlin Python Ruby Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants