Skip to content

Conversation

strawlink
Copy link

This fixes #426 where paths would be combined incorrectly on Windows (used backslash when forward slash is expected).

As discussed in the comments, I also added a user setting "Use relative repository path":
image

The value defaults to false to preserve existing behavior.

strawlink added 2 commits May 13, 2021 16:08
- 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
- 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
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

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

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.

Copy link
Collaborator

@chkuang-g chkuang-g left a comment

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

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 {

@chkuang-g
Copy link
Collaborator

Sorry for the late reply and thanks for the PR!

@strawlink
Copy link
Author

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
Copy link
Collaborator

Thanks for the contribution!

I think this also fixed #239 as well

@chkuang-g
Copy link
Collaborator

chkuang-g commented Jul 16, 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)

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.

@strawlink
Copy link
Author

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.

FileUtils.PosixPathSeparators(
Path.Combine(projectFileUri, repoPath)));
} else {
repoUri = String.Format("(unityProjectPath + \"/{0}\")", repoPath);
Copy link
Collaborator

@chkuang-g chkuang-g Jul 20, 2021

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
Copy link
Collaborator

Hi @strawlink

Thank you for the contribution! This should be patched in #604. I will close this PR for now

@chkuang-g chkuang-g closed this Apr 14, 2023
@googlesamples googlesamples locked and limited conversation to collaborators May 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Invalid path to local repository when patching mainTemplate.gradle
2 participants