Incorporate review comments

platforms/core-runtime/base-services/src/main/java/org/gradle/internal/classpath/TransformedClassPath.java

@@ -317,7 +317,7 @@ private static TransformedClassPath fromInstrumentingArtifactTransformOutputWith
                 checkArgument(i + 2 < inputFiles.size(), "Missing the instrumented or original JAR for classpath %s", inputFiles);
                 File instrumentedJar = inputFiles.get(i + 1);
                 File originalJar = inputFiles.get(i + 2);
-                checkArgument(areInstrumentedAndOriginalJarValid(instrumentedJar, originalJar), "Instrumented JAR doesn't match original JAR %s", originalJar.getName());
+                checkArgument(areInstrumentedAndOriginalJarValid(instrumentedJar, originalJar), "Instrumented JAR %s doesn't match original JAR %s", instrumentedJar.getAbsolutePath(), originalJar.getAbsolutePath());
                 result.add(originalJar, instrumentedJar);
                 i += 2;
             } else {

platforms/software/dependency-management/src/integTest/groovy/org/gradle/integtests/resolve/transform/ArtifactTransformBuildOperationIntegrationTest.groovy

@@ -22,6 +22,7 @@ import org.gradle.api.artifacts.transform.TransformAction
 import org.gradle.api.artifacts.transform.TransformOutputs
 import org.gradle.api.artifacts.transform.TransformParameters
 import org.gradle.api.file.FileSystemLocation
+import org.gradle.api.internal.initialization.transform.ProjectDependencyInstrumentingArtifactTransform
 import org.gradle.api.provider.Property
 import org.gradle.api.provider.Provider
 import org.gradle.api.tasks.Input
@@ -1365,7 +1366,12 @@ class ArtifactTransformBuildOperationIntegrationTest extends AbstractIntegration
         getPlannedNodes(0)
         getExecutePlannedStepOperations(1)
 
+        // We have 4 artifact transforms: 2 MakeColor transforms and 2 from Gradle instrumentation
         buildOperations.progress(IdentifyTransformExecutionProgressDetails).size() == 4
+        Map<String, List<String>> artifactTransforms = groupArtifactTransformByArtifactName()
+        artifactTransforms["buildSrc.jar"] == [ProjectDependencyInstrumentingArtifactTransform.class.name]
+        artifactTransforms["nested-producer.jar"] ==~ ["MakeColor", ProjectDependencyInstrumentingArtifactTransform.class.name]
+        artifactTransforms["nested-producer.jar.red"] == ["MakeColor"]
         buildOperations.all(ExecuteWorkBuildOperationType).size() == 4
         buildOperations.all(SnapshotTransformInputsBuildOperationType).size() == 4
         buildOperations.all(ExecuteTransformActionBuildOperationType).size() == 4
@@ -1389,12 +1395,23 @@ class ArtifactTransformBuildOperationIntegrationTest extends AbstractIntegration
         getPlannedNodes(0)
         getExecutePlannedStepOperations(4)
 
+        // We have 4 artifact transforms: 2 MakeColor transforms and 2 from Gradle instrumentation
         buildOperations.progress(IdentifyTransformExecutionProgressDetails).size() == 4
+        Map<String, List<String>> artifactTransforms = groupArtifactTransformByArtifactName()
+        artifactTransforms["buildSrc.jar"] == [ProjectDependencyInstrumentingArtifactTransform.class.name]
+        artifactTransforms["nested-producer.jar"] ==~ ["MakeColor", ProjectDependencyInstrumentingArtifactTransform.class.name]
+        artifactTransforms["nested-producer.jar.red"] == ["MakeColor"]
         buildOperations.all(ExecuteWorkBuildOperationType).size() == 4
         buildOperations.all(SnapshotTransformInputsBuildOperationType).size() == 4
         buildOperations.all(ExecuteTransformActionBuildOperationType).size() == 4
     }
 
+    private Map<String, List<String>> groupArtifactTransformByArtifactName() {
+        return buildOperations.progress(IdentifyTransformExecutionProgressDetails)
+            .groupBy { it.details["artifactName"] as String }
+            .collectEntries { [(it.key): it.value.collect { it.details["transformActionClass"] }] }
+    }
+
     private void setupProjectTransformInBuildScriptBlock(boolean inExternalScript) {
         setupBuildWithChainedColorTransform()
 

subprojects/core/src/integTest/groovy/org/gradle/internal/classpath/BuildScriptClasspathIntegrationSpec.groovy

@@ -242,12 +242,12 @@ class BuildScriptClasspathIntegrationSpec extends AbstractIntegrationSpec implem
         succeeds("showBuildscript")
 
         then:
-        def jar = inJar9Cache("proj.jar").assertExists()
+        def jar = inJarCache("proj.jar").assertExists()
         journal.assertExists()
 
         when:
         run '--stop' // ensure daemon does not cache file access times in memory
-        jars9GcFile.lastModified = daysAgo(2)
+        jarsGcFile.lastModified = daysAgo(2)
         writeLastFileAccessTimeToJournal(jar.parentFile, daysAgo(MAX_CACHE_AGE_IN_DAYS + 1))
 
         and:
@@ -275,7 +275,7 @@ class BuildScriptClasspathIntegrationSpec extends AbstractIntegrationSpec implem
             userHomeCacheDir.createDir("${DefaultClasspathTransformerCacheFactory.CACHE_NAME}-1"),
             userHomeCacheDir.createDir("${DefaultClasspathTransformerCacheFactory.CACHE_NAME}-2")
         ]
-        jars9GcFile.createFile().lastModified = daysAgo(2)
+        jarsGcFile.createFile().lastModified = daysAgo(2)
 
         when:
         succeeds("help")
@@ -513,13 +513,13 @@ class BuildScriptClasspathIntegrationSpec extends AbstractIntegrationSpec implem
         getArtifactTransformJarsByName("instrumented/testClasses.jar").size() == 1
     }
 
-    void notInJar9Cache(String filename) {
-        inJar9Cache(filename, false)
+    void notInJarCache(String filename) {
+        inJarCache(filename, false)
     }
 
-    TestFile inJar9Cache(String filename, boolean shouldBeFound = true) {
+    TestFile inJarCache(String filename, boolean shouldBeFound = true) {
         String fullpath = result.output.readLines().find { it.matches(">>>file:.*${filename}") }.replace(">>>", "")
-        assert fullpath.startsWith(jars9CacheDir.toURI().toString()) == shouldBeFound
+        assert fullpath.startsWith(jarsCacheDir.toURI().toString()) == shouldBeFound
         return new TestFile(new File(URI.create(fullpath)))
     }
 
@@ -534,11 +534,11 @@ class BuildScriptClasspathIntegrationSpec extends AbstractIntegrationSpec implem
     }
 
 
-    TestFile getJars9GcFile() {
-        return jars9CacheDir.file("gc.properties")
+    TestFile getJarsGcFile() {
+        return jarsCacheDir.file("gc.properties")
     }
 
-    TestFile getJars9CacheDir() {
+    TestFile getJarsCacheDir() {
         return userHomeCacheDir.file(DefaultClasspathTransformerCacheFactory.CACHE_KEY)
     }
 
@@ -552,7 +552,7 @@ class BuildScriptClasspathIntegrationSpec extends AbstractIntegrationSpec implem
      * @return the list of transformed JARs in the cache
      */
     List<File> getArtifactTransformJarsByName(String jarName) {
-        return Files.find(artifactTransformCacheDir.toPath(), 4, (path, attributes) -> normaliseFileSeparators(path.toString()).endsWith(jarName))
+        return Files.find(artifactTransformCacheDir.toPath(), Integer.MAX_VALUE, (path, attributes) -> normaliseFileSeparators(path.toString()).endsWith(jarName))
             .map { new TestFile(it.toFile()) }
             .collect(Collectors.toList())
     }

subprojects/core/src/main/java/org/gradle/api/internal/initialization/DefaultScriptClassPathResolver.java

@@ -15,12 +15,12 @@
  */
 package org.gradle.api.internal.initialization;
 
-import com.google.common.collect.ImmutableSet;
 import org.gradle.api.Action;
 import org.gradle.api.JavaVersion;
 import org.gradle.api.artifacts.ArtifactView;
 import org.gradle.api.artifacts.Configuration;
 import org.gradle.api.artifacts.ConfigurationContainer;
+import org.gradle.api.artifacts.component.ComponentIdentifier;
 import org.gradle.api.artifacts.component.ProjectComponentIdentifier;
 import org.gradle.api.artifacts.dsl.DependencyHandler;
 import org.gradle.api.attributes.Attribute;
@@ -54,10 +54,10 @@
 
 public class DefaultScriptClassPathResolver implements ScriptClassPathResolver {
 
-    private static final Set<DependencyFactoryInternal.ClassPathNotation> NO_GRADLE_API = EnumSet.copyOf(ImmutableSet.of(
+    private static final Set<DependencyFactoryInternal.ClassPathNotation> GRADLE_API_NOTATIONS = EnumSet.of(
         DependencyFactoryInternal.ClassPathNotation.GRADLE_API,
         DependencyFactoryInternal.ClassPathNotation.LOCAL_GROOVY
-    ));
+    );
 
     private static final Attribute<Boolean> HIERARCHY_COLLECTED_ATTRIBUTE = Attribute.of("org.gradle.internal.hierarchy-collected", Boolean.class);
     private static final Attribute<String> INSTRUMENTED_ATTRIBUTE = Attribute.of("org.gradle.internal.instrumented", String.class);
@@ -129,20 +129,26 @@ public ClassPath resolveClassPath(Configuration classpathConfiguration, Dependen
     private static FileCollection getInstrumentedExternalDependencies(Configuration classpathConfiguration) {
         return classpathConfiguration.getIncoming().artifactView((Action<? super ArtifactView.ViewConfiguration>) config -> {
             config.attributes(it -> it.attribute(INSTRUMENTED_ATTRIBUTE, INSTRUMENTED_EXTERNAL_DEPENDENCY_ATTRIBUTE));
-            config.componentFilter(componentId -> {
-                if (componentId instanceof OpaqueComponentIdentifier) {
-                    DependencyFactoryInternal.ClassPathNotation classPathNotation = ((OpaqueComponentIdentifier) componentId).getClassPathNotation();
-                    return !DefaultScriptClassPathResolver.NO_GRADLE_API.contains(classPathNotation);
-                }
-                return !(componentId instanceof ProjectComponentIdentifier);
-            });
+            config.componentFilter(componentId -> !isGradleApi(componentId) && !isProject(componentId));
         }).getFiles();
     }
 
+    private static boolean isGradleApi(ComponentIdentifier componentId) {
+        if (componentId instanceof OpaqueComponentIdentifier) {
+            DependencyFactoryInternal.ClassPathNotation classPathNotation = ((OpaqueComponentIdentifier) componentId).getClassPathNotation();
+            return DefaultScriptClassPathResolver.GRADLE_API_NOTATIONS.contains(classPathNotation);
+        }
+        return false;
+    }
+
+    private static boolean isProject(ComponentIdentifier componentId) {
+        return componentId instanceof ProjectComponentIdentifier;
+    }
+
     private static FileCollection getInstrumentedProjectDependencies(Configuration classpathConfiguration) {
         return classpathConfiguration.getIncoming().artifactView((Action<? super ArtifactView.ViewConfiguration>) config -> {
             config.attributes(it -> it.attribute(INSTRUMENTED_ATTRIBUTE, INSTRUMENTED_PROJECT_DEPENDENCY_ATTRIBUTE));
-            config.componentFilter(componentId -> componentId instanceof ProjectComponentIdentifier);
+            config.componentFilter(DefaultScriptClassPathResolver::isProject);
         }).getFiles();
     }
 }

subprojects/core/src/main/java/org/gradle/internal/classpath/DefaultClasspathBuilder.java

@@ -30,9 +30,12 @@
 
 /**
  * Default implementation of {@link ClasspathBuilder} that is registered also as a service.
- *
+ * <p>
  * This implementation first writes Jar file to a temp file and then moves the result to the destination file.
- * You can use @{@link InPlaceClasspathBuilder} if you want to avoid this indirection and write directly to the destination file.
+ * You can use {@link InPlaceClasspathBuilder} if you want to avoid this indirection and write directly to the destination file.
+ * <p>
+ * If you execute work where output integrity and atomicity is enforced (e.g. with execution engine) you should prefer {@link InPlaceClasspathBuilder},
+ * otherwise this implementation can help you to avoid having partially written files.
  */
 @NonNullApi
 @ServiceScope(Scopes.UserHome.class)