fix(build): avoid compile-time VisionOS enum references#1113
Open
JMartinezRuiz wants to merge 2 commits intoCoplayDev:betafrom
Open
fix(build): avoid compile-time VisionOS enum references#1113JMartinezRuiz wants to merge 2 commits intoCoplayDev:betafrom
JMartinezRuiz wants to merge 2 commits intoCoplayDev:betafrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemoves compile-time guards for VisionOS in BuildTargetMapping and adds runtime parsing/helpers; centralizes unknown-target error messages in ManageBuild to use BuildTargetMapping.GetUnknownBuildTargetMessage; adds unit tests validating target resolution and VisionOS-aware errors. ChangesBuild target mapping & messaging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildTargetMappingTests.cs (1)
46-53: 💤 Low valueHappy-path branch silently passes without explicit assertion.
When both
BuildTarget.VisionOSandBuildTargetGroup.VisionOSare available,errorisnulland the innerStringAssert.Containsis skipped, so the test only verifies thatTryResolveBuildTarget("visionos", out _)returnedtrue— it never asserts thatTryResolveNamedBuildTargetitself returnednull. Adding an explicitAssert.IsNull(error)(or asserting either null or contains "VisionOS") makes the success path observable.♻️ Suggested tightening
if (visionOSAvailable) { Assert.IsTrue(BuildTargetMapping.TryResolveBuildTarget("visionos", out _)); - if (error != null) - { - StringAssert.Contains("VisionOS", error); - } + // Either fully supported (no error) or BuildTargetGroup missing (helpful error). + if (error != null) + StringAssert.Contains("VisionOS", error); + else + Assert.Pass("VisionOS BuildTarget and BuildTargetGroup both exposed."); }🤖 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 `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildTargetMappingTests.cs` around lines 46 - 53, The test's visionOS success branch only asserts TryResolveBuildTarget("visionos", out _) is true but doesn't assert that the returned error from TryResolveNamedBuildTarget is null, so add an explicit assertion to make the happy path observable: when visionOSAvailable is true, after Assert.IsTrue(BuildTargetMapping.TryResolveBuildTarget("visionos", out _)), add Assert.IsNull(error) (or Assert.IsTrue(error == null || StringAssert.Contains("VisionOS", error)) if you prefer) to validate TryResolveNamedBuildTarget's result; locate this change around the visionOSAvailable block and the variables error and BuildTargetMapping.TryResolveBuildTarget/TryResolveNamedBuildTarget.MCPForUnity/Editor/Tools/Build/BuildTargetMapping.cs (1)
30-34: 💤 Low value
Enum.TryParsefallback also accepts numeric strings and deprecated enum names.Beyond enabling VisionOS resolution, this generalized fallback now resolves any string that matches a
BuildTargetenum name case-insensitively, including obsolete targets (e.g.StandaloneOSXIntel,Stadia,Lumin) and any numeric string (e.g."5","12") sinceEnum.TryParseaccepts numeric values for the underlying integer. If the intent is solely VisionOS forward-compat, scoping the fallback would avoid accidentally accepting unsupported names; otherwise anEnum.IsDefinedcheck is sufficient to reject pure numeric inputs.♻️ Targeted alternative
default: - if (Enum.TryParse(name, true, out target)) - return true; + // Reject pure-numeric inputs that Enum.TryParse would otherwise accept. + if (!int.TryParse(name, out _) && + Enum.TryParse(name, true, out target) && + Enum.IsDefined(typeof(BuildTarget), target)) + { + return true; + } target = default; return false;Or, if only VisionOS dynamic resolution is desired:
default: - if (Enum.TryParse(name, true, out target)) - return true; + if (string.Equals(name, "visionos", StringComparison.OrdinalIgnoreCase) && + Enum.TryParse(VisionOSName, true, out target)) + { + return true; + } target = default; return false;🤖 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 `@MCPForUnity/Editor/Tools/Build/BuildTargetMapping.cs` around lines 30 - 34, The current default branch uses Enum.TryParse(name, true, out target) which will accept numeric strings and obsolete enum names; change the fallback to parse then validate—call Enum.TryParse(name, true, out target) and then only accept it if Enum.IsDefined(typeof(BuildTarget), target) (or first reject numeric inputs via int.TryParse(name, out _) before parsing), or limit acceptance to the specific VisionOS names you want; update the default branch around the Enum.TryParse/target handling so it sets target = default and returns false unless the parsed value passes Enum.IsDefined (or matches the allowed VisionOS identifiers).
🤖 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 `@MCPForUnity/Editor/Tools/Build/BuildTargetMapping.cs`:
- Around line 30-34: The current default branch uses Enum.TryParse(name, true,
out target) which will accept numeric strings and obsolete enum names; change
the fallback to parse then validate—call Enum.TryParse(name, true, out target)
and then only accept it if Enum.IsDefined(typeof(BuildTarget), target) (or first
reject numeric inputs via int.TryParse(name, out _) before parsing), or limit
acceptance to the specific VisionOS names you want; update the default branch
around the Enum.TryParse/target handling so it sets target = default and returns
false unless the parsed value passes Enum.IsDefined (or matches the allowed
VisionOS identifiers).
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildTargetMappingTests.cs`:
- Around line 46-53: The test's visionOS success branch only asserts
TryResolveBuildTarget("visionos", out _) is true but doesn't assert that the
returned error from TryResolveNamedBuildTarget is null, so add an explicit
assertion to make the happy path observable: when visionOSAvailable is true,
after Assert.IsTrue(BuildTargetMapping.TryResolveBuildTarget("visionos", out
_)), add Assert.IsNull(error) (or Assert.IsTrue(error == null ||
StringAssert.Contains("VisionOS", error)) if you prefer) to validate
TryResolveNamedBuildTarget's result; locate this change around the
visionOSAvailable block and the variables error and
BuildTargetMapping.TryResolveBuildTarget/TryResolveNamedBuildTarget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b922606-e138-4cd5-8c2d-37f3a0f1312a
📒 Files selected for processing (4)
MCPForUnity/Editor/Tools/Build/BuildTargetMapping.csMCPForUnity/Editor/Tools/ManageBuild.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildTargetMappingTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BuildTargetMappingTests.cs.meta
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #1063.
That PR fixed one part of the VisionOS compatibility problem by moving the direct enum references behind
UNITY_2023_2_OR_NEWER. New reports in #1112, plus the later comment on #1063, show that this still is not quite safe: some Unity 2023.2.x installations do not exposeBuildTarget.VisionOS/BuildTargetGroup.VisionOS, so the package can still fail to compile before Unity MCP is usable.This changes the mapping to avoid compile-time references to the VisionOS enum values. VisionOS is now resolved by enum name at runtime, only when the current editor actually exposes it.
Changes Made
BuildTarget.VisionOSandBuildTargetGroup.VisionOSreferences fromBuildTargetMapping.Enum.TryParse.visionosis requested but unavailable in the current Unity installation.Testing/Screenshots/Recordings
git diff --check2022.3.4f1batchmode compile ofTestProjects/UnityMCPTestscompleted successfully:Tundra build successExiting batchmode successfully now!I did not run a real VisionOS build locally because I do not have the VisionOS build module/target installed. The positive path is preserved by resolving the enum by name when Unity exposes it.
Documentation Updates
No tools or resources were added, removed, or modified.
Related Issues
Fixes #1112
Follow-up to #1063
Summary by CodeRabbit
New Features
Improvements
Tests