[MSOURCES-140] fail only if re-attach different files #24
Conversation
hboutemy
force-pushed
the
MSOURCES-140
branch
2 times, most recently
from
9078ef4 to
8f4ab52
Compare
| + "for at least one of them."); | ||
| Artifact previouslyAttached = getPreviouslyAttached(attachedArtifact, project, getClassifier()); | ||
| if (previouslyAttached != null) { | ||
| if (!outputFile.equals(previouslyAttached.getFile())) { |
There was a problem hiding this comment.
I think we must check the (current and "future") content is the same otherwise we hide a bug with such a logic - ie if between first and second run you add sources then the output artifact is likely wrong.
There was a problem hiding this comment.
too complex, without real benefit: before, mvn install deploy worked in any case
this notion of recording really the content each time attach is done is over-complexifying things: it will detect for example if build is not reproducible = not at all the intent
There was a problem hiding this comment.
Hmm, while I agree it is useless this means this PR too otherwise it defeats the enhancements to prevent to write twice the same artifact and corrupt a build so do we just close it?
There was a problem hiding this comment.
it defeats the enhancements to prevent to write twice the same artifact and corrupt a build
wrong:
- it avoids adding twice: see the
boolean requiresAttach - it checks if 2 attaches try to attach real different files = different file names = where you really need to configure a classifier
as I wrote on the ML
I think it does the right job at differentiating 2 cases that the initial MSOURCES-121 merged too aggressively:
- do not fail when executed twice with the same output file
- but fail when re-attach happens to another output file = another classifier has to be configured
There was a problem hiding this comment.
Not really, I understand this PR but you introduce a regression which is that you can attach twice the same artifact due to two executions so this "fix" is actually a regression.
requiresAttach is never needed until the build is broken - ie you build twice the source artifact which means you had a broken one until it is exactly the same content (why I originally proposed to test it since it must never happen in sane builds so cost is supposed to be 0).
requiresAttach is more a ignoreAlreadyAttachedArtifact but looses the "attached artifact was not wrongly used" by another mojo case (ie you enable to process a source jar which is not the one deployed).
The fix for the two tickets is quite easy and is on build side (it is already fixed on maven side):
- disable attach-sources phase which was embedded in maven 3.9 (or use maven 4 which does not have it anymore)
- use the same attach-sources than in 3.9 (jar-no-fork instead of jar)
So think we don't need to loose the check which was introduced in the plugin.
To make it more obvious I think https://issues.apache.org/jira/browse/MSOURCES-121 is right and shouldn't be reverted by this PR.
There was a problem hiding this comment.
then do nothing? keep ignoring? keep saying "fixed in Maven 4"? keep saying "don't do"? keep saying "if you do that, you are wrong"?
we're staying in circle being a pain in the ass of users who just conclude: "last working version was 3.2.1"
There was a problem hiding this comment.
Agree with Herve here.
We should be more users friendly here.
Let's simply introduce a new flag called ignoreAlreadyAttachedArtifact.
If the artifact is already attached and the flag true then simply accept to override the previous entry (but log a warn) otherwise just fail as the change introduces MSOURCES-121
At least users will have the choice.
There was a problem hiding this comment.
I'm ok to drop this PR in favor of a new boolean config parameter: it will improve user experience a little bit
who can code it, please?
I don't see how to code "override the previous entry"
There was a problem hiding this comment.
@hboutemy just replace it by documentation, if you reread the threads it is just people don't understanding maven superpom notion, we did our homework and fixed it now it is just a matter of managing the legacy.
@hboutemy @olamy I'm ok with ignoreAlreadyAttachedArtifact if it is not a @Parameter and if it is initialized with mvn.version == 3 + a comment must be false for maven 4 since see MSOURCES-121 and @github/#24. I'm not ok to do positive enhancements then revert them because we are not able to explain what maven does or do a new maven release - literally this PR from my window.
There was a problem hiding this comment.
I added new IT for MSOURCES-140 and improved messages to clarify cases:
[ERROR] Artifact org.apache.maven.its.sources:jar-no-fork:java-source:sources:1.0-SNAPSHOT already attached to a file target/jar-no-fork-1.0-SNAPSHOT-sources.jar: attach to target/jar-no-fork-1.0-SNAPSHOT-secondary-sources.jar should be done with another classifier
if build is trying to attach 2 different files with same coordinates => failure and ask for configuring classifier
[INFO] Artifact org.apache.maven.its.sources:jar-no-fork:java-source:sources:1.0-SNAPSHOT already attached to target/jar-no-fork-1.0-SNAPSHOT-sources.jar: ignoring same re-attach (same artifact, same file)
if re-executing same goal configuration twice: just ignore the second time
https://issues.apache.org/jira/browse/MSOURCES-140
hoping this will also solve https://issues.apache.org/jira/browse/MSOURCES-141 and https://issues.apache.org/jira/browse/MSOURCES-143