Search code, repositories, users, issues, pull requests...


 Added UseRelativeRepoPath user setting

dad76f2

- Allow users to choose whether to use a relative or absolute path
 when patching the mainTemplate.gradle file.
 Value defaults to `false` to preserve existing behavior.
Fixes: googlesamples#426


 Fixed invalid patching of absolute repository path

2179ce8

- Path would be combined with backslash on Windows, resulting in an
 invalid repoUri. The fix ensures that only forward slashes are used.
Fixes: googlesamples#426

@vimanyu vimanyu requested review from chkuang-g and vimanyu

June 29, 2021 19:12

chkuang-g

chkuang-g requested changes

Jun 30, 2021

source/AndroidResolver/src/SettingsDialog.cs Outdated

if (settings.useRelativeRepoPath) {

GUILayout.Label(

"The mainTemplate.gradle file will be patched with a relative path to " +

"the Local Maven Repository. This can be used to limit file changes when " +

Copy link

Collaborator

@chkuang-g chkuang-g Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this new flag only matters when GradleProjectExportEnabled is true.

Perhaps change to something like "The mainTemplate.gradle file will be patched with a relative path to the Local Maven Repository for an exported Android project."

Copy link

Author

@strawlink strawlink Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. I've updated the description to match your suggestion.

chkuang-g

chkuang-g requested changes

Jun 30, 2021

unity-jar-resolver/source/AndroidResolver/test/src/AndroidResolverIntegrationTests.cs

Copy link

Collaborator

@chkuang-g chkuang-g left a comment •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, worth adding a test like this

Line 238 in 5da4d94

Name = "ResolveForGradleBuildSystemAndExport",

Once added, you can run locally using

./gradles :testAndroidResolverIntegrationTests

If it is too overwhelming to add integration test (that part not well-documented unfortunately), please just add a comment like the following in the SettingsDialog.cs as a reminder for us :)

 //TODO: Please add integration test for this flag.
 internal static bool UseRelativeRepoPath {

Copy link

Collaborator

chkuang-g commented Jun 30, 2021

Sorry for the late reply and thanks for the PR!

strawlink added 2 commits

July 14, 2021 14:17


 Updated settings description

b45c3d0


 Added TODO

9ba0fe9

Copy link

Author

strawlink commented Jul 14, 2021

Unfortunately I have not been able to run the existing integration tests locally, so I've opted to add a TODO. The gradle process simply gets stuck at 85% (:testAndroidResolverIntegrationTestsBatchMode).

As a sidenote, I have also had multiple issues with setting up the build environment, which took a while to figure out:

Project only works with Unity 5.6.7f1.
The build script failed on multiple steps:
- Path to editor only works with default paths
- Python bootstrapping (ended up commenting it out locally & just using my system install)
  I've attached the output here: PythonBootstrapStacktrace.txt
- Mono path refers to a .bat file, but file is an .exe in my clean editor installation.
- Solutions refer to source/unity_dlls and unity_dlls - had to symlink them to inside the editor folder (5.6.7f1\Editor\Data\Managed)

@strawlink strawlink requested a review from chkuang-g

July 15, 2021 14:19

chkuang-g

chkuang-g approved these changes

Jul 16, 2021

Copy link

Collaborator

chkuang-g commented Jul 16, 2021

Thanks for the contribution!

I think this also fixed #239 as well

Copy link

Collaborator

chkuang-g commented Jul 16, 2021 •

edited

Loading

Unfortunately I have not been able to run the existing integration tests locally, so I've opted to add a TODO. The gradle process simply gets stuck at 85% (:testAndroidResolverIntegrationTestsBatchMode).

As a sidenote, I have also had multiple issues with setting up the build environment, which took a while to figure out:

Project only works with Unity 5.6.7f1.

The build script failed on multiple steps:

Path to editor only works with default paths

Python bootstrapping (ended up commenting it out locally & just using my system install)
I've attached the output here: PythonBootstrapStacktrace.txt

Mono path refers to a .bat file, but file is an .exe in my clean editor installation.

Solutions refer to source/unity_dlls and unity_dlls - had to symlink them to inside the editor folder (5.6.7f1\Editor\Data\Managed)

There are a couple of issue to build EDM4U from Windows now. You can see more information on #448

If you cannot build EDM4U, have you ever checked if your change works for your case?

If not, I will need to take some time to test your change locally later.

Copy link

Author

strawlink commented Jul 17, 2021

There are a couple of issue to build EDM4U from Windows now. You can see more information on #448

Those are indeed the same issues I've experienced 👍

If you cannot build EDM4U, have you ever checked if your change works for your case?

I did get it to build by making the mentioned changes, and have verified the behavior for my case locally.

chkuang-g

chkuang-g requested changes

Jul 20, 2021

source/AndroidResolver/src/PlayServicesResolver.cs

FileUtils.PosixPathSeparators(

Path.Combine(projectFileUri, repoPath)));

} else {

repoUri = String.Format("(unityProjectPath + \"/{0}\")", repoPath);

Copy link

Collaborator

@chkuang-g chkuang-g Jul 20, 2021 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did tried your change locally and I noticed that this will generate the following block in exported build.gradle

([rootProject] + (rootProject.subprojects as List)).each { project ->
 def unityProjectPath = $/file:////Users/myusername/Workspace/myprojectfolder/$.replace("\\", "/")
 maven {
 url "https://maven.google.com"
 }
 maven {
 url (unityProjectPath + "/Assets/GeneratedLocalRepo/Firebase/m2repository") // Assets/Firebase/Editor/AnalyticsDependencies.xml:18, Assets/Firebase/Editor/AppDependencies.xml:22
 }

Note that this is still an absolute path instead of a relative path because it combines unityProjectPath and the relative path. For my instance, that will be /file:////Users/myusername/Workspace/myprojectfolder/Assets/GeneratedLocalRepo/Firebase/m2repository

I think the right way to do is to add another String field to specify the relative path from the project root to your export folder.

For instance, given the following project structure:

Project root: "/foo"
Export Android project path: "/foo/bar/baz"
A new setting Export Project Relative Path: "bar/baz"

That way, the code can export something like the following as a relative path:

([rootProject] + (rootProject.subprojects as List)).each { project ->
 def unityProjectPath = $/file:////Users/myusername/Workspace/myprojectfolder/$.replace("\\", "/")
 maven {
 url "https://maven.google.com"
 }
 maven {
 url ("../../Assets/GeneratedLocalRepo/Firebase/m2repository") // Assets/Firebase/Editor/AnalyticsDependencies.xml:18, Assets/Firebase/Editor/AppDependencies.xml:22
 }

I have a feeling that there should be a way to get the exported Android project path somewhere during build time. But that would be a major refactoring for Android Resolver. I think a simple setting like Export Project Relative Path should do.

@chkuang-g chkuang-g force-pushed the master branch from 46ea826 to c599ab3 Compare

April 6, 2022 21:17

@chkuang-g chkuang-g force-pushed the master branch 2 times, most recently from 883eedd to 5c6f23b Compare

October 7, 2022 03:35