fix: unify licensing precedence across platforms, warn on ambiguity - #256
Merged
Conversation
Two related changes to how UNITY_LICENSING_METHOD auto-resolution works, both aimed at the same root problem: two real regressions (#254, #255) took multiple rounds to fully diagnose because nothing said out loud which of several plausible activation strategies actually got used, and the four platform scripts didn't even agree with each other about which one that should be. 1. Unify precedence across all four platform scripts. ubuntu, mac and windows/steps already agreed: file -> serial -> floating -> personal. The windows *container* script set diverged in two ways that predated this session entirely: it checked floating before serial, and its catch-all attempted a doomed serial activation with whatever credentials happened to be set (including none at all) instead of the same clear "could not be determined" guidance the other three already gave. Aligned the minority to the majority rather than the reverse, so this only changes behaviour for the narrow edge cases where the old windows-container-only order actually mattered: a build that set both a complete serial triple and a licensing server (took floating, now takes serial, matching every other platform), or one with no usable credentials at all (previously ran Unity with empty -serial/-username/-password and failed with Unity's generic error, now gets the same guidance message every other platform already gives). 2. Warn out loud whenever a credential naming a *specific* strategy (a .ulf, a real serial, a licensing server) is silently overridden by a different auto-resolved strategy. This is purely additive - it changes no resolution outcome on any platform, just makes an already-surprising outcome visible in the log instead of only discoverable by reading source. An explicit --unityLicensingMethod is never second-guessed and never triggers it. New test coverage: scripts/test-licensing-steps.ps1 is a new sibling of the existing bash suite, covering both Windows script sets end-to-end (no PowerShell test harness existed for them before this - a syntax check alone would not have caught a mistake in the precedence change). Wired into tests.yml; pwsh is preinstalled on ubuntu-latest, so no Windows runner is needed. 5 new bash cases and 30 new PowerShell cases cover the warning and the unified precedence directly. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Warning Review limit reachedNext included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (5)
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. Comment |
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
1 participant
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.
Summary
Two related changes to how
UNITY_LICENSING_METHODauto-resolution works, both aimed at the same root problem: two real regressions (#254, #255) took multiple rounds to fully diagnose because nothing said out loud which of several plausible activation strategies actually got used, and the four platform scripts didn't even agree with each other about which one that should be.1. Unify precedence across all four platform scripts
ubuntu,macandwindows/stepsalready agreed:file -> serial -> floating -> personal. The windows container script set diverged in two ways that predated this session entirely:floatingbeforeserialAligned the minority to the majority rather than the reverse, so this only changes behaviour for the narrow edge cases where the old windows-container-only order actually mattered:
floating, now takesserial, matching every other platform)-serial/-username/-passwordand failed with Unity's generic error, now gets the same guidance message every other platform already gives)2. Warn out loud on ambiguous auto-resolution
Whenever a credential naming a specific strategy (a
.ulf, a real serial, a licensing server) is silently overridden by a different auto-resolved strategy, this now says so. Purely additive - it changes no resolution outcome on any platform, just makes an already-surprising outcome visible in the log instead of only discoverable by reading source. An explicit--unityLicensingMethodis never second-guessed and never triggers it.Test plan
scripts/test-licensing-steps.ps1is a new sibling of the existing bash suite, covering both Windows script sets end-to-end - no PowerShell test harness existed for them before this, and a syntax check alone would not have caught a mistake in the precedence change. Wired intotests.yml;pwshis preinstalled onubuntu-latest, so no Windows runner is needed.bash scripts/test-licensing-steps.sh- 37/37 pass (32 existing + 5 new ambiguity-warning cases)pwsh scripts/test-licensing-steps.ps1- 30/30 pass (precedence, return-strategy, and ambiguity-warning cases, run against both windows script sets)bash scripts/validate-platform-scripts.sh- cleanbun test ./src/logic/unity/license/licensing-method.test.ts- 15/15 passbun test ./src- same 2 pre-existing, unrelated failures as cleanmain(confirmed via stash-and-compare), no regressionsnode scripts/generate-embedded-assets.mjs --check- clean after regenerating for thedist/changes🤖 Generated with Claude Code