search

googlesamples / unity-jar-resolver

Unity plugin which resolves Android & iOS dependencies and performs version management
Other
1.21k stars 336 forks source link

Fix project settings resetting on domain reload #678

Closed berniegp closed 1 month ago

berniegp commented 3 months ago

This resolves #524 by avoiding to needlessly save GvhProjectSettings.xml on every domain reload.

In all 3 modified files, the line

VerboseLoggingEnabled = VerboseLoggingEnabled;

does 3 things on domain reload because of [InitializeOnLoad]:

  1. Loads GvhProjectSettings.xml if not already done
  2. Writes GvhProjectSettings.xml unconditionally
  3. Sets logger.Level

This PR removes step 2) which is unneeded and the cause of #524.

It's potentially possible that writing GvhProjectSettings.xml as a result of user interaction might still in some cases have a race condition. I didn't investigate this. However it's much less problematic than intermittent problems on domain reload.

Why the problem happens

The current code is correctly protecting critical sections, including saving GvhProjectSettings.xml, with lock (classLock). However a problem occurs with parallel asset importing enabled in Unity. In this case there are other Unity Editor processes aside from the "main" one that take care of parallel imports. These other processes each have their own separate instance of classLock.

This results in multiple processes reading and writing to GvhProjectSettings.xml on domain reload. Eventually one of them reads the file while it's being written and ends up with empty settings from Google.ProjectSettings.Load which it then re-saves. As the screenshot in #524 shows, when corruption happens it's always one or more VerboseLoggingEnabled that remain in the settings.

About asset import worker processes

The command line of asset import worker processes looks like:

"C:\tools\Unity-Hub\2022.3.19f1\Editor\Unity.exe" "-adb2" "-batchMode" "-noUpm" "-name" "AssetImportWorker7" "-projectPath" "C:/dev/unity-project" "-logFile" "Logs/AssetImportWorker7.log" "-srvPort" "64205"

The -adb2 argument is unique to asset import worker processes so could be used to identify them as needed.

Build on Windows

Binaries here https://github.com/berniegp/unity-jar-resolver/releases/tag/v1.2.179

I could not build the plugin as it stands. The (reverted) commit ff03ffda690943f17800d3a26b97ba0d6e753c33 has the changes I needed to do to build.gradle.

As far as I can see, the build process prefers Unity 5.6. This version (obviously) isn't supported on Apple Silicon so I tried on Windows.

Steps for successful build:

  1. Install Unity 5.6 with Android and iOS support
  2. Start Unity 5.6 to sort out activation
  3. Apply ff03ffda690943f17800d3a26b97ba0d6e753c33
  4. Download and extract https://download.java.net/java/GA/jdk11/9/GPL/openjdk-11.0.2_windows-x64_bin.zip to c:\tools
  5. As administrator (so Python installs correctly, but pollutes add/remove programs) build with: 
    $env:JAVA_HOME='C:\tools\openjdk-11.0.2_windows-x64_bin\jdk-11.0.2\'; .\gradlew build -PUNITY_EXE=’<PATH_TO>\Unity-Hub\5.6.7f1\Editor\Unity.exe’; Remove-Item Env:\JAVA_HOME

Not sure if I'm doing something wrong regarding the build?

google-cla[bot] commented 3 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

berniegp commented 2 months ago

Update: 1 month in at my org we're not seeing setting file corruption anymore on Windows and MacOS.

harlan commented 1 month ago

What are the steps to try to get @berniegp 's https://github.com/googlesamples/unity-jar-resolver/pull/678 tested/merged etc? is that that you need to sign this CLA thing or something?

berniegp commented 1 month ago

I already signed the CLA thing, so next steps aren't on me :)

image

argzdev commented 1 month ago

Hey thanks for the PR, let me get some of our engineering folks to take a look into this.

a-maurice commented 1 month ago

Thanks for the fix, and the detailed write up actually explaining what the issue was, that is definitely a weird case so really appreciate that you tracked it down. I will work on getting a release out with this fix.

As for building it yourself, it can definitely be a bit finicky and depends on a bunch of tools. I wasn't aware of a Unity 5.6 bias, the tests we have run with Unity 2019, but I do know that Apple Silicon can have issues because it depends on a specific python version which isn't available.