unity-builder icon indicating copy to clipboard operation
unity-builder copied to clipboard

Add parameter `linux64RemoveExecutableExtension`

Open kitlith opened this issue 4 months ago • 4 comments

Changes

  • Add parameter linux64RemoveExecutableExtension
    • Allows the end-user to restore the default unity behavior of using the file extension .x86_64, instead of no file extension.
    • Defaults to true for now, but is named such to highlight it as a non-default with an eye to flip the default on the next version bump.

Related Issues

  • #722

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.

  • ...
    • is there documentation (aside from reading all the workflow files) on what secrets this repo expects?

Checklist

  • [x] Read the contribution guide and accept the code of conduct
  • [x] Docs (If new inputs or outputs have been added or changes to behavior that should be documented. Please make a PR in the documentation repo)
    • There may yet be some bikeshedding to do on the naming/precise functionality of the new input, since I never received any feedback on the initial issue. I'd like to wait until that is settled before I open a documentation PR.
  • [x] Readme (updated or not needed)
  • [x] Tests (added, updated or not needed)

Summary by CodeRabbit

  • New Features

    • Added an optional input to control removal/retention of the default Linux 64-bit executable extension; preserves prior default behavior.
  • Behavior

    • Linux 64-bit build output now reflects the chosen extension behavior.
  • Tests

    • Added tests for the new input's default and user-specified behavior and its impact on Linux 64-bit filenames.
  • Chores

    • Adjusted CI artifact naming formatting in Mac and Windows build workflows.

kitlith avatar Aug 19 '25 23:08 kitlith

Cat Gif

github-actions[bot] avatar Aug 19 '25 23:08 github-actions[bot]

📝 Walkthrough

Walkthrough

Adds a new boolean input linux64RemoveExecutableExtension, exposes it via Input.linux64RemoveExecutableExtension, threads it into BuildParameters.parseBuildFile to control adding/removing the .x86_64 extension for StandaloneLinux64, updates tests, and adjusts two CI workflow artifact-name YAML values.

Changes

Cohort / File(s) Summary of Changes
Build parameters implementation
src/model/build-parameters.ts
Added linux64RemoveExecutableExtension parameter to parseBuildFile signature and applied it for StandaloneLinux64; BuildParameters.create now passes Input.linux64RemoveExecutableExtension.
Build parameters tests
src/model/build-parameters.test.ts
Expanded test matrix to include linux64RemoveExecutableExtension cases for StandaloneLinux64; mocked Input.linux64RemoveExecutableExtension; updated test descriptions and assertions to verify extension removal vs retention.
Input implementation
src/model/input.ts
Added public static getter linux64RemoveExecutableExtension that reads the input (default 'true') and returns a boolean.
Input tests
src/model/input.test.ts
Added tests validating default true and custom value retrieval for linux64RemoveExecutableExtension, and verifying core.getInput usage.
Action metadata
action.yml
Added optional input linux64RemoveExecutableExtension (default: 'true', required: false) with description about removing the .x86_64 extension for StandaloneLinux64.
CI workflows
.github/workflows/build-tests-mac.yml, .github/workflows/build-tests-windows.yml
Changed upload-artifact step name values from single-line interpolated strings to YAML block-style multi-line values, introducing line breaks in the artifact name strings.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Action as GitHub Action
  participant Input as Input
  participant BP as BuildParameters

  User->>Action: trigger workflow
  Action->>Input: read linux64RemoveExecutableExtension
  Input-->>Action: return true / false
  Action->>BP: BuildParameters.create(filename, platform, androidExportType, linux64RemoveExecutableExtension)
  note over BP: create() forwards flag to parseBuildFile
  BP->>BP: parseBuildFile(filename, platform, androidExportType, linux64RemoveExecutableExtension)
  alt StandaloneLinux64 & linux64RemoveExecutableExtension == true
    BP-->>Action: return filename (no .x86_64)
  else StandaloneLinux64 & linux64RemoveExecutableExtension == false
    BP-->>Action: return filename + ".x86_64"
  else other platforms
    BP-->>Action: existing filename handling (unchanged)
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

  • game-ci/unity-builder#722 — Directly related: this PR introduces the linux64RemoveExecutableExtension input and implements controlling whether .x86_64 is added for StandaloneLinux64, matching the issue objective.

Suggested labels

codex

Suggested reviewers

  • webbertakken
  • davidmfinol
  • cloudymax

Poem

I hop through CI with nimble feet,
A boolean tweak makes builds complete.
For Linux targets, tidy and keen,
Extensions vanish—or stay—on screen.
A rabbit’s nudge for cleaner art, 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Add parameter linux64RemoveExecutableExtension" directly and clearly summarizes the main objective of the pull request. The changes span multiple files (action.yml, Input class, BuildParameters class, tests, and workflows), all of which center on introducing this new input parameter to allow users to control Linux 64-bit executable extension handling. The title is specific enough that someone scanning the project's git history would immediately understand the primary change being introduced.
Description Check ✅ Passed The PR description is mostly complete and addresses the core required sections. The Changes section is well-documented, explaining the new parameter's purpose and behavior with clear details about its default value and naming rationale. The Related Issues section properly references #722. The Checklist section is substantially filled with the author noting they marked relevant items as complete and explaining why the documentation PR is deferred. While the Related PRs section is entirely missing and the Successful Workflow Run Link is incomplete (marked with "..."), these are non-critical omissions; the author's comment about lacking secrets access is a reasonable explanation for the incomplete workflow link, and the Related PRs section is optional context rather than essential information for understanding the change.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Aug 19 '25 23:08 coderabbitai[bot]

making a couple of the AI nitpicks more visible:

256-259: Add a Linux64 predicate to Platform (optional)

There isn’t currently a Platform.isLinux64 (or Platform.isLinux) helper in src/model/platform.ts. To stay consistent with the existing Platform.isWindows and Platform.isAndroid predicates—and for easier future extensions—you might add one:

Felt redundant to me, then again Platform.isAndroid feels redundant to me, unlike Platform.isWindows, so shrug

285-289: Normalize boolean parsing to be case/whitespace-insensitive

Users sometimes pass True/TRUE or include spaces in workflow YAML. Normalizing here avoids subtle surprises and mirrors a more robust pattern.

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.

kitlith avatar Aug 20 '25 18:08 kitlith

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 38.43%. Comparing base (ab64768) to head (e2cdacd).

Additional details and impacted files

Impacted file tree graph

@@            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              
Files with missing lines Coverage Δ
src/model/build-parameters.ts 90.27% <100.00%> (+0.27%) :arrow_up:
src/model/input.ts 89.17% <100.00%> (+0.21%) :arrow_up:
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Oct 16 '25 15:10 codecov[bot]