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

@jketemajketema commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

See internal PR for a full explanation.

@github-actionsgithub-actionsBot added C# JS Java Python Go Ruby Rust Pull requests that update Rust code labels Jul 6, 2026
@jketemajketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jul 6, 2026
@jketema
jketemaforce-pushed the jketema/subst-fix branch from ff4372b to 8b2ab6eCompareJuly 6, 2026 17:09
@jketemajketema changed the title Update expected test results after CLI/extractor changesIntroduce canonicalization library for Windows subst drivesJul 7, 2026
@jketema
jketemaforce-pushed the jketema/subst-fix branch 4 times, most recently from ef25e44 to 5d4429cCompareJuly 7, 2026 15:23
@jketemajketema changed the title Introduce canonicalization library for Windows subst drivesIntroduce canonicalization library for Windows subst drivesJul 10, 2026
@jketema
jketema marked this pull request as ready for review July 21, 2026 11:43
CopilotAI review requested due to automatic review settings July 21, 2026 11:43
@jketema
jketema requested review from a team as code ownersJuly 21, 2026 11:43

CopilotAI 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
FileDescription
MODULE.bazelExposes the Go Windows dependency.
go/extractor/extractor.goResolves subst paths during normalization.
go/extractor/util/subst_windows.goImplements Windows DLL integration.
go/extractor/util/subst_other.goAdds the non-Windows no-op.
go/extractor/util/BUILD.bazelBuilds platform-specific resolver sources.
go/extractor/go.modAdds x/sys.
go/extractor/go.sumRecords dependency checksums.
java/kotlin-extractor/src/main/java/com/semmle/util/files/SubstResolver.javaAdds Java native resolution.
java/kotlin-extractor/src/main/java/com/semmle/util/files/FileUtil.javaApplies resolution during canonicalization.
go/ql/integration-tests/subst/file.expectedUpdates Go expected paths.
java/ql/integration-tests/java/subst/file.expectedUpdates Java/Kotlin expected paths.
javascript/ql/integration-tests/subst/file.expectedUpdates JavaScript expected paths.
python/ql/integration-tests/subst/file.expectedUpdates Python expected paths.
ruby/ql/integration-tests/subst/file.expectedUpdates Ruby expected paths.
rust/ql/integration-tests/subst/file.expectedUpdates Rust expected paths.
csharp/ql/integration-tests/windows/subst/file.expectedUpdates C# expected paths.

Review details

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

Comment threadgo/extractor/util/subst_windows.go
Comment threadgo/extractor/util/subst_windows.go
geoffw0
geoffw0 previously approved these changes Jul 23, 2026

@geoffw0geoffw0 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

Copy link
Copy Markdown
ContributorAuthor

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

@geoffw0

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

Copy link
Copy Markdown
ContributorAuthor

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.

@geoffw0geoffw0 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:mainJul 28, 2026
100 of 115 checks passed
@jketema
jketema deleted the jketema/subst-fix branch July 28, 2026 14:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C#depends on internal PRThis PR should only be merged in sync with an internal Semmle PRGoJavaJSKotlinPythonRubyRustPull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jketema@geoffw0