Add task to set the project package resolution strategy #171

Azurelol · 2022-12-23T09:38:33Z

Description

Adds task to modify the project package resolution strategy, setResolutionStrategy, according to this:
(https://docs.unity3d.com/Manual/upm-manifestPrj.html#resolutionStrategy)

The task class derives from a base ProjectManifestTask, which is now shared with the existing AddUpmPackages task that was previously implemented.

Changes

setResolutionStrategy plugin task
AddUPMPackages task base layout into a base task, ProjectManifestTask

src/main/groovy/wooga/gradle/unity/UnityPlugin.groovy

src/main/groovy/wooga/gradle/unity/UnityPluginExtension.groovy

src/main/groovy/wooga/gradle/unity/traits/UnityCommandLineSpec.groovy

src/main/groovy/wooga/gradle/unity/tasks/ProjectManifestTask.groovy

Larusso

Ok maybe I asked that before. It seems that this logic will hard switch to a new strategy and leaves the changed value in place after the build is done? I also miss a lot of the property tests for the newly added tasks and task properties. I'm still a little bit confused about the test changes to be honest. Lets check this PR and the changes together.

To the model. I would like to see a class representation for the Manifest. That is not needed know but I think it is nicer to have an actual API to work against than using a object only representation. Something like this:
https://github.com/wooga/atlas-xcodebuild/blob/master/src/main/groovy/wooga/gradle/xcodebuild/config/ExportOptions.groovy

Larusso · 2023-01-13T07:15:58Z

src/main/groovy/wooga/gradle/unity/traits/UnitySpec.groovy

+    void setProjectDirectory(String path) {
+        projectDirectory.set(layout.projectDirectory.dir(path))
+    }
+


I thought we decided not to add too many overloads for the setters. And in this case, because we have a few overloads we wanted to keep, was to use File over String. Because in the build.gradle file one can express this quite nicely with:

unity.projectDirectory = file("path/to/project")

Which is cleaner than allowing any String to be passed to the setter.

Yes, good point.

Larusso · 2023-01-13T07:22:20Z

src/main/groovy/wooga/gradle/unity/UnityTask.groovy

+            def _path = projectDirectory.get().asFile.path
+            projectPath = _path


Why the two lines? I guess you needed to debug a value?

I was transforming the value but ended up taking that back. Will revert.

Larusso · 2023-01-13T07:23:48Z

src/main/groovy/wooga/gradle/unity/UnityPluginExtension.groovy

+    void setPackagesDir(Provider<Directory> value) {
+        packagesDir.set(value)
+    }


To get in line with most other getter/setter I would also add a File setter for packagesDir

Larusso · 2023-01-13T07:29:44Z

src/integrationTest/groovy/wooga/gradle/unity/tasks/AddUPMPackagesTaskIntegrationSpec.groovy

+        runTasksSuccessfully(taskName)

        then: "manifest file contains added packages"
        def dependencies = new JsonSlurper().parse(manifestFile)["dependencies"]
        dependencies["com.unity.testtools.codecoverage"] == "1.1.0"
        dependencies["com.unity.package"] == "anyString"
+
+        where:
+        taskName = "schonerKatzen"


Ok why is it no longer using the subjectUnderTestName? Also I would refrain from non technical names for test values. Especialy if they show up in reports or logs. schonerKatzen in a log output etc for tests reads like some weird error.

Because this is no longer an UnityTask. I did not want to add another TaskIntegrationSpec.

And sure.

Larusso · 2023-01-13T07:34:01Z

src/integrationTest/groovy/wooga/gradle/unity/tasks/AddUPMPackagesTaskIntegrationSpec.groovy

 import wooga.gradle.unity.UnityTaskIntegrationSpec

-class AddUPMPackagesTaskIntegrationSpec extends UnityTaskIntegrationSpec<AddUPMPackages> {
-
+class AddUPMPackagesTaskIntegrationSpec extends UnityTaskIntegrationSpec {


Why are you no longer using the Generic setup for this task type?

See above; as these tasks that modify the project manifest no longer invoke unity themselves, they are not UnityTasks.

Azurelol · 2023-01-13T14:05:34Z

Ok maybe I asked that before. It seems that this logic will hard switch to a new strategy and leaves the changed value in place after the build is done? I also miss a lot of the property tests for the newly added tasks and task properties. I'm still a little bit confused about the test changes to be honest. Lets check this PR and the changes together.

To the model. I would like to see a class representation for the Manifest. That is not needed know but I think it is nicer to have an actual API to work against than using a object only representation. Something like this: https://github.com/wooga/atlas-xcodebuild/blob/master/src/main/groovy/wooga/gradle/xcodebuild/config/ExportOptions.groovy

I don't mind making a model out of the project manifest, will do.

Will also add the task integration tests.

- Add generic task to modify the project manifest file - Add plugin task to ensure there's a package manifest

Azurelol force-pushed the add/modify_project_manifest_task branch 3 times, most recently from 7e164d8 to e42c4c4 Compare December 23, 2022 15:49

Azurelol force-pushed the add/modify_project_manifest_task branch from e42c4c4 to c973e97 Compare January 2, 2023 10:59

Azurelol marked this pull request as ready for review January 2, 2023 11:02

Azurelol requested a review from Larusso as a code owner January 2, 2023 11:02

Azurelol force-pushed the add/modify_project_manifest_task branch 11 times, most recently from d64ef08 to 24974af Compare January 4, 2023 14:21

Larusso requested changes Jan 5, 2023

View reviewed changes

Azurelol force-pushed the add/modify_project_manifest_task branch 2 times, most recently from 8c8aac6 to 5d7a831 Compare January 6, 2023 09:49

Azurelol requested a review from Larusso January 12, 2023 09:10

Larusso requested changes Jan 13, 2023

View reviewed changes

Azurelol force-pushed the add/modify_project_manifest_task branch from 5d7a831 to 28f416a Compare January 13, 2023 16:51

Azurelol requested a review from Larusso January 13, 2023 16:57

Azurelol force-pushed the add/modify_project_manifest_task branch 2 times, most recently from 8c920f2 to c73a282 Compare January 16, 2023 13:39

Add task to set the project package resolution strategy

49f25ee

- Add generic task to modify the project manifest file - Add plugin task to ensure there's a package manifest

Azurelol force-pushed the add/modify_project_manifest_task branch from c73a282 to 49f25ee Compare January 16, 2023 14:09

Larusso approved these changes Jan 17, 2023

View reviewed changes

Larusso merged commit 2a72e9a into master Jan 17, 2023

Larusso deleted the add/modify_project_manifest_task branch January 17, 2023 07:27

Larusso added the road to upm label Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add task to set the project package resolution strategy #171

Add task to set the project package resolution strategy #171

Azurelol commented Dec 23, 2022 •

edited

Loading

Larusso left a comment

Larusso Jan 13, 2023

Azurelol Jan 13, 2023 •

edited

Loading

Larusso Jan 13, 2023

Azurelol Jan 13, 2023

Larusso Jan 13, 2023

Azurelol Jan 13, 2023

Larusso Jan 13, 2023

Azurelol Jan 13, 2023

Larusso Jan 13, 2023

Azurelol Jan 13, 2023

Azurelol commented Jan 13, 2023 •

edited

Loading

		def _path = projectDirectory.get().asFile.path
		projectPath = _path

Add task to set the project package resolution strategy #171

Add task to set the project package resolution strategy #171

Conversation

Azurelol commented Dec 23, 2022 • edited Loading

Description

Changes

Larusso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azurelol Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azurelol commented Jan 13, 2023 • edited Loading

Azurelol commented Dec 23, 2022 •

edited

Loading

Azurelol Jan 13, 2023 •

edited

Loading

Azurelol commented Jan 13, 2023 •

edited

Loading