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

Use Maven Wrapper to build #1548

Closed
wants to merge 4 commits into from

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented May 31, 2024

And do not rely on third party tools as they
may do things that surprise us.

And do not rely on third party tools as they
may do things that surprise us.
@cstamas cstamas self-assigned this May 31, 2024
@slawekjaranowski
Copy link
Member

or we can add:

      - name: Set up Maven
        run:
          mvn --errors --batch-mode --show-version org.apache.maven.plugins:maven-wrapper-plugin:3.3.2:wrapper "-Dmaven=3.9.7"

@laeubi
Copy link

laeubi commented Jun 1, 2024

There is a setup-maven action:
https://github.com/marketplace/actions/setup-maven

I use it all the time without any issues and you can perfectly control the maven version.

@cstamas
Copy link
Member Author

cstamas commented Jun 1, 2024

There is a setup-maven action: https://github.com/marketplace/actions/setup-maven

I use it all the time without any issues and you can perfectly control the maven version.

We use that action, and the last IT failures made us realize how strange it behaves: it set up different Maven versions per OS:

  • ubuntu 3.8.8
  • windows 3.8.7
  • macos 3.9.7

True, we did not ask for specific maven version, but that is not the point. We should use wrapper anyway to be able to precisely control which Maven version we want to use (across all OSes) but also for dogfooding purposes.

See
https://github.com/apache/maven/actions/runs/9322027451/job/25662372003?pr=1546
https://github.com/apache/maven/actions/runs/9322027451/job/25662372450?pr=1546
https://github.com/apache/maven/actions/runs/9322027451/job/25662372946?pr=1546

@laeubi
Copy link

laeubi commented Jun 1, 2024

and the last IT failures made us realize how strange it behaves: it set up different Maven versions per
True, we did not ask for specific maven version, but that is not the point.

Well I would call it strange to use an action without specify a version ... I mean even the readme tells you to specify one, i more suspect that without a version nothing is done actually....

@gnodet
Copy link
Contributor

gnodet commented Jun 1, 2024

There is a setup-maven action: https://github.com/marketplace/actions/setup-maven
I use it all the time without any issues and you can perfectly control the maven version.

We use that action, and the last IT failures made us realize how strange it behaves: it set up different Maven versions per OS:

I can't see the use of this action for ITs. We do use actions/setup-java, but not actions/setup-maven. I would think reusing this action in the context of GH action makes more sense.

@laeubi
Copy link

laeubi commented Jun 1, 2024

I can't see the use of this action for ITs. We do use actions/setup-java, but not actions/setup-maven. I would think reusing this action in the context of GH action makes more sense.

Maybe it is confused with setup-java having support for maven toolchains and caching option named maven ...

If one wants there is a composite action https://github.com/marketplace/actions/setup-maven-action but I usually found setup-java/maven enough and more flexible.

@rmannibucau
Copy link
Contributor

setup-java uses OS maven version which is likely not what we want.
Guess we should reuse our shared action enabling to support setup-maven to have a build matrix (ideally).

@laeubi
Copy link

laeubi commented Jun 1, 2024

setup-java has nothing to do with installing / "using" maven it just do the following:

  • generates toolchain.xml from installed jdks
  • uses pom.xml files as cache keys

beside that I hope that the itest are using the maven that was build anyways (so a matrix des not make much sense here), so whats left is that the maven version to run the build is better specified explicitly to prevent any surprise.

@cstamas
Copy link
Member Author

cstamas commented Jun 1, 2024

Well I would call it strange to use an action without specify a version ... I mean even the readme tells you to specify one, i more suspect that without a version nothing is done actually....

You are mixing things. Again, we do NOT use setup-maven action, hence the readme you point it is... off topic.

@cstamas
Copy link
Member Author

cstamas commented Jun 1, 2024

IMHO we should not use any action to setup Maven but wrapper.... that's my 5 cents (we even have examples of "maven matrix" that is simply doable with wrapper).

@laeubi
Copy link

laeubi commented Jun 1, 2024

You are mixing things. Again, we do NOT use setup-maven action, hence the readme you point it is... off topic.

I mentioned you need to use setup-maven action, you replied that

We use that action, and the last IT failures made us realize how strange it behaves

so I obviously do not mix up anything here but maybe you have just misread my reply...

@gnodet
Copy link
Contributor

gnodet commented Jun 1, 2024

Actually, we're not using it... I thought, but the matrix is used with wrapper

https://github.com/apache/maven-gh-actions-shared/blob/v4/.github/workflows/maven-verify.yml#L196-L198

So I'm fine using wrapper here too.

@@ -47,7 47,7 @@ jobs:
cache: 'maven'

- name: Build with Maven
run: mvn verify -e -B -V -DdistributionFileName=apache-maven
run: ./mvnw verify -e -B -V -DdistributionFileName=apache-maven
Copy link
Member

Choose a reason for hiding this comment

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

before this step we can also add step Set up Maven with wrapper
and we not need add wrapper properties and scripts to code

@slawekjaranowski
Copy link
Member

I hope it is ready to merge @cstamas @gnodet what do you think

@cstamas
Copy link
Member Author

cstamas commented Jun 3, 2024

Superseded by #1550

@cstamas cstamas closed this Jun 3, 2024
@cstamas cstamas deleted the maven-3.9.x-use-mvnw branch June 3, 2024 17:43
@slawekjaranowski
Copy link
Member

#1550 is for master ... this one was for 3.x branch

@slawekjaranowski
Copy link
Member

I will create a new one for 3.x 😄

@slawekjaranowski
Copy link
Member

Done in #1553

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.

6 participants