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

[MBUILDCACHE-103] Allow incremental restore in case of plugin parameter mismatch #177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@

import java.io.File;
import java.nio.file.Path;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -130,14 +133,14 @@ public void execute(
}

boolean restorable = result.isSuccess() || result.isPartialSuccess();
boolean restored = false; // if partially restored need to save increment
CacheRestorationStatus restorationStatus =
CacheRestorationStatus.FAILURE; // if partially restored need to save increment
if (restorable) {
CacheRestorationStatus cacheRestorationStatus =
restoreProject(result, mojoExecutions, mojoExecutionRunner, cacheConfig);
restored = CacheRestorationStatus.SUCCESS == cacheRestorationStatus;
executeExtraCleanPhaseIfNeeded(cacheRestorationStatus, cleanPhase, mojoExecutionRunner);
restorationStatus = restoreProject(result, mojoExecutions, mojoExecutionRunner, cacheConfig);
executeExtraCleanPhaseIfNeeded(restorationStatus, cleanPhase, mojoExecutionRunner);
}
if (!restored) {
if (restorationStatus != CacheRestorationStatus.SUCCESS
&& restorationStatus != CacheRestorationStatus.INCREMENTAL_SUCCESS) {
for (MojoExecution mojoExecution : mojoExecutions) {
if (source == Source.CLI
|| mojoExecution.getLifecyclePhase() == null
Expand All @@ -147,7 +150,8 @@ public void execute(
}
}

if (cacheState == INITIALIZED && (!result.isSuccess() || !restored)) {
if (cacheState == INITIALIZED
&& (!result.isSuccess() || restorationStatus != CacheRestorationStatus.SUCCESS)) {
if (cacheConfig.isSkipSave()) {
LOGGER.info("Cache saving is disabled.");
} else if (cacheConfig.isMandatoryClean()
Expand Down Expand Up @@ -228,20 +232,35 @@ private CacheRestorationStatus restoreProject(
// Verify cache consistency for cached mojos
LOGGER.debug("Verify consistency on cached mojos");
Set<MojoExecution> forcedExecutionMojos = new HashSet<>();
Set<MojoExecution> reconciliationExecutionMojos = new HashSet<>();
for (MojoExecution cacheCandidate : cachedSegment) {
if (cacheController.isForcedExecution(project, cacheCandidate)) {
forcedExecutionMojos.add(cacheCandidate);
} else {
if (!reconciliationExecutionMojos.isEmpty()) {
Copy link
Contributor

@AlexanderAshitkin AlexanderAshitkin Aug 27, 2024

Choose a reason for hiding this comment

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

what is this check for?

reconciliationExecutionMojos.add(cacheCandidate);
continue;
}
if (!verifyCacheConsistency(
cacheCandidate, build, project, session, mojoExecutionRunner, cacheConfig)) {
LOGGER.info("A cached mojo is not consistent, continuing with non cached build");
return CacheRestorationStatus.FAILURE;
if (!cacheConfig.isIncrementalReconciliationOnParameterMismatch()) {
LOGGER.info("A cached mojo is not consistent, continuing with non cached build");
return CacheRestorationStatus.FAILURE;
} else {
LOGGER.info("A cached mojo is not consistent, will reconciliate from here");
reconciliationExecutionMojos.add(cacheCandidate);
}
}
}
}

Set<MojoExecution> plannedExecutions = Stream.concat(
forcedExecutionMojos.stream(), reconciliationExecutionMojos.stream())
.collect(Collectors.toSet());
// Restore project artifacts
ArtifactRestorationReport restorationReport = cacheController.restoreProjectArtifacts(cacheResult);
ArtifactRestorationReport restorationReport = cacheController.restoreProjectArtifacts(
cacheResult,
!containsExecution(plannedExecutions, "org.apache.maven.plugins", "maven-jar-plugin", "jar"));
Copy link
Contributor

Choose a reason for hiding this comment

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

this hardcoded list of plugins doesnt look good and manifests a workaround which might not work in general case. What is it for?

if (!restorationReport.isSuccess()) {
LOGGER.info("Cannot restore project artifacts, continuing with non cached build");
return restorationReport.isRestoredFilesInProjectDirectory()
Expand All @@ -267,6 +286,12 @@ private CacheRestorationStatus restoreProject(
// mojoExecutionScope.seed(
// org.apache.maven.api.MojoExecution.class, new DefaultMojoExecution(cacheCandidate));
mojoExecutionRunner.run(cacheCandidate);
} else if (reconciliationExecutionMojos.contains(cacheCandidate)) {
Copy link
Contributor

@AlexanderAshitkin AlexanderAshitkin Aug 27, 2024

Choose a reason for hiding this comment

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

I'm not convinced that this logic is valid. If you have build 1 with the plugins [(X, parameters PX1), (Y, parameters PX1)], with the flag "on",the new cached build might be a result of [(X, parameters PX2), (Y, parameters PY1)]. The issue here is that without tracking inputs/outputs, we can't guarantee that reusing PY1 is valid if the first plugin changed parameters. It's possible that it relies on outputs from the first plugin and could yield a different result without the cache. This logic seems flawed to me at the moment. It might lead to all sorts of corruptions inadvertently. it probably could run for certain plugins in presence of additional metadata, but not in a general case.

This optimization could work for a leaf plugins (plugins which do not contribute and do not impact subsequent plugins) but that will require additional plugin metadata

LOGGER.info(
"Mojo execution is needed for reconciliation: {}",
cacheCandidate.getMojoDescriptor().getFullGoalName());
mojoExecutionScope.seed(MojoExecution.class, cacheCandidate);
mojoExecutionRunner.run(cacheCandidate);
} else {
LOGGER.info(
"Skipping plugin execution (cached): {}",
Expand Down Expand Up @@ -295,12 +320,34 @@ private CacheRestorationStatus restoreProject(
for (MojoExecution mojoExecution : postCachedSegment) {
mojoExecutionRunner.run(mojoExecution);
}
return CacheRestorationStatus.SUCCESS;

if (reconciliationExecutionMojos.isEmpty()) {
return CacheRestorationStatus.SUCCESS;
} else {
return CacheRestorationStatus.INCREMENTAL_SUCCESS;
}
} finally {
mojoExecutionScope.exit();
}
}

private boolean containsExecution(
Collection<MojoExecution> executions, String groupId, String artifactId, String goal) {
for (MojoExecution execution : executions) {
if (!groupId.equals(execution.getGroupId())) {
continue;
}
if (!artifactId.equals(execution.getArtifactId())) {
continue;
}
if (!goal.equals(execution.getGoal())) {
continue;
}
return true;
}
return false;
}

private boolean verifyCacheConsistency(
MojoExecution cacheCandidate,
Build cachedBuild,
Expand All @@ -320,9 +367,7 @@ private boolean verifyCacheConsistency(
final String fullGoalName = cacheCandidate.getMojoDescriptor().getFullGoalName();

if (completedExecution != null && !isParamsMatched(project, cacheCandidate, mojo, completedExecution)) {
LOGGER.info(
"Mojo cached parameters mismatch with actual, forcing full project build. Mojo: {}",
fullGoalName);
LOGGER.info("Mojo cached parameters mismatch with actual. Mojo: {}", fullGoalName);
consistent = false;
}

Expand Down Expand Up @@ -433,6 +478,7 @@ private static String normalizedPath(Path path, Path baseDirPath) {

private enum CacheRestorationStatus {
SUCCESS,
INCREMENTAL_SUCCESS,
FAILURE,
FAILURE_NEEDS_CLEAN
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public interface CacheController {
CacheResult findCachedBuild(
MavenSession session, MavenProject project, List<MojoExecution> mojoExecutions, boolean skipCache);

ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult);
ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult, boolean setProjectArtifact);

void save(
CacheResult cacheResult,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ private void verifyRestorationInsideProject(final MavenProject project, Path pat
}

@Override
public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult) {
public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult, boolean setProjectArtifact) {

LOGGER.debug("Restore project artifacts");
final Build build = cacheResult.getBuildInfo();
Expand Down Expand Up @@ -412,7 +412,9 @@ public ArtifactRestorationReport restoreProjectArtifacts(CacheResult cacheResult
// Actually modify project at the end in case something went wrong during restoration,
// in which case, the project is unmodified and we continue with normal build.
if (restoredProjectArtifact != null) {
project.setArtifact(restoredProjectArtifact);
if (setProjectArtifact) {
project.setArtifact(restoredProjectArtifact);
}
// need to include package lifecycle to save build info for incremental builds
if (!project.hasLifecyclePhase("package")) {
project.addLifecyclePhase("package");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,6 @@ public interface CacheConfig {
* Flag to save in cache only if a build went through the clean lifecycle
*/
boolean isMandatoryClean();

boolean isIncrementalReconciliationOnParameterMismatch();
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ public class CacheConfigImpl implements org.apache.maven.buildcache.xml.CacheCon
public static final String RESTORE_GENERATED_SOURCES_PROPERTY_NAME = "maven.build.cache.restoreGeneratedSources";
public static final String ALWAYS_RUN_PLUGINS = "maven.build.cache.alwaysRunPlugins";
public static final String MANDATORY_CLEAN = "maven.build.cache.mandatoryClean";
public static final String INCREMENTAL_RECONCILIATION_ON_PARAMETER_MISMATCH =
"maven.build.cache.incrementalReconciliationOnParameterMismatch";

/**
* Flag to control if we should skip lookup for cached artifacts globally or for a particular project even if
Expand Down Expand Up @@ -538,6 +540,13 @@ public boolean isMandatoryClean() {
return getProperty(MANDATORY_CLEAN, getConfiguration().isMandatoryClean());
}

@Override
public boolean isIncrementalReconciliationOnParameterMismatch() {
return getProperty(
INCREMENTAL_RECONCILIATION_ON_PARAMETER_MISMATCH,
getConfiguration().isIncrementalReconciliationOnParameterMismatch());
}

@Override
public String getId() {
checkInitializedState();
Expand Down
8 changes: 8 additions & 0 deletions src/main/mdo/build-cache-config.mdo
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,14 @@ under the License.
<description>FileHash (causes file hash is saved in build metadata) or
EffectivePom (causes effective pom info is saved in build metadata)</description>
</field>
<field>
<name>incrementalReconciliationOnParameterMismatch</name>
<type>boolean</type>
<defaultValue>false</defaultValue>
<description>If true, on plugin execution parameter mismatch, the build will try to complete by
running the mismatched execution and executions that follow. If false, on plugin execution parameter
mismatch, a full build will be performed.</description>
</field>
</fields>
</class>

Expand Down
72 changes: 72 additions & 0 deletions src/test/java/org/apache/maven/buildcache/its/Issue103Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.maven.buildcache.its;

import java.util.Arrays;

import org.apache.maven.buildcache.its.junit.IntegrationTest;
import org.apache.maven.it.VerificationException;
import org.apache.maven.it.Verifier;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import static org.apache.maven.buildcache.util.LogFileUtils.findFirstLineContainingTextsInLogs;

/**
* @author Réda Housni Alaoui
*/
@IntegrationTest("src/test/projects/mbuildcache-103")
class Issue103Test {

private static final String PROJECT_NAME = "org.apache.maven.caching.test.mbuildcache-103:simple";

@Test
void simple(Verifier verifier) throws VerificationException {
verifier.setAutoclean(false);

verifier.getCliOptions().clear();
verifier.addCliOption("-Dmaven.build.cache.incrementalReconciliationOnParameterMismatch");
verifier.addCliOption("-DskipTests");
verifier.setLogFileName("../log-1.txt");
verifier.executeGoals(Arrays.asList("clean", "verify"));
verifier.verifyErrorFreeLog();

verifier.getCliOptions().clear();
verifier.addCliOption("-Dmaven.build.cache.incrementalReconciliationOnParameterMismatch");
verifier.setLogFileName("../log-2.txt");
verifier.executeGoals(Arrays.asList("clean", "verify"));
verifier.verifyErrorFreeLog();
verifier.verifyTextInLog("No tests to run.");
verifier.verifyTextInLog("Building jar");
verifier.verifyTextInLog("Saved Build to local file");
verifyNoTextInLog(verifier, "A cached mojo is not consistent, continuing with non cached build");

verifier.getCliOptions().clear();
verifier.addCliOption("-Dmaven.build.cache.incrementalReconciliationOnParameterMismatch");
verifier.setLogFileName("../log-3.txt");
verifier.executeGoals(Arrays.asList("clean", "verify"));
verifier.verifyErrorFreeLog();
verifier.verifyTextInLog("Found cached build, restoring " + PROJECT_NAME + " from cache by");
verifier.verifyTextInLog("Skipping plugin execution (cached): surefire:test");
}

private static void verifyNoTextInLog(Verifier verifier, String text) throws VerificationException {
Assertions.assertNull(findFirstLineContainingTextsInLogs(verifier, text));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?xml version="1.0" encoding="UTF-8" ?>

<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->

<cache xmlns="http://maven.apache.org/BUILD-CACHE-CONFIG/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/BUILD-CACHE-CONFIG/1.0.0 https://maven.apache.org/xsd/build-cache-config-1.0.0.xsd">
<executionControl>
<reconcile>
<plugins>
<plugin artifactId="maven-surefire-plugin" goal="test">
<reconciles>
<reconcile propertyName="skipTests" skipValue="true"/>
</reconciles>
</plugin>
</plugins>
</reconcile>
</executionControl>
</cache>
42 changes: 42 additions & 0 deletions src/test/projects/mbuildcache-103/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<!--

Copyright 2021 the original author or authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

-->
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">

<modelVersion>4.0.0</modelVersion>
<groupId>org.apache.maven.caching.test.mbuildcache-103</groupId>
<artifactId>simple</artifactId>
<version>0.0.1-SNAPSHOT</version>
<packaging>jar</packaging>

<properties>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</maven.compiler.target>
</properties>

<build>
<extensions>
<extension>
<groupId>org.apache.maven.extensions</groupId>
<artifactId>maven-build-cache-extension</artifactId>
<version>${projectVersion}</version>
</extension>
</extensions>
</build>

</project>
Loading