Invalidate Configuration Cache when buildSrc builds are added #31460
Conversation
alllex
force-pushed
the
alllex/cc/buildsrc-presence-as-input
branch
from
02a19b5
to
e0d26aa
Compare
| @@ -24,9 +24,10 @@ import static org.junit.Assume.assumeFalse | |||
|
|
|||
| class ConfigurationCacheBuildSrcChangesIntegrationTest extends AbstractConfigurationCacheIntegrationTest { | |||
|
|
|||
| def configurationCache = newConfigurationCacheFixture() | |||
There was a problem hiding this comment.
Doing it here is not typical, but fine by me.
There was a problem hiding this comment.
All tests use this fixture. I saw no need in creating it in every test
| configurationCacheRun "help" | ||
| then: | ||
| configurationCache.assertStateStored() | ||
| buildOperations.none("Load build (:buildSrc)") |
There was a problem hiding this comment.
❓ Did you consider reusing the build ops fixture owned by the CC fixture (by adding corresponding assertions there?)
There was a problem hiding this comment.
I forgot that CC fixture comes with operation tracking as well. It's a good idea to reuse it.
It's as easy as
| buildOperations.none("Load build (:buildSrc)") | |
| configurationCache.operations.none("Load build (:buildSrc)") |
There was a problem hiding this comment.
I changed my mind about this - since the checks we are performing are not related to CC, it makes sense not to use the one from CC fixture
| configurationCacheRun "help" | ||
| then: | ||
| configurationCache.assertStateStored() | ||
| outputContains("Calculating task graph as configuration cache cannot be reused because a buildSrc build at 'buildSrc' has been added.") |
| "buildSrc" | "src/main/groovy/MyClass.groovy" | ||
| } | ||
|
|
||
| def "reuses cache upon changing invalid buildSrc by creating #description"() { |
| @@ -108,6 +114,7 @@ class ConfigurationCacheFingerprintWriter( | |||
| FeatureFlagListener, | |||
| FileCollectionObservationListener, | |||
| ScriptSourceListener, | |||
| BuildAddedListener, | |||
| @@ -204,6 +215,21 @@ class ConfigurationCacheFingerprintWriter( | |||
| CompositeStoppable.stoppable(buildScopedWriter, projectScopedWriter).stop() | |||
| } | |||
|
|
|||
| override fun buildAdded(buildState: BuildState) { | |||
| captureBuildSrcPresence(buildState) | |||
There was a problem hiding this comment.
💭 Maybe add a note for lines 200 and 219 on which cases each one covers? At first, I thought they may be redundant, but confirmed they aren't, not sure why though
There was a problem hiding this comment.
There are two places we capture because we add the listener late. The first visitBuilds call goes over all builds that have been added early, which includes the root build, root/buildSrc, root/buildSrc/buildSrc, etc. The second place react to discovering any included builds, e.g. during the evaluation of settings of any build that is already in the tree.
I think having these two places is good enough solution for now. However, I'd try and find a solution that guarantees we have not skipped any builds this way. We'd need to either make sure our listener is registered unconditionally and early enough, or we should find/create a general Gradle callback that is called after all builds have been added to the registry (and no more builds can be added). I wasn't able to find such a callback if it exists today.
| @@ -760,6 +760,10 @@ tmpdir is currently ${System.getProperty("java.io.tmpdir")}""") | |||
| recreateExecuter() | |||
| } | |||
|
|
|||
| BuildOperationsFixture newBuildOperationsFixture() { | |||
There was a problem hiding this comment.
Yeah, it's not needed if we go with the CC fixture operations
| is ConfigurationCacheFingerprint.BuildSrcCandidate -> input.run { | ||
| val currentValid = host.hasValidBuildSrc(buildSrcDir) | ||
| ifOrNull(currentValid != valid) { | ||
| text("a buildSrc build at ").reference(displayNameOf(buildSrcDir)) |
| @@ -291,6 +294,14 @@ class ConfigurationCacheFingerprintChecker(private val host: Host) { | |||
| text("the set of system properties prefixed by ").reference(prefix).text(" has changed: ").text(detailedMessageForChanges(snapshotWithoutIgnored, currentWithoutIgnored)) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
❌ This blows up the 100 line limit. I would add:
@Suppress("CyclomaticComplexMethod", "LongMethod")
instead of somehow avoiding going over the limit.
Fixes: #31097