Add parameter linux64RemoveExecutableExtension#726
Add parameter linux64RemoveExecutableExtension#726kitlith wants to merge 3 commits intogame-ci:mainfrom
linux64RemoveExecutableExtension#726Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughA new boolean input parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/model/input.test.ts (1)
338-349: Consider adding a fallback-env test to exercise non-core input paths.This strengthens confidence that LINUX64_FILE_EXTENSION (env-var) works when core.getInput is empty.
You could append something like this:
it('falls back to env var LINUX64_FILE_EXTENSION when core input is empty', () => { const original = process.env.LINUX64_FILE_EXTENSION; try { process.env.LINUX64_FILE_EXTENSION = '.x86_64'; jest.spyOn(core, 'getInput').mockReturnValue(''); // force fallback path expect(Input.linux64FileExtension).toBe('.x86_64'); } finally { if (original === undefined) { delete process.env.LINUX64_FILE_EXTENSION; } else { process.env.LINUX64_FILE_EXTENSION = original; } } });src/model/build-parameters.test.ts (1)
107-115: Use empty string instead of 'n/a' for non-Linux rows to avoid unrealistic values.The property is a plain string and unused outside the Linux branch, but keeping it '' makes the test matrix semantically cleaner.
Apply:
- ${Platform.types.Android} | ${'.apk'} | ${'androidPackage'} | ${'n/a'} - ${Platform.types.Android} | ${'.aab'} | ${'androidAppBundle'} | ${'n/a'} - ${Platform.types.Android} | ${''} | ${'androidStudioProject'} | ${'n/a'} - ${Platform.types.StandaloneWindows} | ${'.exe'} | ${'n/a'} | ${'n/a'} - ${Platform.types.StandaloneWindows64} | ${'.exe'} | ${'n/a'} | ${'n/a'} + ${Platform.types.Android} | ${'.apk'} | ${'androidPackage'} | ${''} + ${Platform.types.Android} | ${'.aab'} | ${'androidAppBundle'} | ${''} + ${Platform.types.Android} | ${''} | ${'androidStudioProject'} | ${''} + ${Platform.types.StandaloneWindows} | ${'.exe'} | ${'n/a'} | ${''} + ${Platform.types.StandaloneWindows64} | ${'.exe'} | ${'n/a'} | ${''}src/model/build-parameters.ts (2)
256-259: Normalize and validate linux64FileExtension to avoid surprises (leading dot, path separators).Prevents accidental “filenamex86_64” (missing dot) or injection of path separators. Non-breaking for current tests.
Apply:
- if (platform === Platform.types.StandaloneLinux64) { - return `${filename}${linux64FileExtension}`; - } + if (platform === Platform.types.StandaloneLinux64) { + const trimmed = linux64FileExtension.trim(); + if (/[\/\\]/.test(trimmed)) { + throw new Error('Invalid linux64FileExtension: must not contain path separators'); + } + const normalized = trimmed === '' ? '' : (trimmed.startsWith('.') ? trimmed : `.${trimmed}`); + return `${filename}${normalized}`; + }
256-259: Add a Linux64 predicate to Platform (optional)There isn’t currently a
Platform.isLinux64(orPlatform.isLinux) helper insrc/model/platform.ts. To stay consistent with the existingPlatform.isWindowsandPlatform.isAndroidpredicates—and for easier future extensions—you might add one:In
src/model/platform.ts:static isLinux64(platform: string): boolean { return platform === Platform.types.StandaloneLinux64; }Then in
src/model/build-parameters.ts(around line 256):- if (platform === Platform.types.StandaloneLinux64) { + if (Platform.isLinux64(platform)) { return `${filename}${linux64FileExtension}`; }This refactor is optional but will keep your platform checks uniform.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
src/model/build-parameters.test.ts(1 hunks)src/model/build-parameters.ts(3 hunks)src/model/input.test.ts(1 hunks)src/model/input.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/model/build-parameters.test.ts (1)
src/model/input.ts (3)
targetPlatform(80-82)androidExportType(162-164)linux64FileExtension(285-287)
src/model/build-parameters.ts (1)
src/model/input.ts (2)
androidExportType(162-164)linux64FileExtension(285-287)
🔇 Additional comments (4)
src/model/input.ts (1)
285-288: LGTM: new linux64FileExtension input is correctly exposed and defaults to empty string.Matches existing Input getter patterns and preserves current behavior by default.
src/model/input.test.ts (1)
338-349: LGTM: tests cover default and explicit values for linux64FileExtension.Clear, minimal, and consistent with other Input getter tests.
src/model/build-parameters.test.ts (1)
106-127: LGTM: matrix now validates Linux64 filename behavior with and without extension.Good coverage tying Input.linux64FileExtension into BuildParameters.create and parse logic.
src/model/build-parameters.ts (1)
104-109: Correctly threading linux64FileExtension into parseBuildFile.This keeps the extension decision centralized in parseBuildFile and preserves default behavior when the input is empty.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
action.yml (2)
271-272: Clarify accepted formats in the description (leading dot vs no dot).Minor wording tweak to preempt ambiguity about whether users should include the leading dot. If your parsing already normalizes both, reflect that here; if not, this makes the expectation explicit.
Apply this diff to clarify:
- description: - 'Specify the file extension of the executable when building for StandaloneLinux64. For example, ".x86_64"' + description: + 'Specify the file extension of the executable when building for StandaloneLinux64. For example, ".x86_64". Leave empty to omit. You may include or omit the leading dot; the builder will handle both.'
268-273: Remember to update docs and usage examples.Once merged, add this input to README/action usage matrix and provide a snippet for enabling Unity’s default “.x86_64” extension.
Happy to draft the documentation snippet and a minimal example workflow if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
action.yml(1 hunks)
🔇 Additional comments (1)
action.yml (1)
268-273: LGTM: New linux64FileExtension input is well-scoped and consistent.Name, placement, and defaults align with existing inputs. Description clearly communicates the intent and example.
kitlith
changed the title
linux64FileExtensionlinux64RemoveExecutableExtension
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/model/input.ts (1)
285-289: Normalize boolean parsing to be case/whitespace-insensitiveUsers sometimes pass True/TRUE or include spaces in workflow YAML. Normalizing here avoids subtle surprises and mirrors a more robust pattern.
static get linux64RemoveExecutableExtension(): boolean { - const input = Input.getInput('linux64RemoveExecutableExtension') ?? 'true'; - - return input === 'true'; + const raw = (Input.getInput('linux64RemoveExecutableExtension') ?? 'true').trim().toLowerCase(); + return raw === 'true'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (5)
action.yml(1 hunks)src/model/build-parameters.test.ts(1 hunks)src/model/build-parameters.ts(3 hunks)src/model/input.test.ts(1 hunks)src/model/input.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/model/input.test.ts
- action.yml
- src/model/build-parameters.ts
- src/model/build-parameters.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/model/input.ts (1)
src/model/index.ts (1)
Input(19-19)
🔇 Additional comments (2)
src/model/input.ts (2)
285-289: LGTM: adds boolean input with backward-compatible defaultThe getter correctly defaults to removing the extension (true), which preserves current behavior and avoids breaking changes.
285-289: No lingering linux64FileExtension — repo uses linux64RemoveExecutableExtensionSearch results show no occurrences of linux64FileExtension; the code, tests and action metadata use linux64RemoveExecutableExtension:
- action.yml — linux64RemoveExecutableExtension (lines ~268–270)
- src/model/input.ts — getter linux64RemoveExecutableExtension (lines ~285–289)
- src/model/input.test.ts — tests for linux64RemoveExecutableExtension (around lines ~338–351)
- src/model/build-parameters.ts & src/model/build-parameters.test.ts — consume/mock linux64RemoveExecutableExtension
- dist/index.js — compiled references to linux64RemoveExecutableExtension
Conclusion: no changes required in code/docs to remove an old name — you can consider updating the PR title (if it still says linux64FileExtension) to match linux64RemoveExecutableExtension.
|
making a couple of the AI nitpicks more visible:
Felt redundant to me, then again
Feels like this would be better off in a separate PR that normalizes the boolean parsing everywhere. As-is, I'm using the same pattern that is used everywhere else in the file. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
.github/workflows/build-tests-mac.yml(1 hunks).github/workflows/build-tests-windows.yml(2 hunks)
| name: | ||
| Build ${{ matrix.targetPlatform }} on MacOS (${{ matrix.unityVersion }})${{ matrix.buildProfile && ' With | ||
| Build Profile' || '' }} |
There was a problem hiding this comment.
Prevent newline insertion in artifact-name literal.
The line break inside ' With … ' Build Profile' injects a newline into the evaluated string, yielding a multi-line artifact name when buildProfile is set. Keep the literal on one line (or use a folded scalar) so the name stays single-line.
- with:
- name:
- Build ${{ matrix.targetPlatform }} on MacOS (${{ matrix.unityVersion }})${{ matrix.buildProfile && ' With
- Build Profile' || '' }}
+ with:
+ name: >-
+ Build ${{ matrix.targetPlatform }} on MacOS (${{ matrix.unityVersion }})${{ matrix.buildProfile && ' With Build Profile' || '' }}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/build-tests-mac.yml around lines 85 to 87, the artifact
name literal is split across lines causing a newline to be inserted when the
buildProfile branch is included; keep the entire name expression on a single
line (or replace with a folded scalar) so the conditional `' With Build
Profile'` remains contiguous and produces a single-line artifact name; update
the YAML to remove the line break between `' With` and `Build Profile'`
ensuring the evaluated string does not contain any embedded newlines.
| name: | ||
| Build ${{ matrix.targetPlatform }} on Windows (${{ matrix.unityVersion }})${{ matrix.enableGpu && ' With | ||
| GPU' || '' }}${{ matrix.buildProfile && ' With Build Profile' || '' }} |
There was a problem hiding this comment.
Avoid embedding newlines in artifact-name expressions.
Splitting the scalar across lines puts an actual newline inside the ' With …' literal, so the artifact name becomes multi-line when enableGpu is true. Keep the literal on one line or use a folded scalar to preserve readability without introducing the newline.
- with:
- name:
- Build ${{ matrix.targetPlatform }} on Windows (${{ matrix.unityVersion }})${{ matrix.enableGpu && ' With
- GPU' || '' }}${{ matrix.buildProfile && ' With Build Profile' || '' }}
+ with:
+ name: >-
+ Build ${{ matrix.targetPlatform }} on Windows (${{ matrix.unityVersion }})${{ matrix.enableGpu && ' With GPU' || '' }}${{ matrix.buildProfile && ' With Build Profile' || '' }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: | |
| Build ${{ matrix.targetPlatform }} on Windows (${{ matrix.unityVersion }})${{ matrix.enableGpu && ' With | |
| GPU' || '' }}${{ matrix.buildProfile && ' With Build Profile' || '' }} | |
| with: | |
| name: >- | |
| Build ${{ matrix.targetPlatform }} on Windows (${{ matrix.unityVersion }})${{ matrix.enableGpu && ' With GPU' || '' }}${{ matrix.buildProfile && ' With Build Profile' || '' }} |
🤖 Prompt for AI Agents
.github/workflows/build-tests-windows.yml around lines 149 to 151: the artifact
name scalar is split across lines so the string literal for ' With GPU' contains
a newline when enableGpu is true; fix by keeping the entire name expression on a
single line (move the ' With GPU' segment up to the same line) or convert the
scalar to a folded scalar (using >) so the expression remains readable but does
not introduce a newline into the rendered artifact name.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
==========================================
+ Coverage 38.34% 38.43% +0.09%
==========================================
Files 78 78
Lines 3169 3174 +5
Branches 663 665 +2
==========================================
+ Hits 1215 1220 +5
Misses 1809 1809
Partials 145 145
🚀 New features to boost your workflow:
|
|
@frostebite Given that this has been mentioned in relation to a tracking issue for a major version bump, should I rebase and flip the default? Or is this action not involved in that version bump, and the default should stay the same? Additionally, given the length of time that this has been left open, are there any blockers, such as the missing "Successful Workflow Run Link"? Last I was told, there were no blockers. |
Allows the end-user to restore the default unity behavior of using the file extension `.x86_64`, instead of no file extension. Preserves the current behavior, to avoid surprise breakage.
and the rest that this entails. This PR should probably be squashed before merging, with attention paid to the commit message.
kitlith
force-pushed
the
linux-extension
branch
from
e2cdacd to
37698d9
Compare
2 participants
Changes
linux64RemoveExecutableExtension.x86_64, instead of no file extension.Related Issues
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.
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit
linux64RemoveExecutableExtensioninput parameter for Linux 64-bit builds. When enabled (default), removes the.x86_64file extension from executable outputs, restoring behavior from earlier versions.