Skip to content
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

Add Maven Wrapper #1044

Closed
wants to merge 2 commits into from
Closed

Add Maven Wrapper #1044

wants to merge 2 commits into from

Conversation

vorburger
Copy link
Member

@cushon how about this?

See https://maven.apache.org/wrapper/

This is sorta like https://github.com/bazelbuild/bazelisk (-ish).

Also makes it more reproducible (the project, instead of the user's environment, or GitHub image, controls the Maven version, through .mvn/wrapper/maven-wrapper.properties).

@cushon
Copy link
Collaborator

cushon commented Feb 9, 2024

The gradlew / bazelisk model seems like a good approach, but on the other hand I don't remember running into differences between mvn versions that mattered for this build. Is this required for #1045?

@vorburger
Copy link
Member Author

but on the other hand I don't remember running into differences between mvn versions that mattered for this build.

More so than "problems", this means you don't have to have Maven pre-installed to build it, which seems handy?

Is this required for #1045?

Yeah, I'm using it there. In theory, without this, the Dockerfile from that PR could also just install Maven itself somehow, of course; but this approach seemed better, to me.

@vorburger
Copy link
Member Author

@cushon what's your verdict on this one?

I still think this "lowers the entry barrier", and can't see much downside/drawback?

@vorburger
Copy link
Member Author

@cushon do you now (after the other PRs were merged) want me to rebase (and make required adaptions), or just abandon this one, if you are not a big fan of merging this? (Absolutely no offense taken!) Are there any other active maintainers of this project who may want to chime in? @plumpy @cpovirk @nreid260 perhaps?

@cushon
Copy link
Collaborator

cushon commented Feb 11, 2024

Let me mull it over. Lowering the barrier to entry is a good thing , but it's not clear to me how much this does that, and it feels a bit like clutter. I am open to being convinced this is become a best practice for maven repos and I should get with the times. I see that Guava has recently adopted mvnw, maybe I'll ask them how that's going after the weekend.

@vorburger
Copy link
Member Author

vorburger commented Feb 12, 2024 via email

@cushon
Copy link
Collaborator

cushon commented Feb 12, 2024

https://github.com/search?q=mvnw.cmd&type=code may be a "data point" of interest in this context?

Thanks, it looks like there are about 1.5k mvnw files on github, and >500k pom.xml files

@cpovirk
Copy link
Member

cpovirk commented Feb 12, 2024

mvnw/gradlew/bazelisk certainly "feels like" the right thing to me, but I have little concrete evidence about mvnw in particular. (I have an impression, very possibly incorrect, that Maven is relatively stable in comparison to the other two, at least in the projects I'm familiar with.)

I can only say that:

@vorburger
Copy link
Member Author

https://github.com/search?q=mvnw.cmd&type=code may be a "data point" of interest in this context?

Thanks, it looks like there are about 1.5k mvnw files on github, and >500k pom.xml files

While I do not own any $MAVEN stock 😆 and have no reason to "push" any further for this, I do have to point out that this mixes "repos" and "modules"?

Assuming rather arbitrarily that a Maven enabled Gir repo has 10 Maven modules on average, that would mean 3% have adopted mvnw?

This is likely misleading because there's 20 years of ancient code, and a more interesting analysis would be "how many repos which contain at least 1 pom.xml and that have had at least 1 commit within the last 1 year contain a mvnw"... but I don't think that's (easily) doable with a simple query?

@vorburger
Copy link
Member Author

I do worry than a Maven version in config file will go out of date more quickly than the versions on our workstations and in GitHub Actions, since Dependabot doesn't currently update the config file: Support vendoring for Maven dependabot/dependabot-core#485

To me, this is a Feature, not a Bug... ;-) You get reproducible builds without Maven plugins breaking because of compatibility issues with Maven tool updates. This used to be a major PITA, and only isn't anymore so more recently because... how shall I put this, let me phrase it as "velocity has decreased". The principle still holds though.

From how I look at this, there is no drawback to building a project with any tool's version that's 2 years old and known to work, but not HEAD anymore. That approach only really works in... well, the "other world" (the huge "all in one" mono repo bullet train 🚆 way).

@cpovirk
Copy link
Member

cpovirk commented Feb 13, 2024

It did occur to me that I think we've sometimes had to upgrade "things" in order to build with newer versions of Java. However, I'm not sure if we've ever needed to update more than plugins. Granted, upgrading Maven will give you (IIUC) some plugin updates for free. So if we're going to pin our Maven version, we might want to make sure that we're setting explicit plugin versions in our pom.xml files (which is possibly a good idea, anyway, so that the version of Maven affects one less thing) and having Dependabot or some other tool update them periodically.

Or we just wait for things to break and then upgrade at that point :)

(Honestly, I'm kind of fuzzy on the exact relationship between Maven and its plugins. Like, besides the fact that upgrading Maven can give you new default versions of plugins, but can it also affect which versions of plugins can even work with your Maven version? I thought I remembered having to tweak versions of dependencies of plugins before, similar to this old plexus-io change, perhaps related to Maven versions. I don't think I actually care much about the answer here; I'm just recording my ignorance.)

@vorburger vorburger closed this Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants