feat: add buildProfile parameter
#685
Conversation
|
|
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Relates to game-ci/unity-builder#674 Relates to game-ci/unity-builder#685
MichaelBuhler
force-pushed
the
build-profile
branch
from
2acba7c to
21e3152
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/model/input.test.ts (1)
62-73: LGTM! Consider adding edge case tests.The test suite follows the established pattern and properly verifies both default value and user input scenarios.
Consider adding the following test cases to improve coverage:
- Invalid file paths (e.g., paths with invalid characters)
- Relative vs absolute paths
- File extension validation (if required)
Example:
it('handles invalid file paths gracefully', () => { const mockValue = 'invalid/*/path'; const spy = jest.spyOn(core, 'getInput').mockReturnValue(mockValue); expect(Input.buildProfile).toStrictEqual(mockValue); expect(spy).toHaveBeenCalledTimes(1); }); it('handles absolute paths', () => { const mockValue = '/absolute/path/to/build_profile.asset'; const spy = jest.spyOn(core, 'getInput').mockReturnValue(mockValue); expect(Input.buildProfile).toStrictEqual(mockValue); expect(spy).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/platforms/mac/steps/build.shis excluded by!**/dist/**dist/platforms/ubuntu/steps/build.shis excluded by!**/dist/**dist/platforms/windows/build.ps1is excluded by!**/dist/**
📒 Files selected for processing (7)
action.yml(1 hunks)src/model/build-parameters.test.ts(1 hunks)src/model/build-parameters.ts(2 hunks)src/model/image-environment-factory.ts(1 hunks)src/model/input.test.ts(1 hunks)src/model/input.ts(1 hunks)src/model/platform-setup/setup-mac.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/model/platform-setup/setup-mac.ts
- src/model/build-parameters.ts
- src/model/input.ts
- src/model/build-parameters.test.ts
- action.yml
- src/model/image-environment-factory.ts
add new `buildProfile` action param, which will be passed into Unity as the `-activeBuildProfile ...` CLI param. closes game-ci#674
MichaelBuhler
force-pushed
the
build-profile
branch
from
222b806 to
3a87506
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/model/input.test.ts (1)
62-74: LGTM! Consider adding validation test cases.The test suite follows the established patterns and correctly verifies both default value and user input scenarios. The implementation is clean and consistent with other test suites in the file.
Consider adding test cases to verify:
- Path validation (if any)
- Handling of invalid paths
- Path normalization (if implemented)
Example test case structure:
it('handles invalid path characters', () => { const mockValue = 'path\\with\\invalid/mixed/separators'; const spy = jest.spyOn(core, 'getInput').mockReturnValue(mockValue); expect(Input.buildProfile).toStrictEqual(/* expected normalized path */); expect(spy).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/default-build-script/Assets/Editor/UnityBuilderAction/Builder.csis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/platforms/mac/steps/build.shis excluded by!**/dist/**dist/platforms/ubuntu/steps/build.shis excluded by!**/dist/**dist/platforms/windows/build.ps1is excluded by!**/dist/**
📒 Files selected for processing (7)
action.yml(1 hunks)src/model/build-parameters.test.ts(1 hunks)src/model/build-parameters.ts(2 hunks)src/model/image-environment-factory.ts(1 hunks)src/model/input.test.ts(1 hunks)src/model/input.ts(1 hunks)src/model/platform-setup/setup-mac.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/model/platform-setup/setup-mac.ts
- src/model/build-parameters.ts
- src/model/build-parameters.test.ts
- src/model/image-environment-factory.ts
- src/model/input.ts
- action.yml
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build-tests-ubuntu.yml (1)
94-99: Consider adding test cases for edge cases in the buildProfile parameter.While the current test matrix includes basic cases for the new buildProfile parameter, consider adding test cases for:
- Invalid build profile paths
- Build profiles with special characters
- Missing build profile files
This would help ensure robust error handling in the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test-project/Assets/Settings/Build Profiles/Sample WebGL Build Profile.assetis excluded by!**/*.asset
📒 Files selected for processing (4)
.github/workflows/build-tests-ubuntu.yml(1 hunks)test-project/Assets/Settings.meta(1 hunks)test-project/Assets/Settings/Build Profiles.meta(1 hunks)test-project/Assets/Settings/Build Profiles/Sample WebGL Build Profile.asset.meta(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test-project/Assets/Settings/Build Profiles.meta
- test-project/Assets/Settings.meta
- test-project/Assets/Settings/Build Profiles/Sample WebGL Build Profile.asset.meta
🔇 Additional comments (3)
.github/workflows/build-tests-ubuntu.yml (3)
94-99: LGTM! The matrix configuration properly tests the new buildProfile parameter.The changes effectively test the new buildProfile parameter with both default and custom profile configurations for WebGL builds.
94-99: Verify matrix exclusions for new configurations.The new WebGL configurations with Unity version 6000.0.36f1 should be explicitly excluded from incompatible combinations in the
excludesection, similar to other WebGL configurations:
- Exclude il2cpp builds (already covered by existing exclusion)
- Exclude server builds (already covered by existing exclusion)
✅ Verification successful
Existing matrix exclusions are sufficient ✅
The current matrix configuration already handles WebGL builds correctly through platform-based exclusions that apply to all Unity versions:
- WebGL builds are excluded from il2cpp builds
- WebGL builds are excluded from server builds
No additional exclusions are needed for Unity version 6000.0.36f1 as the constraints are version-independent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new configurations are properly excluded # by checking the effective matrix after GitHub Actions processes it gh workflow view "build-tests-ubuntu.yml" --json jobs -q '.jobs[].strategy.matrix' || echo "Unable to fetch effective matrix configuration"Length of output: 508
Script:
#!/bin/bash # Get the complete matrix configuration including exclude patterns cat .github/workflows/build-tests-ubuntu.yml | grep -A 50 "matrix:"Length of output: 2086
95-96: Verify the Unity version number format.The Unity version
6000.0.36f1appears to be non-standard. Unity versions typically follow the formatYYYY.N.PfX(e.g., 2023.2.2f1). Is this a test version or should it be updated to match a real Unity version?✅ Verification successful
The Unity version
6000.0.36f1is intentionally used for testing purposes.The version follows an established testing pattern in the codebase, where year 6000 is used in test files for version parsing and feature validation. This is consistent with similar test versions found in
unity-versioning.test.tsandimage-tag.test.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if this is a valid Unity version # Note: This script helps verify if the version exists in Unity's download archive curl -s "https://unity.com/releases/editor/archive" | grep -o "Unity [0-9]\{4\}\.[0-9]\+\.[0-9]\+[[:alpha:]][0-9]\+" || echo "Version not found in Unity's archive"Length of output: 202
Script:
#!/bin/bash # Check for test documentation and similar version patterns echo "=== Checking workflow file ===" cat .github/workflows/build-tests-ubuntu.yml echo -e "\n=== Checking for test documentation ===" fd -e md | xargs grep -l "test" || true echo -e "\n=== Checking for similar version patterns ===" rg "6000\.0\." || trueLength of output: 9871
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #685 +/- ##
==========================================
+ Coverage 37.00% 37.04% +0.04%
==========================================
Files 77 77
Lines 3129 3131 +2
Branches 610 647 +37
==========================================
+ Hits 1158 1160 +2
+ Misses 1971 1828 -143
- Partials 0 143 +143
|
There was a problem hiding this comment.
LGTM
Could use a second pair of eyes from @davidmfinol, @GabLeRoux and/or @AndrewKahr
| if [ -z "$BUILD_PROFILE" ]; then | ||
| # User has not provided a build profile | ||
| # | ||
| echo "Doing a default \"$BUILD_TARGET\" platform build." |
There was a problem hiding this comment.
This is already said on line 20.
Probably better to only add the echo from when $BUILD_PROFILE is set.
| echo "Doing a default \"$BUILD_TARGET\" platform build." | ||
| # | ||
| else | ||
| # User has provided a path to a build profile `.asset` file | ||
| # | ||
| echo "Using build profile \"$BUILD_PROFILE\" relative to \"$UNITY_PROJECT_PATH\"." |
| Write-Output "$('Using build profile "')$($Env:BUILD_PROFILE)$('" relative to "')$($Env:UNITY_PROJECT_PATH)$('".')" | ||
| # | ||
| } | ||
| else | ||
| { | ||
| # User has not provided a build profile | ||
| # | ||
| Write-Output "$('Doing a default "')$($Env:BUILD_TARGET)$('" platform build.')" |
| describe('buildProfile', () => { | ||
| it('returns the default value', () => { | ||
| expect(Input.buildProfile).toStrictEqual(''); | ||
| }); | ||
|
|
||
| it('takes input from the users workflow', () => { | ||
| const mockValue = 'path/to/build_profile.asset'; | ||
| const spy = jest.spyOn(core, 'getInput').mockReturnValue(mockValue); | ||
| expect(Input.buildProfile).toStrictEqual(mockValue); | ||
| expect(spy).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test gods thank you for your contribution
|
Are we able to do a manual dispatch of the Builds - Ubuntu workflow using this PR branch? |
|
@MichaelBuhler Unfortunately not, but I can give you repo access, so that you can push your branch to upstream, in which case it can be achieved and you could run these by yourself. Invite pending. |
| #if UNITY_6000_0_OR_NEWER | ||
| using UnityEditor.Build.Profile; | ||
| #endif |
There was a problem hiding this comment.
Should we upgrade the dist/default-build-script project to Unity 6 too? I don't think it would affect anything, but it would make opening Unity in that folder easier to confirm that feature and Unity would complain less. 🤔
The Unity complaints 😛
There was a problem hiding this comment.
Yea, that'd probably be a great idea!
There was a problem hiding this comment.
Unless I am mistaken, the dist/default-build-script is never opened in Unity. Rather, that folder is bind mounted onto the Docker container, and the contents of dist/default-build-script/Assets/Editor are copied into the users's Unity project, then compiled by Unity when their project launches.
There was a problem hiding this comment.
^ For this reason, I think the code in dist/default-build-script must be backwards compatible using scripting defines: so that the default-build-script can be built by older versions of Unity.
Such as most of these jobs: https://github.com/game-ci/unity-builder/actions/runs/13057500403
There was a problem hiding this comment.
dist/default-build-scriptis never opened in Unity.
That’s correct for script usage, but opening the file in JetBrains Rider with Unity integration makes sense for development.
Rather, that folder is bind mounted onto the Docker container, and the contents of dist/default-build-script/Assets/Editor are copied into the users's Unity project, then compiled by Unity when their project launches.
✅ That is correct
I think the code in dist/default-build-script must be backwards compatible using scripting defines
I definitely agree that the code in dist/default-build-script should remain backwards compatible using scripting defines.
I can confirm the folder itself is a Unity project.
The directory structure of `dist/default-build-script`
.
├── Assets
│ ├── Editor
│ │ ├── UnityBuilderAction
│ │ │ ├── Builder.cs
│ │ │ ├── Builder.cs.meta
│ │ │ ├── Input
│ │ │ │ ├── AndroidSettings.cs
│ │ │ │ ├── AndroidSettings.cs.meta
│ │ │ │ ├── ArgumentsParser.cs
│ │ │ │ └── ArgumentsParser.cs.meta
│ │ │ ├── Input.meta
│ │ │ ├── Reporting
│ │ │ │ ├── CompileListener.cs
│ │ │ │ ├── CompileListener.cs.meta
│ │ │ │ ├── StdOutReporter.cs
│ │ │ │ └── StdOutReporter.cs.meta
│ │ │ ├── Reporting.meta
│ │ │ ├── System
│ │ │ │ ├── ProcessExtensions.cs
│ │ │ │ └── ProcessExtensions.cs.meta
│ │ │ ├── System.meta
│ │ │ ├── UnityBuilderAction.asmdef
│ │ │ ├── UnityBuilderAction.asmdef.meta
│ │ │ ├── Versioning
│ │ │ │ ├── Git.cs
│ │ │ │ ├── Git.cs.meta
│ │ │ │ ├── GitException.cs
│ │ │ │ ├── GitException.cs.meta
│ │ │ │ ├── VersionApplicator.cs
│ │ │ │ ├── VersionApplicator.cs.meta
│ │ │ │ ├── VersionGenerator.cs
│ │ │ │ └── VersionGenerator.cs.meta
│ │ │ └── Versioning.meta
│ │ └── UnityBuilderAction.meta
│ └── Editor.meta
├── Packages
│ └── manifest.json
└── ProjectSettings
├── AudioManager.asset
├── ClusterInputManager.asset
├── DynamicsManager.asset
├── EditorBuildSettings.asset
├── EditorSettings.asset
├── GraphicsSettings.asset
├── InputManager.asset
├── NavMeshAreas.asset
├── Physics2DSettings.asset
├── PresetManager.asset
├── ProjectSettings.asset
├── ProjectVersion.txt
├── QualitySettings.asset
├── TagManager.asset
├── TimeManager.asset
├── UnityConnectSettings.asset
└── VFXManager.asset
10 directories, 46 files
My suggestion is more about files like ProjectSettings/ProjectVersion.txt, updating it wouldn’t change much, but it could improve development workflow. However, since the alternative is creating a blank Unity project and manually copying files (which mirrors the action’s behavior), that approach remains valid too.
The main goal was to open the project in Unity/Rider to check for issues. I'm just lacking an older version on my system. I’m still unsure if we should upgrade the project in that folder, it likely wouldn’t make much difference, but might make sense for development. :) 🤷
There was a problem hiding this comment.
@GabLeRoux Ah, I get what you are saying. That's actually what I did: I copied dist/default-build-script/Assets/Editor into my Unity 6 project, drafted/tested it, and copied it back into this repo.
|
@webbertakken Okay, great. ❤️ Here is a success workflow job with a defined https://github.com/game-ci/unity-builder/actions/runs/13057500403/job/36432150028#step:6:4 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/build-tests-ubuntu.yml (1)
204-204: Simplify and standardize artifact naming.The artifact name is overly complex and contains redundant conditional logic. Long artifact names might cause issues with file system limitations on some platforms.
Consider simplifying the artifact name:
-"Build ${{ matrix.targetPlatform }}${{ startsWith(matrix.buildProfile, 'Assets') && ' (via Build Profile)' || '' }} on Ubuntu (${{ matrix.unityVersion }}_il2cpp_${{ matrix.buildWithIl2cpp }}_params_${{ matrix.additionalParameters }})" +"${{ matrix.targetPlatform }}-${{ matrix.unityVersion }}${{ matrix.buildProfile && '-profile' || '' }}"This shorter format:
- Remains informative while being more concise
- Uses hyphen separators for better readability
- Avoids potential path length issues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-tests-ubuntu.yml(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
|
|
||
| // Apply Android settings | ||
| if (buildTarget == BuildTarget.Android) { | ||
| VersionApplicator.SetAndroidVersionCode(options["androidVersionCode"]); |
There was a problem hiding this comment.
Can we still set the androidVersionCode when using a build profile for Android?
There was a problem hiding this comment.
This is a complex question, which applies not only to the version code, but also to all other Android settings supported by this unity-builder GitHub action.
You can set the Android Bundle Version Code within the Build Profile itself. In this case I suppose that you would run the GitHub action with buildProfile: ... and versioning: None.
However, if you want to rely on the GitHub action to determine the version code... I think I have not implemented that--actually, it might even be impossible:
After we load the BuildProfile from the /Assets folder, the object has no public members or methods by which we can modify/edit the Build Profile at runtime.
From decompiled code, I can see that the BuildProfile class has internal classes and getter/setters, as well as private members, that hold the platform build settings and player overrides deserialized from the build profile asset. There also seems to be no public constructor, so you cannot instantiate/build the build profile at runtime.
I would guess that Unity will add these features/abilities in a later version, but they do not seem to be in Unity 6.1 Beta, yet.
So, I suppose the only possible solution today would be to modify (edit in place) the build profile's .asset YAML before launching the build. For instance, I just made a new Android Build Profile and set Bundle Version Code: 990099:
This resulted in this on line 206 of the .asset file:
- line: '| AndroidBundleVersionCode: 990099'So we would have to Regex replace that 6-digit value before launching Unity.
There was a problem hiding this comment.
Ok, we may want to create a follow-up issue to check on setting Android version code when using a build profile, but I don't think that should be a blocker for this PR.
|
|
||
| #if UNITY_2021_2_OR_NEWER | ||
| // Determine subtarget | ||
| StandaloneBuildSubtarget buildSubtarget; |
There was a problem hiding this comment.
Do build profiles set StandaloneBuildSubtarget?
There was a problem hiding this comment.
It's appears to me that the Build Profile feature eliminates the need for defining the build subtarget.
The old build method works by setting BuildPlayerOptions.target in conjunction with BuildPlayerOptions.subtarget. However these fields do not exist on the new BuildPlayerWithProfileOptions.
Rather, Windows Server, macOS Server, and Linux Server are entirely different types of build profiles:
There was a problem hiding this comment.
Ok, then we should be fine to ignore subtarget when using a build profile. We may just want to add a comment about it to the docs.
Relates to game-ci/unity-builder#674 Relates to game-ci/unity-builder#685
add new
buildProfileaction param, which will be passed into Unity as-customBuildProfile ...on the CLI. The build script will detect this and, if the Unity version supports build profiles, will run the build pipeline using the requested build profile.closes #674
Changes
buildProfileGitHub action parameter to support Unity's new Build Profiles.Related Issues
Related PRs
buildProfileconfig option documentation#498Successful Workflow Run Link
Checklist
buildProfileconfig option documentation#498Summary by CodeRabbit
Release Notes
New Features
buildProfileinput parameter to enhance build configuration flexibility.buildProfilein the Input class to improve input handling.Documentation
projectPathinput description to specify path relative to repository root.Tests
buildProfileproperty across relevant classes.