Skip to content

bugfix: stop a build tool restarting its daemon on every debug session - #8774

Open
damiankus-vl wants to merge 1 commit into
scalameta:main-v2from
damiankus-vl:bugfix-BazelDapMbtLspSuite
Open

bugfix: stop a build tool restarting its daemon on every debug session#8774
damiankus-vl wants to merge 1 commit into
scalameta:main-v2from
damiankus-vl:bugfix-BazelDapMbtLspSuite

Conversation

@damiankus-vl

@damiankus-vl damiankus-vl commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Reproduction steps

  • run BazelDapMbtLspSuite
  • check the last test case - bazel-mbt-test-multiple-test-targets-same-build-file

Actual behavior (main-v2)

The test case fails due to differences in JAVA_HOME path spelling
If we add a client_debug parameter (apply this patch: client_debug.patch), we can observe this warning

Note the difference in the trailing slash
default_system_javabase=/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home
default_system_javabase=/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home/

2026.08.06 10:07:04 ERROR [INFO 10:07:04.480 src/main/cpp/blaze.cc:1032] Args from the running server that are not included in the current request:
2026.08.06 10:07:04 ERROR [INFO 10:07:04.480 src/main/cpp/blaze.cc:1035]   --default_system_javabase=/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home
2026.08.06 10:07:04 ERROR [INFO 10:07:04.480 src/main/cpp/blaze.cc:1039] Args from the current request that were not included when creating the server:
2026.08.06 10:07:04 ERROR [INFO 10:07:04.480 src/main/cpp/blaze.cc:1042]   --default_system_javabase=/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home/
2026.08.06 10:07:04 ERROR [WARNING 10:07:04.480 src/main/cpp/blaze.cc:1075] Running Bazel server needs to be killed, because the startup options are different.
2026.08.06 10:07:04 ERROR [INFO 10:07:04.480 src/main/cpp/blaze.cc:1833] Shutting running server with RPC request
Screenshot 2026-08-06 at 09 40 06

Expected behavior (fix)

BazelDapMbtLspSuite succeeds
Screenshot 2026-08-06 at 11 42 14


Description of changes

The MBT debug path exported no JAVA_HOME when neither the build target nor the user configured a java home, so the launched command inherited whatever the environment held. That value often ends with a separator, /jdk/Home/, while every command that goes through ShellRunner exports the same JDK as /jdk/Home. The build tool compares JAVA_HOME against the value its running daemon started with, sees two different JDKs, and shuts the daemon down. It did that between the importer's commands and the ones Metals launched, and the analysis cache went with it.

The debug path now goes through JdkSources.envVariables, like every other command, so all of them export the same spelling. JdkSources also accepts a file: URI, which is how a BSP JvmBuildTarget reports its java home; AbsolutePath alone reads file:///jdk/Home as a relative path whose first segment is file:.

JdkSourcesSuite covers the spellings a java home reaches Metals in, one case per platform: a trailing separator, a file: URI, a percent-encoded space, a Windows drive or share, and the spellings that no path can hold.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of Java home locations supplied as file URIs or filesystem paths.
    • Fixed parsing for Windows drive paths, UNC paths, Unix and macOS paths, and paths containing encoded spaces or backslashes.
    • Improved Java home environment setup for debugging and command execution.
    • Added safer handling for empty or invalid Java home values while preserving existing warnings.
  • Tests
    • Expanded coverage for platform-specific Java home parsing scenarios.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 37d15d7d-36bd-4690-bbf8-f22e636ecd5f

📥 Commits

Reviewing files that changed from the base of the PR and between 2939548 and b69a69c.

📒 Files selected for processing (4)
  • metals/src/main/scala/scala/meta/internal/builds/ShellRunner.scala
  • metals/src/main/scala/scala/meta/internal/metals/mbt/MbtDebugSessionStarter.scala
  • mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala
  • tests/unit/src/test/scala/tests/JdkSourcesSuite.scala
🚧 Files skipped from review as they are similar to previous changes (4)
  • metals/src/main/scala/scala/meta/internal/builds/ShellRunner.scala
  • mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala
  • tests/unit/src/test/scala/tests/JdkSourcesSuite.scala
  • metals/src/main/scala/scala/meta/internal/metals/mbt/MbtDebugSessionStarter.scala

📝 Walkthrough

Walkthrough

The change centralizes JAVA_HOME environment construction in JdkSources, adds case-insensitive file: URI parsing, and tests platform-specific path forms.

Changes

Java home parsing and integration

Layer / File(s) Summary
Parse and validate Java home values
mtags/src/main/.../JdkSources.scala, tests/unit/src/test/.../JdkSourcesSuite.scala
JdkSources.fromString parses file: URIs through URI and Paths, retains direct path parsing, and adds platform-specific validation tests.
Use shared Java environment construction
metals/src/main/.../mbt/MbtDebugSessionStarter.scala, metals/src/main/.../builds/ShellRunner.scala
MbtDebugSessionStarter and ShellRunner delegate Java environment construction to JdkSources.envVariables.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: tgodzik

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary fix: preventing build tool daemon restarts during debug sessions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala`:
- Line 51: Update the fromString parsing branch in JdkSources so the file:
prefix check is case-insensitive, allowing mixed-case schemes such as FILE:.
Preserve URI-to-AbsolutePath conversion for matching values and add a test
covering a mixed-case file URI.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d815745a-5494-4705-9005-6b3e13a82b3c

📥 Commits

Reviewing files that changed from the base of the PR and between 33f383d and e8901d9.

📒 Files selected for processing (3)
  • metals/src/main/scala/scala/meta/internal/metals/mbt/MbtDebugSessionStarter.scala
  • mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala
  • tests/unit/src/test/scala/tests/JdkSourcesSuite.scala

Comment thread mtags/src/main/scala/scala/meta/internal/metals/JdkSources.scala Outdated
@damiankus-vl
damiankus-vl marked this pull request as draft August 6, 2026 09:58
@damiankus-vl
damiankus-vl force-pushed the bugfix-BazelDapMbtLspSuite branch 2 times, most recently from d8eb02d to 569d67e Compare August 6, 2026 10:54
@damiankus-vl
damiankus-vl marked this pull request as ready for review August 6, 2026 10:56
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

1 similar comment
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

The MBT debug path exported no JAVA_HOME when neither the build target nor
the user configured a java home, so the launched command inherited whatever
the environment held. That value often ends with a separator, `/jdk/Home/`,
while every command that goes through `ShellRunner` exports the same JDK as
`/jdk/Home`. The build tool compares JAVA_HOME against the value its running
daemon started with, sees two different JDKs, and shuts the daemon down. It
did that between the importer's commands and the ones Metals launched, and
the analysis cache went with it.

The debug path now goes through `JdkSources.envVariables`, like every other
command, so all of them export the same spelling. `JdkSources` also accepts
a `file:` URI, which is how a BSP `JvmBuildTarget` reports its java home;
`AbsolutePath` alone reads `file:///jdk/Home` as a relative path whose first
segment is `file:`.

`JdkSourcesSuite` covers the spellings a java home reaches Metals in, one
case per platform: a trailing separator, a `file:` URI, a percent-encoded
space, a Windows drive or share, and the spellings that no path can hold.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@damiankus-vl
damiankus-vl force-pushed the bugfix-BazelDapMbtLspSuite branch from 2c95316 to b69a69c Compare August 6, 2026 11:40
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant