-
Notifications
You must be signed in to change notification settings - Fork 20
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
[MSHARED-1066] - Upgrade Plexus Archiver to 4.3.0, Improve the Reproducible Builds methods #22
Conversation
e11bd5e
to
fe368cf
Compare
c9249ff
to
36b3bf9
Compare
Can we split out the two thing here? Update p-archiver (and plexus-io) is unrelated to repro methods, is it? |
Removed plexus-io update as is actually unrelated (will open a follow-up PR). The update of plexus-archiver and reproducible methods are actually related, the reproducible methods depends on plexus-archiver 4.3.0 as plexus-archiver also contains changes to methods. It's split in two commits, and to make them two different PRs, the plexus-archiver update should be merged first and then the reproducible methods later, yet since the dependency update is a really small change I would like to keep them together. |
Kk, but it would be good to sort out maven-archiver changes, make a release soon: As MASSEMBLY is eagerly waiting updates |
This should be ready for review, after merging this and #24 a release can be made. |
We need to move this forward to also move apache/maven-jar-plugin#43 and release maven-jar-plugin 3.3.0 |
c3a5bf7
to
eecb791
Compare
wazzup here? @michael-o |
* @throws IllegalArgumentException if the outputTimestamp is neither ISO 8601 nor an integer | ||
*/ | ||
public static Optional<Instant> parseBuildOutputTimestamp( String outputTimestamp ) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that such parsing is done over and over again in many components. We need to consider to have ths at as central place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this means, are you referring to moving this out of Maven Archiver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a note that this could would be better suited in a shared util for other subcomponents as well.
} | ||
|
||
@ParameterizedTest | ||
@CsvSource( { "0,0", "1,1", "9,9", "1570300662,1570300662", "2147483648,2147483648", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That source is quite insane :-D
Except for the remaining comments this looks good. I will run this snapshot against MJAR, MWAR, and likely other projects just to make sure. |
Please have a look (maven-javadoc-plugin):
|
Please have a look (maven-source-plugin):
|
The |
Done. Issue resolved. |
I can confirm that maven-source-plugin passes from master, but not this PR. |
The IT from And actually, the root cause is not even in this PR, you can make that test fail by just updating plexus-archiver to 4.2.7 (right now is at 4.2.4), plexus-archiver 4.2.7 fixes the order of META-INF/ and META-INF/MANIFEST.MF entries in a JAR file, this "reorder" of entries makes that the output (and the checksum) change. So, essentially the "fix" is just to use the new sha1 checksum. |
Right, I did the fix for the order because it was broken for quite some time. Let me double check. |
I will use diffoscope. |
Here it is:
@jorsol Would you mind to create a PR this? Your analysis is correct. |
I will assign issues in JIRA to me and handle this PR. Muchas gracias por tu ayuda! |
Signed-off-by: Jorge Solórzano <jorsol@gmail.com> This closes apache#22
Signed-off-by: Jorge Solórzano <jorsol@gmail.com> This closes apache#22
Fixes MSHARED-1066 and MSHARED-1067
Following this checklist to help us incorporate your
contribution quickly and easily:
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.
[MSHARED-XXX] - Fixes bug in ApproximateQuantiles
,where you replace
MSHARED-XXX
with the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verify
to make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
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.