Revert "resolve race condition with jar manifest manipulation (#1150)"

.palantir/revapi.yml

@@ -13,3 +13,8 @@ acceptedBreaks:
       old: "class com.palantir.gradle.dist.ProductDependency"
       new: "class com.palantir.gradle.dist.ProductDependency"
       justification: "Old class is semantically equivalent to optional=false"
+  "5.3.0":
+    com.palantir.sls-packaging:gradle-recommended-product-dependencies:
+    - code: "java.field.removed"
+      old: "field com.palantir.gradle.dist.RecommendedProductDependencies.SLS_RECOMMENDED_PRODUCT_DEPS_ATTRIBUTE"
+      justification: "Reverting change"

changelog/@unreleased/pr-1154.v2.yml

@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: removes change to depend directly on configurations
+  links:
+  - https://github.com/palantir/sls-packaging/pull/1154

gradle-recommended-product-dependencies/src/main/java/com/palantir/gradle/dist/RecommendedProductDependencies.java

@@ -20,8 +20,6 @@
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 import java.util.Set;
-import java.util.jar.Attributes;
-import java.util.jar.Attributes.Name;
 import org.immutables.value.Value;
 
 @Value.Immutable
@@ -30,8 +28,6 @@
 @JsonDeserialize(as = ImmutableRecommendedProductDependencies.class)
 public interface RecommendedProductDependencies {
     String SLS_RECOMMENDED_PRODUCT_DEPS_KEY = "Sls-Recommended-Product-Dependencies";
-    Attributes.Name SLS_RECOMMENDED_PRODUCT_DEPS_ATTRIBUTE =
-            new Name(RecommendedProductDependencies.SLS_RECOMMENDED_PRODUCT_DEPS_KEY);
 
     @JsonProperty("recommended-product-dependencies")
     Set<ProductDependency> recommendedProductDependencies();

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/tasks/ResolveProductDependenciesTask.java

@@ -20,23 +20,23 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.palantir.gradle.dist.BaseDistributionExtension;
+import com.palantir.gradle.dist.ConfigureProductDependenciesTask;
 import com.palantir.gradle.dist.ProductDependency;
 import com.palantir.gradle.dist.ProductDependencyMerger;
 import com.palantir.gradle.dist.ProductDependencyReport;
 import com.palantir.gradle.dist.ProductId;
 import com.palantir.gradle.dist.RecommendedProductDependencies;
+import com.palantir.gradle.dist.RecommendedProductDependenciesPlugin;
 import com.palantir.gradle.dist.Serializations;
 import java.io.File;
 import java.io.IOException;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
-import java.util.jar.Attributes;
 import java.util.jar.Manifest;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -45,11 +45,15 @@
 import org.gradle.api.DefaultTask;
 import org.gradle.api.Project;
 import org.gradle.api.artifacts.Configuration;
-import org.gradle.api.artifacts.ResolvedArtifact;
-import org.gradle.api.artifacts.ResolvedDependency;
+import org.gradle.api.artifacts.component.ComponentIdentifier;
+import org.gradle.api.artifacts.component.ProjectComponentIdentifier;
+import org.gradle.api.artifacts.result.ResolvedComponentResult;
+import org.gradle.api.file.ConfigurableFileCollection;
+import org.gradle.api.file.FileCollection;
 import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
+import org.gradle.api.plugins.JavaPlugin;
 import org.gradle.api.provider.ListProperty;
 import org.gradle.api.provider.Property;
 import org.gradle.api.provider.Provider;
@@ -59,12 +63,17 @@
 import org.gradle.api.tasks.OutputFile;
 import org.gradle.api.tasks.TaskAction;
 import org.gradle.api.tasks.TaskProvider;
+import org.gradle.jvm.tasks.Jar;
 
 @CacheableTask
 public abstract class ResolveProductDependenciesTask extends DefaultTask {
     private static final Logger log = Logging.getLogger(ResolveProductDependenciesTask.class);
 
+    private final Property<Configuration> productDependenciesConfig =
+            getProject().getObjects().property(Configuration.class);
+
     public ResolveProductDependenciesTask() {
+        dependsOn(otherProjectProductDependenciesTasks());
         getOutputFile().convention(() -> new File(getTemporaryDir(), "resolved-product-dependencies.json"));
         getDiscoveredProductDependencies().convention(getProject().provider(this::findRecommendedProductDependenies));
     }
@@ -73,22 +82,48 @@ static TaskProvider<ResolveProductDependenciesTask> createResolveProductDependen
             Project project, BaseDistributionExtension ext, Provider<Set<ProductId>> provider) {
         TaskProvider<ResolveProductDependenciesTask> depTask = project.getTasks()
                 .register("resolveProductDependencies", ResolveProductDependenciesTask.class, task -> {
-                    Provider<Configuration> configProvider = project.provider(ext::getProductDependenciesConfig);
                     task.getServiceName().set(ext.getDistributionServiceName());
                     task.getServiceGroup().set(ext.getDistributionServiceGroup());
                     task.getDeclaredProductDependencies().set(ext.getAllProductDependencies());
-                    task.getProductDependenciesConfig().set(configProvider);
+                    task.setConfiguration(project.provider(ext::getProductDependenciesConfig));
                     task.getOptionalProductIds().set(ext.getOptionalProductDependencies());
                     task.getIgnoredProductIds().set(ext.getIgnoredProductDependencies());
                     task.getInRepoProductIds().set(provider);
-
-                    // Need the dependsOn because the configuration is not set as an Input property and
-                    // we need the artifacts in it to be resolved.
-                    task.dependsOn(configProvider);
                 });
         return depTask;
     }
 
+    /**
+     * A lazy collection of tasks that ensure the {@link Jar} task of any project dependencies from
+     * {@link #productDependenciesConfig} is correctly populated with the recommended product dependencies of that
+     * project, if any (specifically, if they apply the {@link RecommendedProductDependenciesPlugin}).
+     */
+    private Provider<FileCollection> otherProjectProductDependenciesTasks() {
+        return productDependenciesConfig.map(productDeps -> {
+            // Using a ConfigurableFileCollection simply because it implements Buildable and provides a convenient API
+            // to wire up task dependencies to it in a lazy way.
+            ConfigurableFileCollection emptyFileCollection = getProject().files();
+            productDeps.getIncoming().getArtifacts().getArtifacts().stream()
+                    .flatMap(artifact -> {
+                        ComponentIdentifier id = artifact.getId().getComponentIdentifier();
+
+                        // Depend on the ConfigureProductDependenciesTask, if it exists, which will wire up the jar
+                        // manifest
+                        // with recommended product dependencies.
+                        if (id instanceof ProjectComponentIdentifier) {
+                            Project dependencyProject = getProject()
+                                    .getRootProject()
+                                    .project(((ProjectComponentIdentifier) id).getProjectPath());
+                            return Stream.of(
+                                    dependencyProject.getTasks().withType(ConfigureProductDependenciesTask.class));
+                        }
+                        return Stream.empty();
+                    })
+                    .forEach(emptyFileCollection::builtBy);
+            return emptyFileCollection;
+        });
+    }
+
     @Input
     public abstract Property<String> getServiceName();
 
@@ -110,8 +145,22 @@ static TaskProvider<ResolveProductDependenciesTask> createResolveProductDependen
     @Input
     public abstract SetProperty<ProductId> getInRepoProductIds();
 
+    /**
+     * Contents of the given configuration.  Cannot list the configuration itself as an input property because the
+     * caching calculations attempt to resolve the configuration at config time.  This can lead to an error for
+     * configs that connot be resolved directly at that stage.  Caching and up-to-date calcs thus use this property.
+     */
     @Input
-    public abstract Property<Configuration> getProductDependenciesConfig();
+    final Set<String> getProductDependenciesConfig() {
+        return productDependenciesConfig.get().getIncoming().getResolutionResult().getAllComponents().stream()
+                .map(ResolvedComponentResult::getId)
+                .map(ComponentIdentifier::getDisplayName)
+                .collect(Collectors.toSet());
+    }
+
+    final void setConfiguration(Provider<Configuration> config) {
+        this.productDependenciesConfig.set(config);
+    }
 
     @OutputFile
     public abstract RegularFileProperty getOutputFile();
@@ -205,58 +254,93 @@ private Set<ProductDependency> dedupDiscoveredProductDependencies() {
     }
 
     private List<ProductDependency> findRecommendedProductDependenies() {
-        // This will find both intra-project and third party artifacts because the project artifacts are resolved to
-        // their generated jar files.
-        Stream<ResolvedArtifact> artifactStream =
-                getProductDependenciesConfig()
-                        .get()
-                        .getResolvedConfiguration()
-                        .getFirstLevelModuleDependencies()
-                        .stream()
-                        .map(ResolvedDependency::getAllModuleArtifacts)
-                        .flatMap(Collection::stream)
-                        .filter(a -> "jar".equals(a.getExtension()))
-                        .distinct();
+        return productDependenciesConfig.get().getIncoming().getArtifacts().getArtifacts().stream()
+                .flatMap(artifact -> {
+                    String artifactName = artifact.getId().getDisplayName();
+                    ComponentIdentifier id = artifact.getId().getComponentIdentifier();
+                    Optional<String> pdeps = Optional.empty();
 
-        return artifactStream
-                .map(ResolveProductDependenciesTask::getProductDependenciesFromArtifact)
-                .flatMap(Collection::stream)
-                .distinct()
-                .collect(Collectors.toList());
-    }
+                    // Extract product dependencies directly from Jar task for in project dependencies
+                    if (id instanceof ProjectComponentIdentifier) {
+                        Project dependencyProject = getProject()
+                                .getRootProject()
+                                .project(((ProjectComponentIdentifier) id).getProjectPath());
+                        if (dependencyProject.getPlugins().hasPlugin(JavaPlugin.class)) {
+                            Jar jar = (Jar) dependencyProject.getTasks().getByName(JavaPlugin.JAR_TASK_NAME);
 
-    private static Collection<ProductDependency> getProductDependenciesFromArtifact(ResolvedArtifact artifact) {
-        File jarFile = artifact.getFile();
-        try (ZipFile zipFile = new ZipFile(jarFile)) {
-            String artifactName = artifact.getId().getDisplayName();
-            ZipEntry manifestEntry = zipFile.getEntry("META-INF/MANIFEST.MF");
-            if (manifestEntry == null) {
-                return Collections.emptySet();
-            }
-            Attributes attrs = new Manifest(zipFile.getInputStream(manifestEntry)).getMainAttributes();
-            if (!attrs.containsKey(RecommendedProductDependencies.SLS_RECOMMENDED_PRODUCT_DEPS_ATTRIBUTE)) {
-                return Collections.emptySet();
-            }
+                            pdeps = Optional.ofNullable(jar.getManifest()
+                                            .getEffectiveManifest()
+                                            .getAttributes()
+                                            .get(RecommendedProductDependencies.SLS_RECOMMENDED_PRODUCT_DEPS_KEY))
+                                    .map(Object::toString);
+                        }
+                    } else {
+                        if (!artifact.getFile().exists()) {
+                            log.debug("Artifact did not exist: {}", artifact.getFile());
+                            return Stream.empty();
+                        } else if (!com.google.common.io.Files.getFileExtension(
+                                        artifact.getFile().getName())
+                                .equals("jar")) {
+                            log.debug("Artifact is not jar: {}", artifact.getFile());
+                            return Stream.empty();
+                        }
 
-            Set<ProductDependency> recommendedDeps = Serializations.jsonMapper
-                    .readValue(
-                            attrs.getValue(RecommendedProductDependencies.SLS_RECOMMENDED_PRODUCT_DEPS_ATTRIBUTE),
-                            RecommendedProductDependencies.class)
-                    .recommendedProductDependencies();
-            if (recommendedDeps.isEmpty()) {
-                return Collections.emptySet();
-            }
-            log.info(
-                    "Product dependency recommendation made by artifact '{}', file '{}', "
-                            + "dependency recommendation '{}'",
-                    artifactName,
-                    artifact,
-                    recommendedDeps);
-            return recommendedDeps;
-        } catch (IOException e) {
-            log.warn("Failed to load product dependency for artifact '{}', file '{}', '{}'", artifact, jarFile, e);
-            return Collections.emptySet();
-        }
+                        Manifest manifest;
+                        try {
+                            ZipFile zipFile = new ZipFile(artifact.getFile());
+                            ZipEntry manifestEntry = zipFile.getEntry("META-INF/MANIFEST.MF");
+                            if (manifestEntry == null) {
+                                log.debug("Manifest file does not exist in jar for '${coord}'");
+                                return Stream.empty();
+                            }
+                            manifest = new Manifest(zipFile.getInputStream(manifestEntry));
+                        } catch (IOException e) {
+                            log.warn(
+                                    "IOException encountered when processing artifact '{}', file '{}', {}",
+                                    artifactName,
+                                    artifact.getFile(),
+                                    e);
+                            return Stream.empty();
+                        }
+
+                        pdeps = Optional.ofNullable(manifest.getMainAttributes()
+                                .getValue(RecommendedProductDependencies.SLS_RECOMMENDED_PRODUCT_DEPS_KEY));
+                    }
+                    if (!pdeps.isPresent()) {
+                        log.debug(
+                                "No product dependency found for artifact '{}', file '{}'",
+                                artifactName,
+                                artifact.getFile());
+                        return Stream.empty();
+                    }
+
+                    try {
+                        RecommendedProductDependencies recommendedDeps =
+                                Serializations.jsonMapper.readValue(pdeps.get(), RecommendedProductDependencies.class);
+                        return recommendedDeps.recommendedProductDependencies().stream()
+                                .peek(productDependency -> log.info(
+                                        "Product dependency recommendation made by artifact '{}', file '{}', "
+                                                + "dependency recommendation '{}'",
+                                        artifactName,
+                                        artifact,
+                                        productDependency))
+                                .map(recommendedDep -> new ProductDependency(
+                                        recommendedDep.getProductGroup(),
+                                        recommendedDep.getProductName(),
+                                        recommendedDep.getMinimumVersion(),
+                                        recommendedDep.getMaximumVersion(),
+                                        recommendedDep.getRecommendedVersion(),
+                                        recommendedDep.getOptional()));
+                    } catch (IOException | IllegalArgumentException e) {
+                        log.debug(
+                                "Failed to load product dependency for artifact '{}', file '{}', '{}'",
+                                artifactName,
+                                artifact,
+                                e);
+                        return Stream.empty();
+                    }
+                })
+                .collect(Collectors.toList());
     }
 
     private boolean isNotSelfProductDependency(ProductDependency dependency) {

gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/tasks/CreateManifestIntegrationSpec.groovy

@@ -185,6 +185,58 @@ class CreateManifestIntegrationSpec extends GradleIntegrationSpec {
         file('bar-server/product-dependencies.lock').text.contains 'com.palantir.group:foo-service ($projectVersion, 1.x.x)'
     }
 
+    def "createManifest does not force compilation of sibling projects"() {
+        setup:
+        buildFile << """
+        allprojects {
+            project.version = '1.0.0'
+            group "com.palantir.group"
+        }
+        """
+        helper.addSubproject("foo-api", """
+            apply plugin: 'java'
+            apply plugin: 'com.palantir.sls-recommended-dependencies'
+
+            recommendedProductDependencies {
+                productDependency {
+                    productGroup = 'com.palantir.group'
+                    productName = 'foo-service'
+                    minimumVersion = '0.0.0'
+                    maximumVersion = '1.x.x'
+                    recommendedVersion = rootProject.version
+                }
+            }
+        """.stripIndent())
+        helper.addSubproject("foo-server", """
+            apply plugin: 'com.palantir.sls-java-service-distribution'
+
+            dependencies {
+                compile project(':foo-api')
+            }
+
+            distribution {
+                serviceGroup 'com.palantir.group'
+                serviceName 'foo-service'
+                mainClass 'com.palantir.foo.bar.MyServiceMainClass'
+                args 'server', 'var/conf/my-service.yml'
+            }
+        """.stripIndent())
+
+        when:
+        def result = runTasks('foo-server:createManifest')
+
+        then:
+        result.task(":foo-server:createManifest").outcome == TaskOutcome.SUCCESS
+        result.task(":foo-api:configureProductDependencies").outcome == TaskOutcome.SUCCESS
+        result.task(':foo-api:jar') == null
+        result.tasks.collect({ it.path }).toSet() == ImmutableSet.of(
+                ":foo-api:configureProductDependencies",
+                ":foo-api:processResources",
+                ":foo-server:mergeDiagnosticsJson",
+                ":foo-server:resolveProductDependencies",
+                ":foo-server:createManifest")
+    }
+
     def "check depends on createManifest"() {
         when:
         def result = runTasks(':check')

gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/tasks/ResolveProductDependenciesTaskIntegrationSpec.groovy

@@ -395,6 +395,7 @@ class ResolveProductDependenciesTaskIntegrationSpec extends GradleIntegrationSpe
 
         then:
         result.task(":foo-server:resolveProductDependencies").outcome == TaskOutcome.SUCCESS
+        result.task(':bar-api:jar') == null
 
         List<ProductDependency> prodDeps = readReport()
         prodDeps.size() == 1
@@ -450,6 +451,7 @@ class ResolveProductDependenciesTaskIntegrationSpec extends GradleIntegrationSpe
 
         then:
         result.task(":foo-server:resolveProductDependencies").outcome == TaskOutcome.SUCCESS
+        result.task(':bar-api:jar') == null
         List<ProductDependency> prodDeps = readReport()
         prodDeps.size() == 1
         prodDeps[0] == new ProductDependency('com.palantir.group', 'bar-service', '0.0.0', '1.x.x', '1.0.0')