Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve race condition with jar manifest manipulation #1150

Merged
merged 4 commits into from
Jun 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1150.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: fix
fix:
description: resolve race condition with jar manifest manipulation
links:
- https://github.com/palantir/sls-packaging/pull/1150
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
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
Expand All @@ -28,6 +30,8 @@
@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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -45,15 +45,11 @@
import org.gradle.api.DefaultTask;
import org.gradle.api.Project;
import org.gradle.api.artifacts.Configuration;
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.artifacts.ResolvedArtifact;
import org.gradle.api.artifacts.ResolvedDependency;
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;
Expand All @@ -63,17 +59,12 @@
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));
}
Expand All @@ -82,48 +73,22 @@ 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.setConfiguration(project.provider(ext::getProductDependenciesConfig));
task.getProductDependenciesConfig().set(configProvider);
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on reviews from @CRogers I had thought that the Provider would automatically set task dependencies. Is that not the case?

Copy link
Contributor Author

@esword esword Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. A property/provider that is the output of task A will create a dependency if it is set to an input property of task B. However, because this provider is not associated with a task, I do not know if the same logic applies. I can say the behavior was different once I added the explicit dependsOn statement - the upstream jar tasks were run.

Copy link
Contributor

@fawind fawind Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be a stupid question, but what does it actually mean to depend on a provider? The provider will be evaluated before the task runs? How is that different from evaluating the provider passed through as a task input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the right answer to this may be that the Configuration is not listed as an Input property to the task. See -

/**
* 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
final Set<String> getProductDependenciesConfig() {
return productDependenciesConfig.get().getIncoming().getResolutionResult().getAllComponents().stream()
.map(ResolvedComponentResult::getId)
.map(ComponentIdentifier::getDisplayName)
.collect(Collectors.toSet());
}

While I didn't set things up this way here (predated me), I have encountered this issue before with in similar situations. I do not know if it is still a problem of if newer versions of gradle have some way to get around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More data - The task has this method to try to enforce the dependencies between the tasks as well -

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;
});
}

This was added with this commit -
3649f1b

I didn't change the functionality of this method when I extracted the task, but it clearly is not doing what we hoped. I have a gradle build scan where configureProductDependencies is starting after resolveProductDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just writing notes to myself - this commit was to try to avoid building jars - b855a77#diff-ffb538d33fb6230122fe54f923c3b269363d3fd26e7f1f38c58b9750aacf9e82R136-R143

It was about a month before the one that added the otherProjectProductDependenciesTasks method above. The problem with all of these is that they assume they have complete knowledge of what could write dependency info into the jar manifest (i.e. the ConfigureProductDependenciesTask). Though I still don't know why I have seen the task ordering difference noted above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline and we'll move forward with this for now. If the overhead by compiling jars becomes too bad, we can look into other options.

});
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();

Expand All @@ -145,22 +110,8 @@ private Provider<FileCollection> otherProjectProductDependenciesTasks() {
@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
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);
}
public abstract Property<Configuration> getProductDependenciesConfig();

@OutputFile
public abstract RegularFileProperty getOutputFile();
Expand Down Expand Up @@ -254,93 +205,58 @@ private Set<ProductDependency> dedupDiscoveredProductDependencies() {
}

private List<ProductDependency> findRecommendedProductDependenies() {
return productDependenciesConfig.get().getIncoming().getArtifacts().getArtifacts().stream()
.flatMap(artifact -> {
String artifactName = artifact.getId().getDisplayName();
ComponentIdentifier id = artifact.getId().getComponentIdentifier();
Optional<String> pdeps = Optional.empty();
// 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();

// 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);

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();
}

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();
}
return artifactStream
.map(ResolveProductDependenciesTask::getProductDependenciesFromArtifact)
.flatMap(Collection::stream)
.distinct()
.collect(Collectors.toList());
}

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();
}
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();
}

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());
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();
}
}

private boolean isNotSelfProductDependency(ProductDependency dependency) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,58 +185,6 @@ 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"() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fawind @esword have we really lost this guarantee? IMO it's pretty critical that running ./gradlew --write-locks remains fast, partly because it gets run a lot locally (e.g. when resolving merge conflicts or cherry picking things) and also gets run a ton on excavator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Dan. Is there another solution? Maybe using variants?

Copy link
Contributor

@ferozco ferozco Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had suggested using variants and artifactTransformers offline. I think a solution similar to what we do for diagnostics could work here.

The only issue I see is that it is more challenging to get access to a manifest than to a file in the resource directory. I think we could work around this though by producing both an entry in the JARs manifest (for backwards compatability) and a file in the resources directory that contained the Pdeps of a given JAR. We could then cleanly re-use the existing transforms (https://github.com/palantir/sls-packaging/blob/develop/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/DiagnosticsManifestPlugin.java#L165-L233) and leverage regular gradle cache validation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that writing the information to resource files is the way to go (it was the original design for the additional conjure information which is now being written), though that won't necessarily clear all of this up, especially if we are also writing to the meta-inf file (because we still have the existing problem).

However, there may be an optimization (a.k.a. hack) we can appy for in-project dependencies for lock files. We do not need to actually compute the minimum version for such when writing lock files - we always write the template variable '$projectVersion'. So if we think of updating locks separately, then we may be able to avoid compiling and writing jars. I'll ponder a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Felipe's idea of also producing a pdeps artifact in addition to writing it in the JAR manifest as this would potentially simplify configuration.

though that won't necessarily clear all of this up, especially if we are also writing to the meta-inf file (because we still have the existing problem).

Wouldn't project dependencies use the same version of sls-packaging though so we could rely on them using the new code path and producing an artifact containing the pdeps that does not require compiling the source?

We do not need to actually compute the minimum version for such when writing lock files - we always write the template variable '$projectVersion'

Maybe I misunderstand but I think we need to know the concrete minimum version for writing the "product-dependencies.lock" file (ref)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in-project dependencies are always changed to the template (ref), unless there is some comparison I am missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to revert this. After this PR, excavator checks will not open PRs if the resulting code does not compile.

Here's an example of a dependency upgrade excavator run on our internal server repo that failed because compileJava was run as part of --write-locks. This run did not open a PR.
https://e.p.b/api/coordinator/checkoutput/c2vrbesi5esaano38i40.stderr

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')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,6 @@ 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
Expand Down Expand Up @@ -451,7 +450,6 @@ 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')
Expand Down