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

Maven Toolchains Support #276

Closed
Okeanos opened this issue Jan 13, 2022 · 13 comments
Closed

Maven Toolchains Support #276

Okeanos opened this issue Jan 13, 2022 · 13 comments
Labels
feature request New feature or request to improve the current logic

Comments

@Okeanos
Copy link
Contributor

Okeanos commented Jan 13, 2022

Description:
This is related to #44, however, while that issue in particular asks for multiple JDKs to be installed with a single setup-java invocation, I would like the general setup-java invocation to provide a Maven Toolchains declaration for the selected JDK(s).

This requires putting a toolchains.xml file with the proper definitions into the $HOME/.m2 location of the current user, i.e. the one running Maven.

Implementation of #44 would require changes to this, i.e. all additional JDKs would have to be correctly exposed in the toolchains declaration as well.

Justification:
This would allow using setup-java with less overhead in projects that have Maven toolchain requirements configured and also allow Gradle to pick up JDK locations automatically starting with 7.4. This was something discussed in #44 as well and is to be implemented in gradle/gradle#14903 with a different solution.

Are you willing to submit a PR?
There's a pull request: #282

@Okeanos Okeanos added feature request New feature or request to improve the current logic needs triage labels Jan 13, 2022
@dmitry-shibanov
Copy link
Contributor

Hello @Okeanos. Thank you for your report. We'll investigate this feature request.

@devminded
Copy link

Related to #245

@paoliniluis
Copy link

tools.deps would be a great one to support to, specially for Clojure based projects

@brcrista
Copy link
Contributor

To set context, I'm not very familiar with Maven. That said, would it be possible today to handle Maven configuration in a separate step or action? If not, is there some sort of output we can add to setup-java to make that possible?

@Okeanos
Copy link
Contributor Author

Okeanos commented Jun 19, 2022

To set context, I'm not very familiar with Maven. That said, would it be possible today to handle Maven configuration in a separate step or action? If not, is there some sort of output we can add to setup-java to make that possible?

That depends a little – the toolchains.xml declaration works independently from a particular Maven installation and can also be used by Gradle to streamline detection of available JDKs.

To generate a useful and valid toolchains declaration for a particular JDK the following information is necessary:

  • JDK version (major version only, e.g. 1.6, 8, 16, 17)
  • JDK location (/path/to/jdk)

Optionally, some additional useful information needs to be made available – in particular important for setup-java because it offers choices with side-effects:

  • JDK vendor
  • some sort of stable unique ID to identify this particular toolchain

The toolchains.xml exists only once and can be extended to contain any number of known JDKs and allow detection/usage by Maven and Gradle. So multiple setup-java (or matrix?) invocations should be consumable and generate a full toolchains declaration. Any other tooling just builds on top of this and makes using setup-java easier.

@brcrista
Copy link
Contributor

setup-java provides outputs for distribution, version, and path. Is there anything else that prevents a subsequent step from generating toolschains.xml using those outputs?

@Okeanos
Copy link
Contributor Author

Okeanos commented Jun 29, 2022

There probably isn't anything missing from a quick glance – that is a fair point and something I overlooked initially. Thanks for providing this insight.

However, the idea is to reduce friction in the overall Java ecosystem on GitHub Runners. I dare say a huge number of people who works with a JVM based language in Github Actions uses setup-java. A large number of these people use Maven or Gradle (because they are the most popular 'defaults' so to speak) to build their projects. Maven and Gradle both have compatible mechanisms to find out which JDKs exist and where they are located. That mechanism is Maven Toolchains (yes, even for Gradle). Hence, this "feature request" is actually an improvement applicable to a huge number of people and not playing favourites. The goal here isn't to do something for Maven (even though I mention it a lot) only.

This addition makes it very easy for the people to correctly consume JDKs (even matrix builds) by getting a minimal discovery setup out of the way "batteries included"-style.
Adding a setup-java-toolchains task in between setup-java and whatever-else instead adds more friction and, for me personally, isn't even an improvement over just shipping the toolchains.xml myself (or building it dynamically in the pipeline). I wouldn't use such an intermediate step because I already have my pipeline setup with a custom step that provides this file. I don't want to replace the step, I want to get rid of it altogether.

Where's the friction imposed by an additional step? Such a step has to be separately maintained. The maintainers will have to carefully watch changes to setup-java in order to react to these changes, which causes tight coupling between the two with no benefit I can see. People have to discover this new step, see a benefit in adopting it – (a non-obvious benefit for Gradle because they didn't know Gradle consumes Maven toolchains; I mean who reads documentation or release notes?) – and add it to their pipeline. Is this a cool developer/user experience? I don't think so.

@brcrista
Copy link
Contributor

I see thanks for the explanation and for suggesting the feature. I'm happy to leave this issue open to see if it attracts more attention, but wanted to make sure you weren't blocked.

@laeubi
Copy link

laeubi commented Jul 7, 2022

I can only second @Okeanos this would be a very useful feature, currently we have to manually setup this stuff and maintain toolchains.xml files. So it would make a lot of sense to have this in the java-setup action.

@cstamas
Copy link

cstamas commented Jul 12, 2022

A very welcomed feature!

@laeubi
Copy link

laeubi commented Jul 29, 2022

@brcrista any progress here?

We have:

  • 12 positive reactions
  • an open PR with code contribution

so what is holding this back?

@brcrista
Copy link
Contributor

Mostly just our bandwidth. Expanding the scope of the action / feature requests are going to take more thought than a bugfix because of the maintenance implications.

Left a comment on the PR.

@laeubi
Copy link

laeubi commented Aug 10, 2022

Mostly just our bandwidth. Expanding the scope of the action / feature requests are going to take more thought than a bugfix because of the maintenance implications.

I'm sure there will always be people helping out in that area once there is a support for toolchains, as this is very very common in maven setups and cumbersume to setup, just one example how this might currently be done in GH actions:

https://blog.bmarwell.de/2020/12/01/testing-maven-with-toolchain-and-github-actions.html

Now when you use more than one JDK it even becomes more complex, but actually toochains are exactly for using multiple JDKs (e.g. for multirelease jars) so this would really be a big benefit to have it supported directly by the action!

marko-zivic-93 added a commit that referenced this issue Sep 28, 2022
yeikel pushed a commit to yeikel/setup-java that referenced this issue Feb 12, 2023
* Add (optional) Maven Toolchains Declaration after JDK is installed
* Extract common/shared Maven constants

Resolves actions#276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

No branches or pull requests

7 participants