-
-
Notifications
You must be signed in to change notification settings - Fork 293
Support activeBuildProfile parameter #738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (6)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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 |
dynamic buildPlayerOptions; | ||
|
||
if (options["customBuildProfile"] != "") { | ||
if (options.TryGetValue("activeBuildProfile", out var buildProfilePath) && !string.IsNullOrEmpty(buildProfilePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we go with a descriptive error, in case of null or empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commited a fix (mac/windows workflows cancelled)
$( [ -z "$BUILD_PROFILE" ] && echo "-buildTarget $BUILD_TARGET" ) \ | ||
-customBuildTarget "$BUILD_TARGET" \ | ||
-customBuildPath "$CUSTOM_BUILD_PATH" \ | ||
-customBuildProfile "$BUILD_PROFILE" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like customBuildProfile
is no longer needed here (and other platforms).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not needed/used anymore in default-build-script
, but user could be picking it up in their own build script via -executeMethod
. Example code of using the old argument exists in the previous iteration, which is public, so I decided to leave it there.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #738 +/- ##
=======================================
Coverage 38.34% 38.34%
=======================================
Files 78 78
Lines 3169 3169
Branches 663 663
=======================================
Hits 1215 1215
Misses 1809 1809
Partials 145 145 🚀 New features to boost your workflow:
|
@ryo0ka can you confirm this works? Is it in a mergeable state? |
@webbertakken yes, I did run Ubuntu workflows on the latest commit and didn't see an error |
Released in v4.6.0 |
Hello, I think this change seems break my automatic build pipeline. Build was successful before this change got merged. Environment:
Workflow job definition:
Command line arguments according to actions log:
Key error message:
In command line arguments, the value of |
I ran into the bug. Unity's default folder path seems to be "Build Profiles" annoyingly. Taking a quick look, my hunch is that the fix is to wrap the arguments in build.ps1, build.sh with the quotes as like the other variables, but I don't know much about shell/ps so someone else should confirm. E.g. "-buildTarget $BUILD_TARGET" is missing proper quotes around $BUILD_TARGET |
Ah yes, taking PRs for the quoting/escaping issue! |
@ryo0ka would you mind doing the follow-up PR? |
@webbertakken this issue is because of this:
I'll add Unity 6000+ paths to Windows/MacOS workflows which are currently missing. That should expose the quoting/escaping issue & I'll fix that too. Regarding #741, it's due to me failing to update the document. @davidmfinol has dealt with it in #520. |
Changes
buildProfile
to-activeBuildProfile
Related Issues
Successful Workflow Run Link
Checklist
in the documentation repo)
Impl details
AI analysis (not validated):
Since this is out of scope, I didn't investigate further.
-activeTarget
compatibility -- New implementation omits-activeTarget
from Unity CLI args ifbuildProfile
is passed to game-ci action. This aligns with Unity's behavior and is discussed in the linked issue. If user passedbuildProfile
to a project of older versions (2023-) by mistake, the build will likely fail.-customBuildProfile
which is picked up indefault-build-script
to set the active build profile during build process. I assume that this is working around the-activeTarget
compatibility issue. New implementation (with-activeBuildProfile
official CLI argument) loads the build profile before the build script runs, so the custom argument is not used anymore. I've left it in the project for backward compatibility.BUILD_PROFILE_LOADED
to the existing profile intest-project
, which is properly loaded to the build script indefault-build-script
, otherwise the integration test will fail.