Fix wrong mac path for License Server Activation when using Unity 6.3+ - #842
Fix wrong mac path for License Server Activation when using Unity 6.3+#842alexis-gervacio wants to merge 3 commits into
Conversation
|
|
📝 WalkthroughWalkthroughThis PR adds integrity tests for built macOS licensing scripts, checking script contents and verifying that Unity licensing client resolution switches at the version gate. ChangesIntegrity Test Additions
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
🧹 Nitpick comments (3)
src/integrity.test.ts (3)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge with existing
node:fs/promisesimport.
statis already imported fromnode:fs/promiseson Line 2; addreadFileto that same import instead of a separate statement.♻️ Proposed fix
-import { stat } from 'node:fs/promises'; -import { readFile } from 'node:fs/promises'; +import { stat, readFile } from 'node:fs/promises';🤖 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/integrity.test.ts` around lines 2 - 3, `src/integrity.test.ts` has duplicate imports from `node:fs/promises`; merge `readFile` into the existing import that already brings in `stat` instead of keeping a separate statement. Update the top-level import block in `integrity.test.ts` so both `stat` and `readFile` come from the same `node:fs/promises` import.
13-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate assertion sets between the two tests.
The
activate.shandreturn_license.shtests assert an identical set of five substrings, differing only in the script path being read. Consider a small data-driven helper to avoid repeating the assertion list.♻️ Proposed refactor
+ const expectedLicensingClientSnippets = [ + 'UNITY_LICENSING_CLIENT_SUBDIR="Frameworks"', + 'UNITY_LICENSING_CLIENT_SUBDIR="Helpers"', + '^6000\\.([3-9]|[1-9][0-9])', + 'https://docs.unity.com/en-us/licensing-server/client-config', + '"$UNITY_EDITOR_DIR/Contents/$UNITY_LICENSING_CLIENT_SUBDIR/UnityLicensingClient.app/Contents/MacOS/Unity.Licensing.Client"', + ]; + describe('mac licensing scripts', () => { - it('activate script switches Unity Licensing Client path for Unity 6000.3+', async () => { - const activateScriptPath = `${process.cwd()}/dist/platforms/mac/steps/activate.sh`; - const activateScript = await readFile(activateScriptPath, 'utf8'); - - expect(activateScript).toContain('UNITY_LICENSING_CLIENT_SUBDIR="Frameworks"'); - expect(activateScript).toContain('UNITY_LICENSING_CLIENT_SUBDIR="Helpers"'); - expect(activateScript).toContain('^6000\\.([3-9]|[1-9][0-9])'); - expect(activateScript).toContain('https://docs.unity.com/en-us/licensing-server/client-config'); - expect(activateScript).toContain( - '"$UNITY_EDITOR_DIR/Contents/$UNITY_LICENSING_CLIENT_SUBDIR/UnityLicensingClient.app/Contents/MacOS/Unity.Licensing.Client"', - ); - }); - - it('return license script uses the same Unity version-gated path logic', async () => { - const returnLicenseScriptPath = `${process.cwd()}/dist/platforms/mac/steps/return_license.sh`; - const returnLicenseScript = await readFile(returnLicenseScriptPath, 'utf8'); - - expect(returnLicenseScript).toContain('UNITY_LICENSING_CLIENT_SUBDIR="Frameworks"'); - expect(returnLicenseScript).toContain('UNITY_LICENSING_CLIENT_SUBDIR="Helpers"'); - expect(returnLicenseScript).toContain('^6000\\.([3-9]|[1-9][0-9])'); - expect(returnLicenseScript).toContain('https://docs.unity.com/en-us/licensing-server/client-config'); - expect(returnLicenseScript).toContain( - '"$UNITY_EDITOR_DIR/Contents/$UNITY_LICENSING_CLIENT_SUBDIR/UnityLicensingClient.app/Contents/MacOS/Unity.Licensing.Client"', - ); - }); + it.each([ + ['activate.sh', 'dist/platforms/mac/steps/activate.sh'], + ['return_license.sh', 'dist/platforms/mac/steps/return_license.sh'], + ])('%s uses the Unity version-gated licensing client path logic', async (_name, relPath) => { + const script = await readFile(`${process.cwd()}/${relPath}`, 'utf8'); + for (const snippet of expectedLicensingClientSnippets) { + expect(script).toContain(snippet); + } + }); });🤖 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/integrity.test.ts` around lines 13 - 37, The two integrity tests in src/integrity.test.ts duplicate the same five substring assertions for activateScript and returnLicenseScript. Refactor the repeated assertion set into a small shared helper or data-driven loop, and call it from both test cases while keeping the differing script path reads in place. Use the existing test names and variables activateScript and returnLicenseScript to locate the duplicated logic.
12-38: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffTests only verify string presence, not that the version-gated branch actually selects the right path.
Both tests assert that
UNITY_LICENSING_CLIENT_SUBDIR="Frameworks",UNITY_LICENSING_CLIENT_SUBDIR="Helpers", the version regex, and the path-construction template all appear somewhere in the script text. This would still pass even if the conditional logic connecting the version check to the subdir selection were broken (e.g., always falling through to"Frameworks", or the regex never matching), since the strings exist in the file regardless of which branch is taken.Given the PR's stated goal of covering "path-selection branches," consider actually executing the built script (e.g., via
child_process.execFilewith a stubbed$UNITY_EDITOR_DIR/mocked Unity version input) and asserting on the resolved path, rather than just grepping literal substrings.🤖 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/integrity.test.ts` around lines 12 - 38, The tests in integrity.test.ts only check that literal strings exist in activateScript and returnLicenseScript, so they do not prove the version-gated path-selection branch works. Update the mac licensing script tests to exercise the actual branching logic in the built scripts, using the existing activateScriptPath and returnLicenseScriptPath checks as a guide. Execute the scripts with controlled inputs (for example a stubbed UNITY_EDITOR_DIR and a mock Unity version) and assert the resolved UnityLicensingClient path for both sides of the version gate, instead of only grepping for substrings.
🤖 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.
Nitpick comments:
In `@src/integrity.test.ts`:
- Around line 2-3: `src/integrity.test.ts` has duplicate imports from
`node:fs/promises`; merge `readFile` into the existing import that already
brings in `stat` instead of keeping a separate statement. Update the top-level
import block in `integrity.test.ts` so both `stat` and `readFile` come from the
same `node:fs/promises` import.
- Around line 13-37: The two integrity tests in src/integrity.test.ts duplicate
the same five substring assertions for activateScript and returnLicenseScript.
Refactor the repeated assertion set into a small shared helper or data-driven
loop, and call it from both test cases while keeping the differing script path
reads in place. Use the existing test names and variables activateScript and
returnLicenseScript to locate the duplicated logic.
- Around line 12-38: The tests in integrity.test.ts only check that literal
strings exist in activateScript and returnLicenseScript, so they do not prove
the version-gated path-selection branch works. Update the mac licensing script
tests to exercise the actual branching logic in the built scripts, using the
existing activateScriptPath and returnLicenseScriptPath checks as a guide.
Execute the scripts with controlled inputs (for example a stubbed
UNITY_EDITOR_DIR and a mock Unity version) and assert the resolved
UnityLicensingClient path for both sides of the version gate, instead of only
grepping for substrings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1213bdb-6a38-4aee-b0ea-9a6660321a82
⛔ Files ignored due to path filters (2)
dist/platforms/mac/steps/activate.shis excluded by!**/dist/**dist/platforms/mac/steps/return_license.shis excluded by!**/dist/**
📒 Files selected for processing (1)
src/integrity.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/integrity.test.ts (3)
94-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo error context captured on script failure.
If the patched script exits non-zero,
execFileAsyncthrows anExecExceptionwhosestdout/stderrare attached but not surfaced anywhere, and the subsequentreadFile(capturePathFile, ...)line is never reached. Debugging a failing integrity test would then require re-running locally rather than reading CI output.♻️ Optional improvement to surface stdout/stderr
- await execFileAsync('bash', [localScriptPath], { env: environment }); - - return (await readFile(capturePathFile, 'utf8')).trim(); + try { + await execFileAsync('bash', [localScriptPath], { env: environment }); + } catch (error) { + const { stdout, stderr } = error as { stdout?: string; stderr?: string }; + throw new Error(`Script execution failed.\nstdout: ${stdout}\nstderr: ${stderr}`, { cause: error }); + } + + return (await readFile(capturePathFile, 'utf8')).trim();🤖 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/integrity.test.ts` around lines 94 - 96, The integrity test helper currently lets execFileAsync fail without surfacing the ExecException stdout/stderr, so failures lack context. Update the helper in integrity.test.ts around execFileAsync and readFile(capturePathFile, ...) to catch script execution errors, include the attached stdout/stderr in the thrown/asserted failure message, and then rethrow so the test output is actionable; use the existing localScriptPath and capturePathFile flow to place the fix.
72-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBrittle string-replace patch for
UNITY_EDITOR_DIR.The patch relies on an exact literal match of
UNITY_EDITOR_DIR="/Applications/Unity/Hub/Editor/$UNITY_VERSION/Unity.app"in the built script. If the source script's quoting/spacing/variable name changes slightly,.replacesilently no-ops, the script then tries to use the real/Applications/...path in the sandbox, and the failure mode (execFileAsyncthrowing on a nonexistent path, or the assertion just failing) won't clearly indicate the root cause is a stale test fixture rather than a script bug.Consider asserting that the replacement actually took effect (e.g.,
if (patchedScript === scriptSource) throw new Error(...)) so a mismatch fails fast with an informative message.
57-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp directories created via
mkdtempare never cleaned up.Each call to
executeAndCaptureResolvedClientPathcreates a fresh temp workspace underos.tmpdir()(mock editor tree, capture file, patched script) but nothing ever removesroot.expectVersionGateBehaviorcalls this twice per test, across two test cases, leaving stray directories on disk after every test run (worse in CI where tmp isn't always cleared between jobs).♻️ Proposed cleanup
+import { rm } from 'node:fs/promises'; + async function executeAndCaptureResolvedClientPath( sourceScriptPath: string, unityVersion: string, kind: ScriptKind, ): Promise<string> { const root = await mkdtemp(join(tmpdir(), 'unity-builder-integrity-')); + try { ... - await execFileAsync('bash', [localScriptPath], { env: environment }); - - return (await readFile(capturePathFile, 'utf8')).trim(); + await execFileAsync('bash', [localScriptPath], { env: environment }); + return (await readFile(capturePathFile, 'utf8')).trim(); + } finally { + await rm(root, { recursive: true, force: true }); + } }🤖 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/integrity.test.ts` around lines 57 - 97, The temp workspace created in executeAndCaptureResolvedClientPath is never removed, so each mkdtemp call leaves behind the mock editor, scripts, and capture files under the temp root. Add cleanup for the root directory after the bash exec/read completes, and make sure expectVersionGateBehavior still works when executeAndCaptureResolvedClientPath is called multiple times by using a try/finally around the existing mkdtemp/mkdir/writeFile/execFileAsync flow.
🤖 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.
Nitpick comments:
In `@src/integrity.test.ts`:
- Around line 94-96: The integrity test helper currently lets execFileAsync fail
without surfacing the ExecException stdout/stderr, so failures lack context.
Update the helper in integrity.test.ts around execFileAsync and
readFile(capturePathFile, ...) to catch script execution errors, include the
attached stdout/stderr in the thrown/asserted failure message, and then rethrow
so the test output is actionable; use the existing localScriptPath and
capturePathFile flow to place the fix.
- Around line 57-97: The temp workspace created in
executeAndCaptureResolvedClientPath is never removed, so each mkdtemp call
leaves behind the mock editor, scripts, and capture files under the temp root.
Add cleanup for the root directory after the bash exec/read completes, and make
sure expectVersionGateBehavior still works when
executeAndCaptureResolvedClientPath is called multiple times by using a
try/finally around the existing mkdtemp/mkdir/writeFile/execFileAsync flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c30ee87-5ad5-41f5-a80e-bb17a1edfb6e
📒 Files selected for processing (1)
src/integrity.test.ts
1 participant
Changes
The paths for activation in the activate and return_license scripts were hardcoded to:
But for Unity 6.3+, the /Frameworks/ substring changed to /Helpers/ instead. This PR detects the Unity version and then changes the Licensing Client path depending on that.
game-ci/unity-builderon Unity Editor versions of 6.3+ on macs (e.g. used for iOS builds)Related Issues
Related PRs
Successful Workflow Run Link
PRs don't have access to secrets so you will need to provide a link to a successful run of the workflows from your own
repo.
BEFORE
🔴 Error when trying to build with
game-ci/unity-builderon Unity 6.3, build target iOSAFTER
Floating license was properly acquired
License properly returned
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit
package-lock.jsonis not present.