Configuration cache by qwert2603 · Pull Request #51 · dropbox/dependency-guard

qwert2603 · 2022-09-15T12:40:07Z

Fixes #28 and improves test coverage.
Changes are split to commits to simplify review.

qwert2603 · 2022-09-22T18:31:21Z

@handstandsam hello! Can you review this PR please?

handstandsam · 2022-09-23T01:43:11Z

Ahh thanks, didn't see this one. Thanks for tagging, will review in the morning. Thanks!

qwert2603 · 2022-09-27T12:05:58Z

@handstandsam are there any updates on this PR?

handstandsam · 2022-09-27T12:23:09Z

I didn't forget about this, but have been busy at work and haven't gotten a chance to pull down and review it. Sorry. Still on my radar.

…ionAction

…sk's configurationAction

…pendencies tree rendering

…nfigurations

handstandsam · 2022-10-10T13:50:21Z

Thanks for rebasing and updating. I might defer to @autonomousapps or @joshafeinberg as I haven't implemented configuration cache myself.

handstandsam · 2022-10-10T14:16:01Z

Thanks for all this work! Because it's such a large change we'll probably have some back and forth on this one.

I know you split things up into commits, but there are 14, so it's hard even with that. If you break this apart into smaller PRs that build on each other, that may make this easier to merge in.

I added a bunch of comments, but I still will want eyes from someone who knows Configuration Cache (as I have not done any conversions).

Additionally, I want to not use Internal APIs unless it's absolutely required like it was for :buildEnvironment.

Thanks, and sorry this took me a while to get to reviewing.

qwert2603 · 2022-10-10T18:42:27Z

@handstandsam yes, I agree that large changes make some subtle bugs harder to find.
So I created PR #57 with the first part of the changes.

I added a bunch of comments,

Do you mean comments on the code? I have not seen ones..

handstandsam

Sorry, I never submitted this. These are the comments I wrote earlier today.

handstandsam · 2022-10-10T13:55:31Z

------------------------------------------------------------
-
-classpath
 +--- org.jetbrains.kotlin:kotlin-gradle-plugin:1.6.10


What causes this change in the way baselines are made?

Earlier tree baseline was equal to the output of the dependencies task, so it included some header and footer from that output. But dependencies task does not support Configuration Cache, that's why it is not used for creating tree baseline now, and AsciiDependencyReportRenderer2 is used instead.

AsciiDependencyReportRenderer2 renders only dependencies tree without header and footer, because rendering header and footer required Configuration. But Configuration can't be used as task's @get:Input and obtaining it during task's execution breaks Configuration Cache.

handstandsam · 2022-10-10T13:56:03Z

-public abstract class com/dropbox/gradle/plugins/dependencyguard/internal/list/DependencyGuardListTask : org/gradle/api/DefaultTask {
-	public fun <init> ()V
-	public abstract fun getAvailableConfigurations ()Lorg/gradle/api/provider/ListProperty;
-	public abstract fun getForRootProject ()Lorg/gradle/api/provider/Property;
-	public abstract fun getMonitoredConfigurations ()Lorg/gradle/api/provider/ListProperty;
-	public abstract fun getShouldBaseline ()Lorg/gradle/api/provider/Property;
-}


Why would this not be public anymore?

dependency-guard plugin is configured by DependencyGuardConfiguration and DependencyGuardPluginExtension, while DependencyGuardListTask is an implementation detail.

DependencyTreeDiffTask was also internal.

handstandsam · 2022-10-10T13:56:42Z

+        // verify tree baseline
+        project.assertFileExistsWithContentEqual(
+            filename = "lib/dependencies/compileClasspath.tree.txt",
+            contentFile = "simple/tree_before_update.txt",


What's the rationale behind naming simple for this directory?

It's named so after fixture/SimpleProject.kt.
It contains files used to verify output of baselines for SimpleProject, so it looks like an appropriate name.

handstandsam · 2022-10-10T13:58:46Z

-            // Gradle 7.4 is the minimum version for which this feature is relevant
-            gradleVersion = GradleVersion.version("7.4"),
-            args = arrayOf(":lib:dependencyGuard", "--configuration-cache")
+            args = arrayOf(":lib:dependencyGuard")


I think for the purposes of being explicit, --configuration-cache is still valid.

Also, by doing this, are we ending support for Gradle 7.4? At this point we've been supporting older versions. I'm open to potentially making this change, but need to discuss.

--configuration-cache flag is added in fun runner in fixture/Builder, so there is no need to add it here.

I also ran tests with gradle-7.4.2 -- they all passed successfully (#58).

I'd suggest writing parameterized tests so you can test against various Gradle versions, if you want to ensure support for older versions as well as newer.

Created #62

handstandsam · 2022-10-10T14:01:30Z


-class SimpleProject : AbstractProject() {
+class SimpleProject(
+    private val tree: Boolean = true,


Could you make this be a parameter without a default value? I feel like the tests would be more explicit that way.

handstandsam · 2022-10-10T14:02:57Z

-                } else {
-                    DependencyTreeDiffTask::class.java
-                }
+                val taskClass = DependencyTreeDiffTask::class.java


🤔 - will have to see later on in this PR how you support the Build Environment tree for ./gradlew :buildEnvironment

The tree baseline for the root project is created from classpath configuration as before.
Now the tree baseline is created in a general way (via ResolvedComponentResult) for the root project and for the usual gradle module.

handstandsam · 2022-10-10T14:09:57Z

+import org.gradle.api.tasks.diagnostics.internal.TextReportRenderer
+import org.gradle.api.tasks.diagnostics.internal.dependencies.AsciiDependencyReportRenderer
+import org.gradle.api.tasks.diagnostics.internal.graph.DependencyGraphsRenderer
+import org.gradle.api.tasks.diagnostics.internal.graph.NodeRenderer
+import org.gradle.api.tasks.diagnostics.internal.graph.SimpleNodeRenderer
+import org.gradle.api.tasks.diagnostics.internal.graph.nodes.RenderableModuleResult
+import org.gradle.internal.graph.GraphRenderer
+import org.gradle.internal.logging.text.StyledTextOutput


I would like to stay away from internal Gradle APIs as much as possible. I had to do it for the buildEnvironment tree only #25, but I want to stay away from internal APIs if there is anything public available. That will ensure better version compatibility in the future.

I agree that it's better to avoid internal api usage, but dependencies task does not support Configuration Cache. That's why AsciiDependencyReportRenderer2 was added to render ResolvedComponentResult.

I didn't find another way to render dependencies tree, so I needed to stay on internal api. But I'm ready to switch to public api that I'm not aware of, if it makes the same thing.

What does this class do? Could you reimplement it? Or even just copy and paste the code into this project?

class AsciiDependencyReportRenderer2 renders dependencies tree of the particular configuration (represented by org.gradle.api.artifacts.result.ResolvedComponentResult).

This is a modified copy of AsciiDependencyReportRenderer from Gradle sources.

AsciiDependencyReportRenderer works with org.gradle.api.artifacts.Configuration, while AsciiDependencyReportRenderer2 works with org.gradle.api.artifacts.result.ResolvedComponentResult. This is because org.gradle.api.artifacts.Configuration is not available in task's actions, when configuration cache is enabled.

Actually, original AsciiDependencyReportRenderer in public void render(Configuration configuration) receives ResolvedComponentResult from org.gradle.api.artifacts.Configuration, so there is not much changes in AsciiDependencyReportRenderer2.

Another change is that AsciiDependencyReportRenderer2 renders only dependencies tree without header and footer, because rendering header and footer requeires org.gradle.api.artifacts.Configuration in original AsciiDependencyReportRenderer. But that is not that important, because we are more interested in dependencies trees diff, that in header and footer.

handstandsam · 2022-10-10T14:10:55Z

+/**
+ * Simplified version of [AsciiDependencyReportRenderer] that renders [ResolvedComponentResult].
+ */
+internal class AsciiDependencyReportRenderer2 : TextReportRenderer() {


I have seen these internal APIs, but haven't used it. Can the comment declare if this is used for just Trees, and if it's used for :buildEnvironment or normal :dependencies tasks?

DependencyTreeDiffTask is not based on :buildEnvironment or :dependencies now.
It is a simple task that receives ResolvedComponentResult as input and renders its dependencies tree to file using AsciiDependencyReportRenderer2.

handstandsam · 2022-10-10T14:13:09Z

    // All dependencies included in Production Release APK
    configuration("releaseRuntimeClasspath") {
        modules = true
+        tree = true


I'd like to discourage the use of tree format as I feel like it's more helpful in local debugging. It's nice to have it there, but I didn't use it in the sample because 1) it's not as useful in most situations as it's so noisy 2) the build environment piece used internal APIs, so it's more of a "use at your own risk".

Ok, tree format is actually not that useful, so I'll revert this change.

qwert2603 · 2022-10-13T13:40:14Z

Arghh, the request for review from joshafeinberg was removed when requesting a review from handstandsam

qwert2603 · 2022-12-12T10:17:02Z

@handstandsam Hi! Are there any updates on this PR?

handstandsam · 2022-12-12T21:28:11Z

@qwert2603 - Sorry, dependency-guard has been working for our use cases so I haven't had any extra time to look into this. Configuration Cache is important, but the workaround ensures that configuration cache is aware of it must running. Therefore it is working, but not with config cache.

In order to feel comfortable merging, I would want to sit down and run the new code as it's not small changes.

I sent you a DM on Kotlin Lang Slack about this topic if you can check and reply there. Thank you!

handstandsam · 2022-12-14T02:15:42Z

@@ -5,5 +5,4 @@ import org.gradle.util.GradleVersion
 internal object GradleVersion {
    private val current = GradleVersion.current()
    val isAtLeast73 = current >= GradleVersion.version("7.3")


Can we drop this as well now?

I think it may be dropped together will adding parameterized tests (#62)

qwert2603 added 14 commits October 10, 2022 10:32

add test cases

2fb0992

save project.path while configuring task in DependencyGuardTreeDiffer

75b7238

update gradle to 7.5.1

38120aa

call ConfigurationValidators.requirePluginConfig in task's configurat…

7494460

…ionAction

call ConfigurationValidators.validateConfigurationsAreAvailable in ta…

2856e81

…sk's configurationAction

get buildDirectory and projectDirectory in task's configurationAction

28e8c2e

use project.path and task's input in DependencyGuardListTask

236199f

use ResolvedComponentResults as DependencyGuardListTask's input

b913d0d

support configuration cache in DependencyTreeDiffTask and refactor de…

8d06dba

…pendencies tree rendering

update tests to check configuration cache support

57c661a

add test guard after dependencies list changes

cd516c8

add test for wrong configuration

2f1262e

make DependencyGuardListTask internal

761f09b

update FullProject test -- check generated baselines for different co…

93356e2

…nfigurations

qwert2603 force-pushed the configuration-cache branch from 65c3921 to 93356e2 Compare October 10, 2022 05:52

handstandsam requested a review from joshafeinberg October 10, 2022 13:53

handstandsam mentioned this pull request Oct 10, 2022

Feature request: filter out BOM artifacts #54

Open

qwert2603 mentioned this pull request Oct 10, 2022

Configuration cache pt1 #57

Closed

handstandsam reviewed Oct 11, 2022

View reviewed changes

qwert2603 added 3 commits October 13, 2022 18:16

disable tree format in sample app

8977697

remove default value of "SimpleProject.tree" to make tests more explicit

ad6207c

add comments on AsciiDependencyReportRenderer2

fc92a62

qwert2603 removed the request for review from joshafeinberg October 13, 2022 13:36

qwert2603 requested a review from handstandsam October 13, 2022 13:36

handstandsam reviewed Dec 14, 2022

View reviewed changes

handstandsam merged commit 6be98d6 into dropbox:main Dec 14, 2022

handstandsam mentioned this pull request Dec 14, 2022

Configuration cache 7.4.2 #58

Closed

qwert2603 mentioned this pull request Dec 14, 2022

Add tests for various Gradle versions #62

Closed

Uh oh!

Conversation

qwert2603 commented Sep 15, 2022

Uh oh!

qwert2603 commented Sep 22, 2022

Uh oh!

handstandsam commented Sep 23, 2022

Uh oh!

qwert2603 commented Sep 27, 2022

Uh oh!

handstandsam commented Sep 27, 2022

Uh oh!

handstandsam commented Oct 10, 2022

Uh oh!

handstandsam commented Oct 10, 2022

Uh oh!

qwert2603 commented Oct 10, 2022

Uh oh!

handstandsam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwert2603 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwert2603 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwert2603 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwert2603 commented Oct 13, 2022

Uh oh!

qwert2603 commented Dec 12, 2022

Uh oh!

handstandsam commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

qwert2603 Oct 13, 2022 •

edited

Loading

qwert2603 Oct 13, 2022 •

edited

Loading

qwert2603 Oct 13, 2022 •

edited

Loading