[MDEPLOY-118] - Enable deployment of attached release artifacts if POM is identical #28

mdrie · 2022-09-27T18:38:14Z

See MDEPLOY-118 for motivation.

The challenge was to reliably retrieve the POM from the deployment repository. In order to be
fully compatible with what Maven usually does, org.eclipse.aether.RepositorySystem has to be used.
However, that only offers the method resolveArtifact (and similar), which provides artifacts in the
local repository, does not offer the possibility to specify an alternate location and possibly never even
retrieves them from the remote repository, if present in the local repository.

The solution was to use an alternate local repository via a clone of the RepositorySystemSession just
to resolve the wanted POM into it.

The comparison is done on the level of the XML Model. This was done mainly to address the
"platform-dependent line-break concerns" mentioned in the issue. Alternatives are possible but this seems
to be the one most appropriate for a POM.

=======================

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MPH-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MPH-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java

mdrie · 2022-09-29T15:28:40Z

Thanks for the first workflow run. A non-JAR-file in a target directory within the unit-tests was missing due to ignore rules. Added it with my last force-push.

mdrie · 2022-09-30T10:41:08Z

I think, I am done now. Let me know if anything is missing or needs to be done differently!

Otherwise, I am patiently awaiting a successful workflow run, merge, and new release of the plugin. 😇

mdrie · 2022-10-06T06:39:56Z

@cstamas Could you please approve the workflow run so I might be able to fix remaining issues?

elharo

If you can merge with the latest changes at head and ping me, I can approve the workflow run.

… feature.

mdrie · 2022-12-20T10:49:01Z

@elharo, I rebased my branch onto current master. Could you, please, approve the workflow run?

Sorry for slow response. I must have overlooked the email.

mdrie · 2023-05-17T09:21:53Z

Is there any chance for this to be merged any time soon? In that case, I would try to find the time to resolve the current conflicts.

elharo · 2023-05-17T10:43:01Z

It can be considered. I'm personally not sure this is a good idea. This seems like it might break some Maven repos stare decisis properties. It's generally not possible to change an artifact after it's been uploaded, and that's worth keeping. The two questions I have are:

Is adding a new file a "change" in this sense?
Does this patch allow not just adding but replacing existing files with something different?

mdrie · 2023-05-17T11:02:41Z

Thanks, @elharo, for the quick response!

I ran into the problem described in #MDEPLOY-118.

I think, I implemented this in a way, that current behavior of the plugin is unchanged if no additional parameters are set. If the feature is enabled, artifacts are still never overwritten. Only additional artifacts are attached to an existing POM.

To your questions:

How else would it be possible to deploy something built on several platforms?
Replacing was done be the deploy plugin before - unless repos prohibited it. Now, the plugin can add artifacts, while the repo ensures, existing artifacts (including the POM) are never changed.

I guess I kinda asked my question at the wrong time for my schedule. Sorry. If you would be interested in more discussion and an eventual merge, I would try to find time for the conflicts, more questions and fixes in about a week from now. I guess, I need to have a closer look myself after such a long time.

elharo · 2023-05-17T12:16:37Z

Ideally things wouldn't be built on several platforms but if they were, the binaries could be copied from one machine to another before deployment. As soon as a build process involves different machines and OS's, I don't see anyway to avoid assembling files from several machines. The only question is whether you do it before the one and only deployment, or piece by piece as different machines upload to the repo. I can see why we might want to insist that a single bundle is provided to the repo once, rather than adding to it over time.

mdrie · 2023-05-17T14:46:03Z

Could you explain, why you might want to insist?

In the company I was doing this for, it is regular practice to publish artifacts for the same version for additional platforms when needed. (...and when build hosts became available.) Building a new version possibly from a stability branch every time some minor RHEL version when some user needed it would be quite impractical for them.

elharo · 2023-05-17T17:03:43Z

Probably worth discussing on the dev list to see what everyone thinks. Maven has gotten a lot of mileage out of the principle that an "artifact" is uniquely identified by coordinates, without any time dependence.

mdrie · 2023-05-23T09:17:26Z

Done. Hope my wording makes sense to you.

KemalSoysal · 2023-05-23T11:12:23Z

CVEs and their possible additional artifacts to a maven coordinate would be a great benefit.

elharo · 2023-05-23T11:15:55Z

Not sure what CVEs has to do with anything. We absolutely do NOT want to address CVEs with the same pom.xml. A fix for a CVE should and MUST have a new version so it can be clearly distinguished which version is in use and whether it's vulnerable or not.

KemalSoysal · 2023-05-23T11:17:47Z

Not sure what CVEs has to do with anything. We absolutely do NOT want to address CVEs with the same pom.xml. A fix for a CVE should and MUST have a new version so it can be clearly distinguished which version is in use and whether it's vulnerable or not.

Well, how do you want to mark a CVE in the original problematic coordinate?
Why don't we mark a coordinate vulnerable if it is?
I agree with the fixing version, that has to be a new coordinate

elharo · 2023-05-23T11:20:20Z

CVEs are not tracked in the repo.

KemalSoysal · 2023-05-23T11:20:43Z

CVEs are not tracked in the repo.

yes, that is unlucky

elharo · 2023-05-23T13:03:14Z

src/it/compare-pom/setup.bsh

+import org.codehaus.plexus.util.*;
+
+File file = new File( localRepositoryPath, "org/apache/maven/its/deploy/comparepom" );
+System.out.println( "Deleting " + file );


You shouldn't need to print debug messages in tests.

elharo · 2023-05-23T13:03:19Z

src/it/compare-pom/setup.bsh

+FileUtils.deleteDirectory( file );
+
+file = new File( basedir, "repo" );
+System.out.println( "Deleting " + file );


elharo · 2023-05-23T13:04:08Z

src/it/compare-pom/verify.groovy

+LogInspector li = new LogInspector( buildLog )
+String groupUrl = "file:///${basedir}/repo/org/apache/maven/its/deploy/comparepom"
+
+// 1st and 2nd run: The POM tried to be downloaded and uploaded:


Log messages are not reliable over time. Tests should not use them to inspect behavior.

elharo · 2023-05-23T13:05:03Z

src/main/java/org/apache/maven/plugins/deploy/TempLocalRepoSession.java

+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.codehaus.plexus.util.FileUtils;


prefer Apache commons-io for this functionality

elharo · 2023-05-23T13:05:42Z

src/main/java/org/apache/maven/plugins/deploy/TempLocalRepoSession.java

+    private final RepositorySystemSession origSession;
+    private final RepositoryCache cache;
+    private final File tempBasedir;
+    private LocalRepositoryManager lrm;


avoid abbreviation

elharo · 2023-05-23T13:05:48Z

src/main/java/org/apache/maven/plugins/deploy/TempLocalRepoSession.java

+ */
+class TempLocalRepoSession extends AbstractForwardingRepositorySystemSession implements Closeable
+{
+    private final RepositorySystemSession origSession;


orig --> original

mdrie · 2023-06-09T08:51:34Z

Hey @elharo, thanks for your review. Can I assume you would agree to a merge, when I fix these issues and resolve the conflicts? I will not have the time for that in the next weeks, but maybe over the summer, though.

elharo · 2023-06-09T11:33:31Z

No, do not assume that. No one has approved this yet, and there does not seem to be agreement that this feature is desirable.

mdrie changed the title ~~WIP: [MDEPLOY-118] -: Untested first implementation.~~ WIP: [MDEPLOY-118] - Untested first implementation. Sep 27, 2022

cstamas reviewed Sep 27, 2022

View reviewed changes

src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java Show resolved Hide resolved

mdrie force-pushed the MDEPLOY-118 branch 2 times, most recently from ccb1794 to 703652a Compare September 29, 2022 14:22

mdrie changed the title ~~WIP: [MDEPLOY-118] - Untested first implementation.~~ [MDEPLOY-118] - Enable deployment of attached release artifacts if POM is identical Sep 29, 2022

mdrie force-pushed the MDEPLOY-118 branch from 703652a to c6cb079 Compare September 29, 2022 15:20

mdrie marked this pull request as ready for review September 30, 2022 09:18

elharo reviewed Nov 20, 2022

View reviewed changes

Mathias de Riese added 5 commits December 20, 2022 11:32

MDEPLOY-118: Add parameter comparePomWithDeployed to deploy goal.

32c4acd

MDEPLOY-118: surefire test for comparePomWithDeployed.

c5aca79

MDEPLOY-118: integration test for comparePomWithDeployed.

2e2ae57

MDEPLOY-118: fix path-handling in surefire tests for Windows.

ef26770

MDEPLOY-118: integration test: add two SNAPSHOT runs, not affected by…

e7600ce

… feature.

mdrie force-pushed the MDEPLOY-118 branch from 12ab002 to e7600ce Compare December 20, 2022 10:45

elharo requested changes May 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEPLOY-118] - Enable deployment of attached release artifacts if POM is identical #28

[MDEPLOY-118] - Enable deployment of attached release artifacts if POM is identical #28

mdrie commented Sep 27, 2022 •

edited

Loading

mdrie commented Sep 29, 2022

mdrie commented Sep 30, 2022

mdrie commented Oct 6, 2022

elharo left a comment

mdrie commented Dec 20, 2022

mdrie commented May 17, 2023

elharo commented May 17, 2023

mdrie commented May 17, 2023

elharo commented May 17, 2023

mdrie commented May 17, 2023

elharo commented May 17, 2023

mdrie commented May 23, 2023

KemalSoysal commented May 23, 2023

elharo commented May 23, 2023

KemalSoysal commented May 23, 2023 •

edited

Loading

elharo commented May 23, 2023

KemalSoysal commented May 23, 2023

elharo May 23, 2023

elharo May 23, 2023

elharo May 23, 2023

elharo May 23, 2023

elharo May 23, 2023

elharo May 23, 2023

mdrie commented Jun 9, 2023

elharo commented Jun 9, 2023

[MDEPLOY-118] - Enable deployment of attached release artifacts if POM is identical #28

Are you sure you want to change the base?

[MDEPLOY-118] - Enable deployment of attached release artifacts if POM is identical #28

Conversation

mdrie commented Sep 27, 2022 • edited Loading

mdrie commented Sep 29, 2022

mdrie commented Sep 30, 2022

mdrie commented Oct 6, 2022

elharo left a comment

Choose a reason for hiding this comment

mdrie commented Dec 20, 2022

mdrie commented May 17, 2023

elharo commented May 17, 2023

mdrie commented May 17, 2023

elharo commented May 17, 2023

mdrie commented May 17, 2023

elharo commented May 17, 2023

mdrie commented May 23, 2023

KemalSoysal commented May 23, 2023

elharo commented May 23, 2023

KemalSoysal commented May 23, 2023 • edited Loading

elharo commented May 23, 2023

KemalSoysal commented May 23, 2023

elharo May 23, 2023

Choose a reason for hiding this comment

elharo May 23, 2023

Choose a reason for hiding this comment

elharo May 23, 2023

Choose a reason for hiding this comment

elharo May 23, 2023

Choose a reason for hiding this comment

elharo May 23, 2023

Choose a reason for hiding this comment

elharo May 23, 2023

Choose a reason for hiding this comment

mdrie commented Jun 9, 2023

elharo commented Jun 9, 2023

mdrie commented Sep 27, 2022 •

edited

Loading

KemalSoysal commented May 23, 2023 •

edited

Loading