From a1222a071622358b594c4369c603b850812a61fa Mon Sep 17 00:00:00 2001 From: Patrick Ziegler Date: Sat, 14 Sep 2024 08:55:32 +0200 Subject: [PATCH] Improve errors handling while downloading p2 artifacts from repository In case an artifact couldn't be downloaded due to e.g. a bad mirror, p2 returns an RETRY error code. This code is currently ignored and Tycho simply fails with an I/O exception. Instead, Tycho attempts to download the artifact up to three times (assuming that this error code was returned), before failing. This value can be configured using the 'eclipse.p2.maxDownloadAttempts' system property. (cherry picked from commit 8462a37d62b112b7ff9f40e519c066357fe5808d) --- .../repository/P2RepositoryManager.java | 43 ++++- src/site/markdown/SystemProperties.md | 5 +- .../baseline/artifacts.xml | 20 +++ .../p2Repository.mirror/baseline/content.xml | 34 ++++ .../p2Repository.mirror/baseline/mirrors.xml | 4 + .../baseline/plugins/bundle1_1.0.0.jar | Bin 0 -> 417 bytes .../bundle/META-INF/MANIFEST.MF | 6 + .../bundle/build.properties | 1 + .../p2Repository.mirror/bundle/pom.xml | 45 +++++ .../projects/p2Repository.mirror/pom.xml | 23 +++ .../p2Repository/P2RepositoryMirrorTest.java | 160 ++++++++++++++++++ .../eclipse/tycho/test/util/HttpServer.java | 7 +- 12 files changed, 339 insertions(+), 9 deletions(-) create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/content.xml create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml create mode 100644 tycho-its/projects/p2Repository.mirror/baseline/plugins/bundle1_1.0.0.jar create mode 100644 tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF create mode 100644 tycho-its/projects/p2Repository.mirror/bundle/build.properties create mode 100644 tycho-its/projects/p2Repository.mirror/bundle/pom.xml create mode 100644 tycho-its/projects/p2Repository.mirror/pom.xml create mode 100644 tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java diff --git a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java index 18f7cdfcb4..f6a7e056bc 100644 --- a/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java +++ b/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/repository/P2RepositoryManager.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2022 Christoph Läubrich and others. + * Copyright (c) 2022, 2024 Christoph Läubrich and others. * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 * which accompanies this distribution, and is available at @@ -37,6 +37,7 @@ import org.eclipse.equinox.p2.repository.metadata.IMetadataRepositoryManager; import org.eclipse.tycho.IRepositoryIdManager; import org.eclipse.tycho.MavenRepositoryLocation; +import org.eclipse.tycho.helper.MavenPropertyHelper; import org.eclipse.tycho.p2maven.ListCompositeArtifactRepository; import org.eclipse.tycho.p2maven.ListQueryable; import org.eclipse.tycho.p2maven.LoggerProgressMonitor; @@ -46,6 +47,10 @@ */ @Component(role = P2RepositoryManager.class) public class P2RepositoryManager { + private static final String PROPERTY_KEY = "eclipse.p2.maxDownloadAttempts"; + + @Requirement + MavenPropertyHelper propertyHelper; @Requirement IRepositoryIdManager repositoryIdManager; @@ -151,18 +156,44 @@ private IMetadataRepository getMetadataRepositor(URI location, String id) throws public void downloadArtifact(IInstallableUnit iu, IArtifactRepository artifactRepository, OutputStream outputStream) throws IOException { Collection artifacts = iu.getArtifacts(); + int maxDownloadAttempts = getMaxDownloadAttempts(); + for (IArtifactKey key : artifacts) { IArtifactDescriptor[] descriptors = artifactRepository.getArtifactDescriptors(key); for (IArtifactDescriptor descriptor : descriptors) { - IStatus status = artifactRepository.getRawArtifact(descriptor, outputStream, - new LoggerProgressMonitor(logger)); - if (status.isOK()) { - return; + for (int downloadAttempts = 0; downloadAttempts < maxDownloadAttempts; ++downloadAttempts) { + IStatus status = artifactRepository.getRawArtifact(descriptor, outputStream, + new LoggerProgressMonitor(logger)); + if (status.isOK()) { + return; + } + // Might happen if e.g. a bad mirror was used + if (status.getCode() == IArtifactRepository.CODE_RETRY) { + logger.warn("Artifact repository requested retry (attempt [%d/%d]): '%s'" + .formatted(downloadAttempts + 1, maxDownloadAttempts, status)); + continue; + } + throw new IOException("Download failed: " + status); } - throw new IOException("Download failed: " + status); } } throw new FileNotFoundException(); } + private int getMaxDownloadAttempts() { + String property = propertyHelper.getGlobalProperty(PROPERTY_KEY, "3"); + try { + int maxDownloadAttempts = Integer.valueOf(property); + if (maxDownloadAttempts <= 0) { + logger.error("Value '%s' for property '%s', is not a positive number! Use 1 as default value." + .formatted(property, PROPERTY_KEY)); + return 1; + } + return maxDownloadAttempts; + } catch (NumberFormatException e) { + logger.error("Value '%s' for property '%s', is not a number! Use 1 as default value.".formatted(property, + PROPERTY_KEY)); + return 1; + } + } } diff --git a/src/site/markdown/SystemProperties.md b/src/site/markdown/SystemProperties.md index 7ba6c87471..d89a3e3dc9 100644 --- a/src/site/markdown/SystemProperties.md +++ b/src/site/markdown/SystemProperties.md @@ -23,7 +23,7 @@ tycho.debug.resolver | `true` or _artifactId_ | Enable debug output for the arti ### Baseline compare Name | Value | Default | Documentation ---- | --- | --- +--- | --- | --- | --- tycho.comparator.showDiff | true / false | false | If set to true if text-like files show a unified diff of possible differences in files tycho.comparator.threshold | bytes | 5242880 (~5MB) | gives the number of bytes for content to be compared semantically, larger files will only be compared byte-by-byte @@ -32,6 +32,7 @@ tycho.comparator.threshold | bytes | 5242880 (~5MB) | gives the number of bytes These properties control the behaviour of P2 used by Tycho Name | Value | Default | Documentation ---- | --- | --- +--- | --- | --- | --- eclipse.p2.mirrors | true / false | true | Each p2 site can define a list of artifact repository mirrors, this controls if P2 mirrors should be used. This is independent from configuring mirrors in the maven configuration to be used by Tycho! +eclipse.p2.maxDownloadAttempts | _any positive integer_ | 3 | Describes how often Tycho attempts to re-download an artifact from a p2 repository in case e.g. a bad mirror was used. One can think of this value as the maximum number of mirrors Tycho/p2 will check. diff --git a/tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml b/tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml new file mode 100644 index 0000000000..a24132b9d5 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/baseline/artifacts.xml @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + + diff --git a/tycho-its/projects/p2Repository.mirror/baseline/content.xml b/tycho-its/projects/p2Repository.mirror/baseline/content.xml new file mode 100644 index 0000000000..61f0c290ea --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/baseline/content.xml @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + Bundle-SymbolicName: bundle1 Bundle-Version: 1.0.0 + + + + + + diff --git a/tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml b/tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml new file mode 100644 index 0000000000..79a7bd8601 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/baseline/mirrors.xml @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/tycho-its/projects/p2Repository.mirror/baseline/plugins/bundle1_1.0.0.jar b/tycho-its/projects/p2Repository.mirror/baseline/plugins/bundle1_1.0.0.jar new file mode 100644 index 0000000000000000000000000000000000000000..529608eb31177c61502765ce706e8d7062a53332 GIT binary patch literal 417 zcmWIWW@h1H0D-dsP9b0hl;C8LVeoYgan$wnbJGtE;bdT*zg}sd?m)3p`_G7H5R)~+$ r+y^lZmwzF~F#-)*(&&tA9E$IOCS&n=fHx}}$O%k9xDrS|0&y4sUcqAN literal 0 HcmV?d00001 diff --git a/tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF b/tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF new file mode 100644 index 0000000000..03efca7aa4 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/bundle/META-INF/MANIFEST.MF @@ -0,0 +1,6 @@ +Manifest-Version: 1.0 +Bundle-ManifestVersion: 2 +Bundle-SymbolicName: bundle1 +Bundle-Version: 1.0.0 +Automatic-Module-Name: bundle1 + diff --git a/tycho-its/projects/p2Repository.mirror/bundle/build.properties b/tycho-its/projects/p2Repository.mirror/bundle/build.properties new file mode 100644 index 0000000000..5f22cdd448 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/bundle/build.properties @@ -0,0 +1 @@ +bin.includes = META-INF/ diff --git a/tycho-its/projects/p2Repository.mirror/bundle/pom.xml b/tycho-its/projects/p2Repository.mirror/bundle/pom.xml new file mode 100644 index 0000000000..fea44d8e91 --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/bundle/pom.xml @@ -0,0 +1,45 @@ + + + 4.0.0 + + + p2Repository.mirror + parent + 0.0.1-SNAPSHOT + + + eclipse-plugin + bundle1 + 1.0.0 + + + + + org.eclipse.tycho + tycho-baseline-plugin + ${tycho-version} + + + compare-version-with-baseline + verify + + verify + + + + + + META-INF/maven/**/* + + + + repo + ${baseline} + p2 + + + + + + + diff --git a/tycho-its/projects/p2Repository.mirror/pom.xml b/tycho-its/projects/p2Repository.mirror/pom.xml new file mode 100644 index 0000000000..26040988fa --- /dev/null +++ b/tycho-its/projects/p2Repository.mirror/pom.xml @@ -0,0 +1,23 @@ + + 4.0.0 + + p2Repository.mirror + parent + 0.0.1-SNAPSHOT + pom + + + bundle + + + + + + org.eclipse.tycho + tycho-maven-plugin + ${tycho-version} + true + + + + diff --git a/tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java b/tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java new file mode 100644 index 0000000000..807000e158 --- /dev/null +++ b/tycho-its/src/test/java/org/eclipse/tycho/test/p2Repository/P2RepositoryMirrorTest.java @@ -0,0 +1,160 @@ +/******************************************************************************* + * Copyright (c) 2024 Patrick Ziegler and others. + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Patrick Ziegler - initial API and implementation + *******************************************************************************/ +package org.eclipse.tycho.test.p2Repository; + +import java.io.File; +import java.io.FileWriter; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; +import javax.xml.xpath.XPath; +import javax.xml.xpath.XPathConstants; +import javax.xml.xpath.XPathFactory; + +import org.apache.maven.it.Verifier; +import org.eclipse.jetty.servlet.DefaultServlet; +import org.eclipse.jetty.util.resource.EmptyResource; +import org.eclipse.jetty.util.resource.Resource; +import org.eclipse.tycho.test.AbstractTychoIntegrationTest; +import org.eclipse.tycho.test.util.HttpServer; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NodeList; + +public class P2RepositoryMirrorTest extends AbstractTychoIntegrationTest { + + private HttpServer server; + + @Before + public void startServer() throws Exception { + server = HttpServer.startServer(); + } + + @After + public void stopServer() throws Exception { + server.stop(); + } + + /** + * Tests whether Tycho is able to recover from a bad mirror repository. If + * multiple mirrors are specified for a repository, Tycho might be able to + * continue by requesting an artifact from a different mirror, depending on the + * error code returned by Equinox. + */ + @Test + public void testMirrorWithRetry() throws Exception { + Verifier verifier = getVerifier("p2Repository.mirror", false); + + String baseline = server.addServer("baseline", FirstBaselineRequestFailsServlet.class, + new File(verifier.getBasedir(), "baseline")); + // Two mirrors, to always have at least one that is still good + String mirror1 = server.addServer("mirror1", FirstBaselineRequestFailsServlet.class, + new File(verifier.getBasedir(), "baseline")); + String mirror2 = server.addServer("mirror2", FirstBaselineRequestFailsServlet.class, + new File(verifier.getBasedir(), "baseline")); + String mirrors = baseline + '/' + "mirrors.xml"; + + setMirrorsUrl(new File(verifier.getBasedir(), "baseline/artifacts.xml"), mirrors); + setMirrors(new File(verifier.getBasedir(), "baseline/mirrors.xml"), mirror1, mirror2); + + // The verifier escapes the 'http://localhost' to 'http:/localhost' + verifier.addCliOption("-Dbaseline=" + baseline.replaceAll("//", "////")); + // Force an update of the HttpCache + verifier.addCliOption("-U"); + verifier.executeGoal("verify"); + verifier.verifyErrorFreeLog(); + verifier.verifyTextInLog("Artifact repository requested retry (attempt [1/3]):"); + } + + @Override + protected boolean isDisableMirrors() { + return false; + } + + /** + * Updates the "p2.mirrorsURL" property in the {@code artifacts.xml} file of the + * baseline repository to point to the {@code mirrors.xml} file. + */ + private static void setMirrorsUrl(File artifactsXml, String mirrorsUrl) throws Exception { + Document document = parseDocument(artifactsXml); + + XPath path = XPathFactory.newInstance().newXPath(); + String expression = "repository/properties/property[@name='p2.mirrorsURL']"; + Element node = (Element) path.evaluate(expression, document, XPathConstants.NODE); + + if (node != null) { + node.setAttribute("value", mirrorsUrl); + writeDocument(document, artifactsXml); + } + } + + /** + * Updates the {@link mirrors.xml} file to contain the bad mirror. + */ + private static void setMirrors(File mirrorsXml, String mirror1, String mirror2) throws Exception { + Document document = parseDocument(mirrorsXml); + + XPath path = XPathFactory.newInstance().newXPath(); + String expression = "mirrors/mirror"; + NodeList nodes = (NodeList) path.evaluate(expression, document, XPathConstants.NODESET); + + if (nodes != null) { + ((Element) nodes.item(0)).setAttribute("url", mirror1); + ((Element) nodes.item(1)).setAttribute("url", mirror2); + writeDocument(document, mirrorsXml); + } + } + + public static Document parseDocument(File file) throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + DocumentBuilder builder = factory.newDocumentBuilder(); + return builder.parse(file); + } + + public static void writeDocument(Document document, File file) throws Exception { + TransformerFactory transformerFactory = TransformerFactory.newInstance(); + Transformer transformer = transformerFactory.newTransformer(); + + try (FileWriter writer = new FileWriter(file)) { + DOMSource source = new DOMSource(document); + StreamResult result = new StreamResult(writer); + transformer.transform(source, result); + } + } + + /** + * Helper servlet to simulate an illegal p2 repository. The first time a plugin + * is requested from the remote repository, a 404 is returned. Any further + * requests succeed, to test whether Tycho is able to recover from bad mirrors. + */ + public static class FirstBaselineRequestFailsServlet extends DefaultServlet { + // We don't know which mirror is selected, so anyone can fail + private static boolean firstRequest = true; + + @Override + public Resource getResource(String pathInContext) { + if (firstRequest && pathInContext.startsWith("/plugins/")) { + firstRequest = false; + return EmptyResource.INSTANCE; + } + return super.getResource(pathInContext); + } + } +} diff --git a/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java b/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java index d22a54bb3f..38e5f46644 100644 --- a/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java +++ b/tycho-its/src/test/java/org/eclipse/tycho/test/util/HttpServer.java @@ -24,6 +24,7 @@ import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; +import javax.servlet.Servlet; import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; @@ -169,10 +170,14 @@ public void stop() throws Exception { } public String addServer(String contextName, final File content) { + return addServer(contextName, DefaultServlet.class, content); + } + + public String addServer(String contextName, Class servlet, final File content) { ServletContextHandler context = new ServletContextHandler(contexts, URIUtil.SLASH + contextName); context.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false"); context.setResourceBase(content.getAbsolutePath()); - context.addServlet(DefaultServlet.class, URIUtil.SLASH); + context.addServlet(servlet, URIUtil.SLASH); registerContext(context); return getUrl(contextName); }