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 #2305

Closed
wants to merge 1 commit into from
Closed

Add Maven Wrapper #2305

wants to merge 1 commit into from

Conversation

sconvent
Copy link

This adds a Maven Wrapper.
Right now, the used Maven version to build this project is unclear and to make reproducible builds, a wrapper should be added.

More info: Maven Wrapper Documentation

Right now, the check-api-compatibility Github Action cannot be changed to use the wrapper as it operates on the base commit which does not yet have a Maven Wrapper.

@google-cla
Copy link

google-cla bot commented Jan 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Marcono1234
Copy link
Collaborator

Disclaimer: I am not a direct project member; they might think differently about this pull request.


Thanks, this looks interesting! One issue might be that there is no official "wrapper validation GitHub action" yet (see MNG-6887), so in the future the Gson maintainers would have to be careful when someone creates a pull request claiming to "upgrade the wrapper JAR".

It might be good to cache the Maven distribution downloaded by the wrapper because the setup-java action does not do that yet (actions/setup-java#448). I assume the build logs for this pull request did not indicate that the wrapper is downloaded because info log messages are discarded by the wrapper by default.

In case this is merged, it might also be useful to use the wrapper for the OSS-Fuzz configuration (probably requires changes to Dockerfile and build.sh).

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Oct 25, 2023

There are also variants for the wrapper setup which don't involve any binary files, see https://maven.apache.org/wrapper/index.html#usage-without-binary-jar
Especially only-script sounds tempting I think because it also does not involve downloading the wrapper JAR. But apparently that variant can fail in some cases, see google/guava#6783 (to be investigated?), and the question is in general how reliable that variant is when it relies apparently the most on shell scripts.
Edit: There is also the issue MWRAPPER-97, which seems to be specific to only-script.

The question would also be whether the checksums of the downloaded files should be checked, see https://maven.apache.org/wrapper/index.html#checksum-verification-of-downloaded-binaries and also google/guava#6805.

@cpovirk
Copy link
Member

cpovirk commented Oct 25, 2023

see google/guava#6783 (to be investigated?), and

I was planning to be lazy and not do anything about it... :) Ideally, I would report it upstream (or I guess even propose a fix).

I got the impression that the only problem was that we had Windows CI that was using mvnw (rather than mvnw.cmd). So if Gson has no Windows CI or it at least would configure that CI to use the right script, only-script would likely be fine.

@Marcono1234
Copy link
Collaborator

I got the impression that the only problem was that we had Windows CI that was using mvnw (rather than mvnw.cmd).

It looks like I reproduced it now locally in a test Maven project by running the Linux mvnw script on Windows using Git Bash (and probably due to having git core.autocrlf true (or similar) / the properties file using CRLF). Though that explicitly required me to use Git Bash.

@sconvent sconvent closed this by deleting the head repository Nov 26, 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