Skip to content

[MRRESOURCES-150] Ensure reproducible order in bundle #69

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.

Sign up for GitHub

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

Jump to bottom
Merged
slawekjaranowski merged 5 commits into from
Dec 19, 2024
Merged

[MRRESOURCES-150] Ensure reproducible order in bundle #69

slawekjaranowski merged 5 commits into from
Dec 19, 2024

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Dec 17, 2024

What changes were proposed in this pull request?

remote-resources:bundle creates output that may not be reproducible.

maven-remote-resources-plugin/src/main/java/org/apache/maven/plugin/resources/remote/BundleRemoteResourcesMojo.java

Lines 108 to 122 in 7a40047

DirectoryScanner scanner = new DirectoryScanner();
scanner.setBasedir(resourcesDirectory);
if (includes != null && includes.length != 0) {
scanner.setIncludes(includes);
} else {
scanner.setIncludes(DEFAULT_INCLUDES);
}
if (excludes != null && excludes.length != 0) {
scanner.setExcludes(excludes);
}
scanner.addDefaultExcludes();
scanner.scan();

DirectoryScanner uses java.io.File.list(), which does not guarantee order.

https://issues.apache.org/jira/browse/MRESOURCES-311

How was this patch tested?

Built the plugin and used it in another project. Changed naturalOrder to reverseOrder, and repeated. Verified output is reversed between the two runs.

@adoroszlai adoroszlai mentioned this pull request Dec 17, 2024
Merged
@adoroszlai
Copy link
Contributor Author

adoroszlai commented Dec 18, 2024

@elharo @slawekjaranowski please review when you have some time

elharo
elharo reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test?

@elharo elharo changed the title MRESOURCES-311. Ensure reproducible order in bundle [MRESOURCES-311] Ensure reproducible order in bundle Dec 18, 2024
@slawekjaranowski
Copy link
Member

slawekjaranowski commented Dec 18, 2024

I moved issue to correct jira project - https://issues.apache.org/jira/browse/MRRESOURCES-150
please update commit message and PR title

@slawekjaranowski slawekjaranowski self-requested a review December 18, 2024 20:34
@adoroszlai adoroszlai changed the title [MRESOURCES-311] Ensure reproducible order in bundle [MRRESOURCES-150] Ensure reproducible order in bundle Dec 18, 2024
@adoroszlai
Copy link
Contributor Author

adoroszlai commented Dec 18, 2024

Test?

Added.

I moved issue to correct jira project

Thanks.

please update commit message and PR title

PR title updated.

Would you like me to squash commits and force-push?

@slawekjaranowski
Copy link
Member

slawekjaranowski commented Dec 18, 2024

Would you like me to squash commits and force-push?

I can squash ....

slawekjaranowski
slawekjaranowski reviewed Dec 18, 2024
View reviewed changes
@slawekjaranowski slawekjaranowski added the enhancement New feature or request label Dec 18, 2024
elharo
elharo requested changes Dec 19, 2024
View reviewed changes
src/test/java/org/apache/maven/plugin/resources/remote/RemoteResourcesMojoTest.java Outdated
import org.eclipse.aether.internal.impl.SimpleLocalRepositoryManagerFactory;
import org.eclipse.aether.repository.LocalRepository;

import static org.assertj.core.api.Assertions.assertThat;
Copy link
Contributor

@elharo elharo Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These days we're using Hamcrest, not assertj

Copy link
Member

@slawekjaranowski slawekjaranowski Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only test dependencies .... I will not require
even more JUnit 5 doesn't use it

Copy link
Contributor

@elharo elharo Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to settle on one thing. I don't care much which one, but let's not have a random mix of libraries doing the same thing.

Copy link
Member

@slawekjaranowski slawekjaranowski Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we didn't use any Hamcrest nor assertj until this PR
@elharo any other remarks?

Copy link
Contributor

@elharo elharo Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression is that across Maven and even more broadly across Java, but definitely within Maven hamcrest is far more common now. I've certainly seen it a lot more while working my way through all the plugins than I've seen assert4j, though they are both present. JUnit 4 already depends on Hamcrest, though that got removed in JUnit 5.

Copy link
Member

@slawekjaranowski slawekjaranowski Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok we have Hamcrest so I would like to merge

@slawekjaranowski slawekjaranowski merged commit 3c59569 into apache:master Dec 19, 2024
20 checks passed
@slawekjaranowski
Copy link
Member

slawekjaranowski commented Dec 19, 2024

@adoroszlai thanks

@adoroszlai
Copy link
Contributor Author

adoroszlai commented Dec 19, 2024

Thanks @elharo, @slawekjaranowski for the review.

@adoroszlai adoroszlai deleted the MRESOURCES-311 branch December 19, 2024 21:42
@slawekjaranowski slawekjaranowski added this to the 3.3.0 milestone Dec 21, 2024
@jira-importer
Copy link

jira-importer commented Jun 19, 2025

Resolve #194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slawekjaranowski slawekjaranowski slawekjaranowski approved these changes

@elharo elharo elharo approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

3.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@adoroszlai @slawekjaranowski @jira-importer @elharo