feat(cli): unityLicensingToolset option + relocate platform-setup to engine scope#50
Conversation
Mirrors unity-builder#838 and orchestrator#24 — surfaces an optional unityLicensingToolset / -lt flag for users on Unity floating-license servers that host multiple toolsets. When set, written into services-config.json so the Licensing Client requests entitlements from the named toolset instead of relying on the server-side default. Background: when a license server hosts multiple toolsets and the wrong one (or no default) is selected, Unity falls through to a license that lacks the requested build-target support and silently produces the wrong artifact. See game-ci/unity-builder#739 for the original investigation. Opt-in and backwards compatible — when unset, the rendered config is byte-for-byte identical to before. The runAsHostUser HOME/USER fix from unity-builder#838 does not apply here: cli's entrypoint.sh has no runAsHostUser branch (runs as root unconditionally).
📝 WalkthroughWalkthroughThis PR adds support for specifying a Unity floating-license toolset identifier via CLI option. The new ChangesUnity Licensing Toolset Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/model/platform-setup.ts`:
- Around line 34-41: The code currently does a raw string replace of
unityLicensingServer into servicesConfig and then calls JSON.parse (in the
SetupShared flow using servicesConfigPathTemplate and unityLicensingToolset),
which can corrupt JSON and throw an uncaught SyntaxError; change the flow to
read and JSON.parse the template first inside a try/catch (wrap the JSON.parse
to throw a clear error if parsing fails), then set the URL property and
parsed.toolset property on the resulting object (instead of replacing in the raw
string), and finally JSON.stringify that object; if you cannot determine the URL
property name programmatically, at minimum wrap the existing JSON.parse in
try/catch and validate/escape unityLicensingServer for JSON-unsafe chars before
doing the string replace.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c19aa71-b66e-4d7f-a2ed-55182412feda
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
src/command-options/unity-options.tssrc/model/platform-setup.ts
| let servicesConfig = nodeFs.readFileSync(servicesConfigPathTemplate, 'utf-8'); | ||
| servicesConfig = servicesConfig.replace('%URL%', unityLicensingServer); | ||
|
|
||
| if (unityLicensingToolset) { | ||
| const parsed = JSON.parse(servicesConfig); | ||
| parsed.toolset = unityLicensingToolset; | ||
| servicesConfig = JSON.stringify(parsed, undefined, 2); | ||
| } |
There was a problem hiding this comment.
JSON.parse can throw an uncaught SyntaxError; URL with JSON-special chars silently corrupts the parse.
There are two related problems on the new unityLicensingToolset code path:
-
Unhandled parse error.
JSON.parse(servicesConfig)at line 38 is uncaught.SetupSharedis synchronous and its caller (setup, line 12) does not wrap it, so aSyntaxErrorwill propagate as an unhandled exception with a confusing stack trace rather than a clean error message. -
URL injection into raw JSON before parse. Line 35 splices
unityLicensingSerververbatim into the raw JSON text with a string-replace. If the URL contains a"or\(e.g. from a misconfigured value), the resulting string is no longer valid JSON andJSON.parseat line 38 will throw. This path is exercised only whenunityLicensingToolsetis also set, making it a silent regression for users using both options together.
The safest fix is to parse the unmodified template first, then assign both the URL field and the toolset field as object properties, which avoids raw-string surgery entirely:
🛡️ Proposed fix
- let servicesConfig = nodeFs.readFileSync(servicesConfigPathTemplate, 'utf-8');
- servicesConfig = servicesConfig.replace('%URL%', unityLicensingServer);
-
- if (unityLicensingToolset) {
- const parsed = JSON.parse(servicesConfig);
- parsed.toolset = unityLicensingToolset;
- servicesConfig = JSON.stringify(parsed, undefined, 2);
- }
-
- nodeFs.writeFileSync(servicesConfigPath, servicesConfig);
+ const templateText = nodeFs.readFileSync(servicesConfigPathTemplate, 'utf-8');
+
+ let servicesConfig: string;
+ if (unityLicensingToolset) {
+ try {
+ const parsed = JSON.parse(templateText);
+ // Assign both fields as typed properties instead of raw-string surgery
+ if (unityLicensingServer) {
+ // Replicate whatever property the template placeholder maps to
+ parsed.licensingServerURL = unityLicensingServer; // adjust key to match template schema
+ }
+ parsed.toolset = unityLicensingToolset;
+ servicesConfig = JSON.stringify(parsed, undefined, 2);
+ } catch (err) {
+ log.error(`Failed to parse services config template: ${err}`);
+ return;
+ }
+ } else {
+ servicesConfig = templateText.replace('%URL%', unityLicensingServer);
+ }
+
+ nodeFs.writeFileSync(servicesConfigPath, servicesConfig);Note: The exact JSON property name that
%URL%maps to in the template is not visible here. If you prefer to keep the string-replace approach for the URL (e.g. because the key name isn't easily determined programmatically), the minimum safe change is to wrap the existingJSON.parsein atry/catchand validate the URL for JSON-unsafe characters before substitution.
Verify the template's schema to confirm the right property key for the URL field, and check whether the %URL% → property mapping can be replaced cleanly:
#!/bin/bash
# Locate the services-config.json.template to inspect its schema
fd 'services-config.json.template' --exec cat {}🤖 Prompt for 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.
In `@src/model/platform-setup.ts` around lines 34 - 41, The code currently does a
raw string replace of unityLicensingServer into servicesConfig and then calls
JSON.parse (in the SetupShared flow using servicesConfigPathTemplate and
unityLicensingToolset), which can corrupt JSON and throw an uncaught
SyntaxError; change the flow to read and JSON.parse the template first inside a
try/catch (wrap the JSON.parse to throw a clear error if parsing fails), then
set the URL property and parsed.toolset property on the resulting object
(instead of replacing in the raw string), and finally JSON.stringify that
object; if you cannot determine the URL property name programmatically, at
minimum wrap the existing JSON.parse in try/catch and validate/escape
unityLicensingServer for JSON-unsafe chars before doing the string replace.
Move src/model/platform-setup.ts → src/logic/unity/platform-setup/platform-setup.ts. The class only writes Unity's services-config.json and dispatches to Unity-specific Mac/Windows/Android setup modules — it is engine-specific code that lived in cli's engine-agnostic core directory by accident, carried over from the original unity-builder extraction. cli's intended boundary: src/command/, src/model/, src/logic/, src/middleware/ ← engine-agnostic src/command-options/unity-options.ts, src/logic/unity/ ← Unity-scoped src/model/unity/, src/command/build/unity-build-command ← Unity-scoped The unityLicensingToolset support added in the previous commit lives inside the SetupShared method that this commit relocates, so the combined diff places the toolset injection in the correct (Unity- scoped) location, not in engine-agnostic core. Single caller (src/command/build/unity-build-command.ts — itself Unity-scoped) updated to import from the new location. Tracking issue: #51 Out of scope (separate phases per the tracking issue): - src/model/image-environment-factory.ts hardcodes UNITY_LICENSE, UNITY_LICENSE_FILE, UNITY_SERIAL, UNITY_LICENSING_SERVER. Engine module should supply its env-var list. - src/model/docker.ts hardcodes --env UNITY_SERIAL. Same. - These are larger refactors that touch critical paths; bundling them with the toolset fix would balloon the PR.
frostebite
changed the title
2 participants
Summary
Two coordinated changes addressing the same architectural concern:
unityLicensingToolsetoption (-lt) for users on Unity floating-license servers that host multiple toolsets. Mirrors fix: license server toolset input + runAsHostUser HOME/USER (closes #739) unity-builder#838.Tracking issue: #51 (cli's engine-agnostic boundary, current violations, phased plan).
Background — why both changes are in one PR
cli is structured around an explicit engine-agnostic / engine-scoped split:
src/command/,src/model/,src/logic/,src/middleware/src/command-options/unity-options.ts,src/logic/unity/,src/model/unity/,src/command/build/unity-build-command.tssrc/model/platform-setup.tswas a violation — engine-agnostic location but engine-specific content (writing Unity'sservices-config.json, dispatching to Unity Mac/Windows/Android setup). My initial commit addedunityLicensingToolsetinjection inside that file, perpetuating the violation. Reviewer flagged this. The follow-up commit fixes the boundary properly: the file moves tosrc/logic/unity/platform-setup/platform-setup.ts, the toolset injection rides along into the correct location, and the single caller (itself Unity-scoped) is updated.Net result: adding a new Unity field touches only Unity-scoped directories. The next licensing knob doesn't repeat this PR's mistake.
Commits
feat(cli): add unityLicensingToolset option for floating-license servers— adds the yargs option and theservices-config.jsontoolset injectionrefactor(cli): move Unity-specific platform-setup to logic/unity/ scope— relocates the file, updates imports, deletes the old engine-agnostic location, adds boundary doc-blockFiles changed
src/command-options/unity-options.tsunityLicensingToolset(alias-lt) — Unity-scoped, correct locationsrc/logic/unity/platform-setup/platform-setup.tssrc/logic/unity/platform-setup/index.tsPlatformSetupsrc/model/platform-setup.tssrc/command/build/unity-build-command.tsdist/index.jsAre these opt-in / backwards compatible?
--unity-licensing-toolset--unity-licensing-toolsetfrom '../../model/platform-setup'Out of scope — separate cleanups per #51
src/model/image-environment-factory.tshardcodesUNITY_LICENSE,UNITY_LICENSE_FILE,UNITY_SERIAL,UNITY_LICENSING_SERVER. Engine module should supply env-var list.src/model/docker.tshardcodes--env UNITY_SERIALin the docker invocation. Same.Test plan
bun test— 86 passed, 3 skipped, 0 failedbun run buildregeneratesdist/index.jscontaining the new option🤖 Generated with Claude Code