chore: editor namespace auto upgrade - #4142
Conversation
There was a problem hiding this comment.
💡 Harness Review
The relocation metadata covers the unconditional public editor API, but the new validation harness has a configuration-specific coverage gap and cannot be run reliably from paths containing spaces.
Reviewed commit dcf9a26
🤖 Helpful? 👍/👎
| ) | ||
|
|
||
| Write-Host 'Running the editor (this imports the project and runs the API updater)...' | ||
| $process = Start-Process -FilePath $unity -ArgumentList $unityArgs -PassThru -Wait -NoNewWindow |
There was a problem hiding this comment.
Start-Process joins this array into one command line without preserving quotes around individual values. If the checkout is under a path such as C:\Users\Jane Doe\src, both $projectPath and $logFile are split at the space, so Unity receives an invalid -projectPath/-logFile and the documented local test cannot run. Quote/escape those two values when building the argument string, or use ProcessStartInfo.ArgumentList to pass them as distinct arguments.
🤖 Helpful? 👍/👎
There was a problem hiding this comment.
@u-pr This is fixed and should no longer be a problem.
There was a problem hiding this comment.
Looks good! Quoting "$projectPath" and "$logFile" ensures the paths won't be split on spaces when passed to Start-Process.
🤖 Helpful? 👍/👎
| # Every 2.x type the sources reference, and what the updater is expected to turn it into. | ||
| # Frozen: this is the public editor API of develop-2.0.0, which is released and will not change. | ||
| # Extend it by hand if a public editor type is ever relocated again within 3.x. | ||
| $expected = @( |
There was a problem hiding this comment.
This expected list has no NetworkObjectOwnership entry, and the fixture never references it. That class is a public old editor type whenever BYPASS_DEFAULT_ENUM_DRAWER && MULTIPLAYER_SERVICES_SDK_INSTALLED is enabled, so the test currently cannot verify the [MovedFrom] just added for that supported compilation mode; a regression there will still report success. Add a fixture/configuration that enables those symbols and include the type in the expected results.
🤖 Helpful? 👍/👎
There was a problem hiding this comment.
@u-pr Can you re-verify this? I remove that legacy script in NetworkObjectEditor.cs so it should not be of concern now.
There was a problem hiding this comment.
Verified. Since NetworkObjectOwnership has been removed from NetworkObjectEditor.cs rather than migrated to the new namespace with a [MovedFrom] attribute, it no longer needs coverage in DeprecatedApiUsage.cs or the $expected list in run-upgrade-test.ps1. The concern is resolved.
🤖 Helpful? 👍/👎
Removed obsolete custom property drawer for NetworkObject.OwnershipStatus.
…com/Unity-Technologies/com.unity.netcode.gameobjects into chore/editor-namespace-auto-upgrade
| @@ -0,0 +1,78 @@ | |||
| # This .gitignore file should be placed at the root of your Unity project directory | |||
There was a problem hiding this comment.
It helps keep it clean from "noise changes".
It is the same as the testproject one.
| @@ -0,0 +1,173 @@ | |||
| <# | |||
There was a problem hiding this comment.
This is a powershell script so it can't be run from Ubuntu or MacOS. It's also pretty opaque claude written code. A python script (@michalChrobot already has a few python scripts) would probably be preferred. Not a blocker, just a concern
There was a problem hiding this comment.
We can make that something more generic.
| @@ -0,0 +1,121 @@ | |||
| # API updater upgrade-path project | |||
There was a problem hiding this comment.
This README has too much info. The README from this project doesn't need info on how the upgrade was implemented, it just needs info on how it works.
If you want this project to be used also for future upgrades then this README shouldn't have any references to the 2.x -> 3.x upgrade specifically, otherwise future us might think we can pull this project out once that upgrade is released.
There was a problem hiding this comment.
This project and the tests associated with it will not be pertinent once v2.x.x becomes deprecated.
But I can remove the extra info...leaving it only provides the context as to why it is there in the first place.
| { | ||
| "disableProjectUpdate": false, | ||
| "dependencies": { | ||
| "com.unity.netcode.gameobjects": "file:../../com.unity.netcode.gameobjects", |
There was a problem hiding this comment.
It might be nice to have N4E in here so we can be sure the upgrade doesn't introduce a conflict with N4E
There was a problem hiding this comment.
This was tested extensively locally. Nothing wrapped in UNIFIED_NETCODE is impacted by the namespace migration since anything that does reference NGO editor classes(etc) already has the change applied and no user has any script that could cause conflicts.
This validates without N4E an NGO users upgrading from v2.x to v3.x will automatically have everything that needs to be updated...updated.
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop-3.x.x #4142 +/- ##
==============================================
Coverage 78.00% 78.00%
==============================================
Files 153 153
Lines 26260 26260
==============================================
Hits 20483 20483
Misses 5777 5777
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Purpose of this PR
This PR handles the auto-upgrade namespace change from Unity.Netcode.Editor to Unity.Netcode.GameObjects.Editor.
It also includes a job that can be manually triggered to validate the auto-upgrade or the job itself could be added to nightly or the like.
Jira ticket
MTT-15570
Changelog
NA
Documentation
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?If any boxes above are checked the QA team will be automatically added as a PR reviewer.
Up-port
None
Backports
None