From 09fc4a2869b7bb0c54033197c506a1f120fdd593 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Fri, 27 Oct 2023 09:27:22 +0200 Subject: [PATCH 1/3] [MRESOLVER-392] Remove change detection logic on install Still, make possible to restore the "skip if missing" as I find no reasonong for it, but my guess is that it may be about some legacy code somewhere, most probably adding Metadata without backing file (as in Maven2 metadata was added by hand). --- https://issues.apache.org/jira/browse/MRESOLVER-392 --- .../internal/impl/DefaultInstaller.java | 34 ++++++++++++++----- .../internal/impl/DefaultInstallerTest.java | 17 ++++++++++ 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java index 6f24fc5bd..6cc203ea3 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java @@ -50,6 +50,7 @@ import org.eclipse.aether.repository.LocalRepositoryManager; import org.eclipse.aether.spi.io.FileProcessor; import org.eclipse.aether.spi.synccontext.SyncContextFactory; +import org.eclipse.aether.util.ConfigUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -61,6 +62,22 @@ @Named public class DefaultInstaller implements Installer { + /** + * The key in the repository session's {@link RepositorySystemSession#getConfigProperties() + * configuration properties} used to restore legacy 1.x behaviour: ignore install request that has source + * file missing. + * + * @since 2.0.0 + */ + static final String CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL = "aether.installer.ignoreMissingFileInstall"; + + /** + * The default value for {@link #CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL}, {@code false}. + * + * @since 2.0.0 + */ + static final boolean CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL_DEFAULT = false; + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultInstaller.class); private final FileProcessor fileProcessor; @@ -95,6 +112,9 @@ public InstallResult install(RepositorySystemSession session, InstallRequest req private InstallResult install(SyncContext syncContext, RepositorySystemSession session, InstallRequest request) throws InstallationException { + boolean ignoreMissingFileInstall = ConfigUtils.getBoolean( + session, CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL_DEFAULT, CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL); + InstallResult result = new InstallResult(request); RequestTrace trace = RequestTrace.newChild(request.getTrace(), request); @@ -124,7 +144,7 @@ private InstallResult install(SyncContext syncContext, RepositorySystemSession s iterator.set(artifact); - install(session, trace, artifact); + install(ignoreMissingFileInstall, session, trace, artifact); result.addArtifact(artifact); } @@ -165,7 +185,8 @@ private List getMetadataGenerators( return generators; } - private void install(RepositorySystemSession session, RequestTrace trace, Artifact artifact) + private void install( + boolean ignoreMissingFileInstall, RepositorySystemSession session, RequestTrace trace, Artifact artifact) throws InstallationException { final LocalRepositoryManager lrm = session.getLocalRepositoryManager(); final File srcFile = artifact.getFile(); @@ -179,13 +200,8 @@ private void install(RepositorySystemSession session, RequestTrace trace, Artifa throw new IllegalStateException("cannot install " + dstFile + " to same path"); } - boolean copy = "pom".equals(artifact.getExtension()) - || srcFile.lastModified() != dstFile.lastModified() - || srcFile.length() != dstFile.length() - || !srcFile.exists(); - - if (!copy) { - LOGGER.debug("Skipped re-installing {} to {}, seems unchanged", srcFile, dstFile); + if (ignoreMissingFileInstall && !srcFile.exists()) { + LOGGER.debug("Skipped installing {} to {}, source is missing", srcFile, dstFile); } else { fileProcessor.copy(srcFile, dstFile); dstFile.setLastModified(srcFile.lastModified()); diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java index 081dab648..02f3f0fba 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java @@ -40,6 +40,7 @@ import org.eclipse.aether.metadata.Metadata.Nature; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.*; @@ -324,6 +325,7 @@ private void checkEvents(String msg, Artifact artifact, boolean failed) { } @Test + @Disabled("Change detection is removed") void testDoNotUpdateUnchangedArtifact() throws InstallationException { request.addArtifact(artifact); installer.install(session, request); @@ -344,6 +346,21 @@ public long copy(File src, File target, ProgressListener listener) throws IOExce installer.install(session, request); } + @Test + void testMissingSourceFails() { + artifact = artifact.setFile(new File("nonexistent artifact")); + request.addArtifact(artifact); + assertThrows(InstallationException.class, () -> installer.install(session, request)); + } + + @Test + void testMissingSourceSilentlySkippedIfSet() throws InstallationException { + session.setConfigProperty(DefaultInstaller.CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL, true); + artifact = artifact.setFile(new File("nonexistent artifact")); + request.addArtifact(artifact); + installer.install(session, request); + } + @Test void testSetArtifactTimestamps() throws InstallationException { artifact.getFile().setLastModified(artifact.getFile().lastModified() - 60000); From 4949cd9b6ee9535b82bbd6214b5dd9bb8751fcc4 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Fri, 27 Oct 2023 10:03:46 +0200 Subject: [PATCH 2/3] Add ref to original issue --- .../org/eclipse/aether/internal/impl/DefaultInstallerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java index 02f3f0fba..96ba4cd70 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java @@ -325,7 +325,7 @@ private void checkEvents(String msg, Artifact artifact, boolean failed) { } @Test - @Disabled("Change detection is removed") + @Disabled("Naive change detection is removed (MRESOLVER-392)") void testDoNotUpdateUnchangedArtifact() throws InstallationException { request.addArtifact(artifact); installer.install(session, request); From a021596a56ebbb5485cf44a6126d047ca464ec0d Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Fri, 27 Oct 2023 10:19:22 +0200 Subject: [PATCH 3/3] Fix wrong interpretation of existing code --- .../internal/impl/DefaultInstaller.java | 39 ++----------------- .../internal/impl/DefaultInstallerTest.java | 15 ------- 2 files changed, 4 insertions(+), 50 deletions(-) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java index 6cc203ea3..f4c60bf3f 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java @@ -50,9 +50,6 @@ import org.eclipse.aether.repository.LocalRepositoryManager; import org.eclipse.aether.spi.io.FileProcessor; import org.eclipse.aether.spi.synccontext.SyncContextFactory; -import org.eclipse.aether.util.ConfigUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import static java.util.Objects.requireNonNull; @@ -61,25 +58,6 @@ @Singleton @Named public class DefaultInstaller implements Installer { - - /** - * The key in the repository session's {@link RepositorySystemSession#getConfigProperties() - * configuration properties} used to restore legacy 1.x behaviour: ignore install request that has source - * file missing. - * - * @since 2.0.0 - */ - static final String CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL = "aether.installer.ignoreMissingFileInstall"; - - /** - * The default value for {@link #CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL}, {@code false}. - * - * @since 2.0.0 - */ - static final boolean CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL_DEFAULT = false; - - private static final Logger LOGGER = LoggerFactory.getLogger(DefaultInstaller.class); - private final FileProcessor fileProcessor; private final RepositoryEventDispatcher repositoryEventDispatcher; @@ -112,9 +90,6 @@ public InstallResult install(RepositorySystemSession session, InstallRequest req private InstallResult install(SyncContext syncContext, RepositorySystemSession session, InstallRequest request) throws InstallationException { - boolean ignoreMissingFileInstall = ConfigUtils.getBoolean( - session, CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL_DEFAULT, CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL); - InstallResult result = new InstallResult(request); RequestTrace trace = RequestTrace.newChild(request.getTrace(), request); @@ -144,7 +119,7 @@ private InstallResult install(SyncContext syncContext, RepositorySystemSession s iterator.set(artifact); - install(ignoreMissingFileInstall, session, trace, artifact); + install(session, trace, artifact); result.addArtifact(artifact); } @@ -185,8 +160,7 @@ private List getMetadataGenerators( return generators; } - private void install( - boolean ignoreMissingFileInstall, RepositorySystemSession session, RequestTrace trace, Artifact artifact) + private void install(RepositorySystemSession session, RequestTrace trace, Artifact artifact) throws InstallationException { final LocalRepositoryManager lrm = session.getLocalRepositoryManager(); final File srcFile = artifact.getFile(); @@ -200,13 +174,8 @@ private void install( throw new IllegalStateException("cannot install " + dstFile + " to same path"); } - if (ignoreMissingFileInstall && !srcFile.exists()) { - LOGGER.debug("Skipped installing {} to {}, source is missing", srcFile, dstFile); - } else { - fileProcessor.copy(srcFile, dstFile); - dstFile.setLastModified(srcFile.lastModified()); - } - + fileProcessor.copy(srcFile, dstFile); + dstFile.setLastModified(srcFile.lastModified()); lrm.add(session, new LocalArtifactRegistration(artifact)); } catch (Exception e) { exception = e; diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java index 96ba4cd70..76f15b713 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultInstallerTest.java @@ -346,21 +346,6 @@ public long copy(File src, File target, ProgressListener listener) throws IOExce installer.install(session, request); } - @Test - void testMissingSourceFails() { - artifact = artifact.setFile(new File("nonexistent artifact")); - request.addArtifact(artifact); - assertThrows(InstallationException.class, () -> installer.install(session, request)); - } - - @Test - void testMissingSourceSilentlySkippedIfSet() throws InstallationException { - session.setConfigProperty(DefaultInstaller.CONFIG_PROP_IGNORE_MISSING_FILE_INSTALL, true); - artifact = artifact.setFile(new File("nonexistent artifact")); - request.addArtifact(artifact); - installer.install(session, request); - } - @Test void testSetArtifactTimestamps() throws InstallationException { artifact.getFile().setLastModified(artifact.getFile().lastModified() - 60000);