[2.x] fix: Fix sbtopts files priority in sbt launch script#8514
Merged
eed3si9n merged 11 commits intosbt:developfrom Jan 13, 2026
Merged
[2.x] fix: Fix sbtopts files priority in sbt launch script#8514eed3si9n merged 11 commits intosbt:developfrom
eed3si9n merged 11 commits intosbt:developfrom
Conversation
- Reorder sbtopts file loading: dist → machine → project - Change project .sbtopts from prepend to append so it appears last - Project-level .sbtopts now correctly overrides machine-wide configs - Fixes issue where project options were overridden by dist defaults
eed3si9n
requested changes
Jan 13, 2026
Member
eed3si9n
left a comment
eed3si9n
left a comment
There was a problem hiding this comment.
Thanks for working on this issue!
Could you add tests in launcher-package/integration-test/src/test/scala/RunnerScriptTest.scala please?
- Add three integration tests to verify sbtopts priority order - Test that project .sbtopts overrides dist sbtopts - Test that project .sbtopts overrides machine sbtopts - Test that project .sbtopts overrides both dist and machine sbtopts - Make retry and javaBinDir protected in ShellScriptUtil for test access
- Restore if-else structure (only one of machine or dist is read) - Change from prepend to append for machine/dist sbtopts - Project .sbtopts still appended last to win for duplicate properties
Contributor
Author
|
@eed3si9n Thank you for your feedback, I have added test, and reverted .sbtopts file. |
eed3si9n
reviewed
Jan 13, 2026
launcher-package/integration-test/src/test/scala/RunnerScriptTest.scala
Outdated
Show resolved
Hide resolved
- Extend testOutput to support distSbtoptsContents and machineSbtoptsContents - Refactor sbtopts priority tests to use testOutput pattern - Remove unused imports from RunnerScriptTest - Follow existing test patterns as requested by reviewer
- Move sbtHome and configHome declarations outside try block - Update vararg syntax from : _* to * for Scala 3 compatibility
mohansinghi
force-pushed
the
fix/sbtopts-priority-7179
branch
from
4e3118b to
da1ae09
Compare
Contributor
Author
|
@eed3si9n I have fixed issues on testing. Please review again.. |
- Set XDG_CONFIG_HOME to empty temp directory when testing dist-only - This prevents script from finding machine sbtopts and ensures dist is loaded - Add assertions to verify dist sbtopts file is created correctly
- When machine sbtopts exists, script only loads machine (not dist) due to if-else - Update test to expect only machine and project, not dist - This matches the actual script behavior
Contributor
Author
|
@eed3si9n I tested on my local, and fixed all issues. Could you please review again? |
eed3si9n
reviewed
Jan 13, 2026
launcher-package/integration-test/src/test/scala/ShellScriptUtil.scala
Outdated
Show resolved
Hide resolved
- Copy entire sbt staging directory to temporary location when testing dist sbtopts - Use copied script instead of modifying staging directory - Addresses reviewer feedback about not modifying staging directory
Contributor
Author
|
👍 |
eed3si9n
changed the title
Member
|
I created an issue to backport this PR to 1.12.x branch, if you're up for it - #8519 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Project-level
.sbtoptssettings were being overridden by distribution defaults because the script loaded files in the wrong order. Since Java processes duplicate-Dproperties left-to-right (last wins), project options appearing first were overridden.Solution
.sbtoptsfrom prepend to append so it appears last and winsChanges
/sbtscript lines 854-862.sbtoptsnow appended instead of prependedTesting
Verified that project-level config now correctly overrides machine-wide config:
machine-configfirst,project-locallastproject-local✓Fixes #7179
Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=158349177