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 Toolchains Declaration (#276) #282

Merged
merged 1 commit into from Sep 28, 2022

Conversation

Okeanos
Copy link
Contributor

@Okeanos Okeanos commented Jan 17, 2022

Description:

  • Add Maven Toolchains Declaration after JDK is installed
  • Extract common/shared Maven constants

Related issue:
#276

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

I also used my fork + branch on a private repository's pipeline to verify a valid toolchains declaration is created for the specified JDK and used by a Maven based build job.

Technically, using ${env.JAVA_HOME} inside the toolchains.xml file is possible, however, this will get interesting with #44 very fast. Not to mention that e.g. version and vendor are not exposed as variables anyway.

@Okeanos
Copy link
Contributor Author

Okeanos commented Mar 5, 2022

I would very much appreciate if @dmitry-shibanov or someone else from the maintainers could chime in here and tell me what I have to do in order to get this merged. Thanks in advance.

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from cc32ecf to 6206324 Compare Apr 9, 2022
@Okeanos Okeanos requested a review from a team Apr 9, 2022
@Okeanos Okeanos force-pushed the maven-toolchains-support branch from 6206324 to 0758be1 Compare Apr 19, 2022
@laeubi
Copy link

laeubi commented Jun 5, 2022

@dmitry-shibanov can you help here? That would be very helpful!

src/toolchains.ts Show resolved Hide resolved
laeubi
laeubi approved these changes Jun 5, 2022
Copy link

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

This is a great feature and would simplify setup for maven based builds!

@laeubi
Copy link

laeubi commented Jul 7, 2022

@marko-zivic-93 can you maybe review/ merge this?

@Okeanos
Copy link
Contributor Author

Okeanos commented Jul 7, 2022

@laeubi there's an ongoing discussion in #276 where some folks from GitHub maintaining this repository chimed in as well. If you are in favour of this and maybe know some other people who'd like this, I suggest you post there and like the issue to give visible feedback to the maintainers.

@Okeanos Okeanos mentioned this pull request Jul 7, 2022
@Okeanos Okeanos force-pushed the maven-toolchains-support branch from c7f4d58 to ca4cb14 Compare Jul 29, 2022
@Okeanos
Copy link
Contributor Author

Okeanos commented Jul 29, 2022

The failing validation checks were caused by connection timeouts during installation. Not by anything in this PR.

@brcrista
Copy link
Contributor

@Okeanos could you point us to a sample workflow run that uses the new functionality? That would help me understand what we're adding here.

I saw the YAML snippet you added for the docs, but I'm also curious how the toolchain gets consumed in later steps.

@laeubi
Copy link

laeubi commented Aug 10, 2022

You can take a look here:

https://github.com/eclipse-platform/eclipse.platform/blob/master/.github/workflows/build.yml

currently we maintain a hand written toolchains.xml and need to pass that to the maven command see:
https://github.com/eclipse-platform/eclipse.platform/blob/b3ad2b40185da67b124e4359468f5d00057776c9/.github/workflows/build.yml#L31

with an automatic one like proposed here, it would be just a matter of calling mvn without any extra parameter and always having the setup java (while currently the toolchains and java setup can easily get out of sync)

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from ca4cb14 to 16eadc0 Compare Aug 11, 2022
@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 11, 2022

@brcrista I setup a minimal project with pipelines that are hopefully self-descriptive here: https://github.com/Okeanos/setup-java-example/actions

It appears that Gradle benefits less from this PR on GitHub Actions than anticipated from Gradle#14903 even for e.g. Gradle 7.3 which did not support Maven Toolchains detection.

Some jobs, such as the Java 8 and 11 are supposed to fail (technically, even for Gradle but it has auto-detection that works on GitHub Actions and for some reason matrix builds behave different from what I expected) because no valid JDK was found/supposed to be available/configured and the toolchains requirements in the pom.xml and build.gradle enforce an exact version requirement.

I also found and fixed a bug in my implementation and improved the tests as well. Asking for in-depth examples was a good idea ❤️ .

Additionally, I discovered that the outputs.java-version value from the setup-java task is actually incompatible with Toolchains declarations because it is the exact version (as evident in the Job called Java Default Actions sample step Show Toolchains (Debug) whereas the Java version for major releases is required (as used in the setup-java.inputs.java-version. Even the distribution value is … not really good. I would have expected temurin to be the value not Temurin-Hotspot; distribution is special because we can filter by vendor within Maven builds and "Temurin-Hotspot" isn't a vendor … "Temurin" could be interpreted as one; technically correct would probably be Eclipse Temurin for this example (and is reported by Gradle as well).

If you have any further questions or need better examples (for an arbitrary value of "better") please let me know.

@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 16, 2022

Just got the pipeline notifications … gonna fix the formatting issues etc. Could have sworn I ran that formatting etc. after checking with the readme/contribution guidelines. Sorry about that :(

src/toolchains.ts Outdated Show resolved Hide resolved
@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 16, 2022

As an aside and something that was not yet fully addressed I believe in either the PR or the issue: we can make the toolchains generation optional/opt-in and properly document that in the readme.

This would ensure there are absolutely no breaking changes for existing users with working solutions and reduce the maintenance/support headache you rightfully pointed out – if they ever get around to upgrading (and reading about the changes in the update) they can still decide to switch. New users will hopefully see the notes and decide for themselves what they like best.

Any thoughts? Personally I am fine with either.

src/toolchains.ts Outdated Show resolved Hide resolved
src/toolchains.ts Outdated Show resolved Hide resolved
@brcrista
Copy link
Contributor

There are some CI failures. Running npm format and npm build should fix those.

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from 16eadc0 to 4043b21 Compare Aug 17, 2022
@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 17, 2022

As an aside and something that was not yet fully addressed I believe in either the PR or the issue: we can make the toolchains generation optional/opt-in and properly document that in the readme.

This would ensure there are absolutely no breaking changes for existing users with working solutions and reduce the maintenance/support headache you rightfully pointed out – if they ever get around to upgrading (and reading about the changes in the update) they can still decide to switch. New users will hopefully see the notes and decide for themselves what they like best.

Any thoughts? Personally I am fine with either.

Along with review fixes I did some additional changes pertaining to this:

  • rename the toolchain-id flag to mvn-toolchain-id
  • add a mvn-toolchain boolean flag (currently set to true by default) allowing users to easily toggle generation/modification of the file per setup-java invocation
  • expand the documentation the readme and advanced-usage docs somewhat to make usage and intentions clearer

@laeubi
Copy link

laeubi commented Aug 17, 2022

@Okeanos mayn many thanks this looks really great!

@brcrista
Copy link
Contributor

add a mvn-toolchain boolean flag (currently set to true by default) allowing users to easily toggle generation/modification of the file per setup-java invocation

I've been thinking about this and haven't come to a decision yet ... I'm leaning towards doing it though. Since the intent is to avoid a potential breaking change, we would have to set it to false by default and then either switch it to true or remove the input on a major version upgrade.

@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 17, 2022

add a mvn-toolchain boolean flag (currently set to true by default) allowing users to easily toggle generation/modification of the file per setup-java invocation

I've been thinking about this and haven't come to a decision yet ... I'm leaning towards doing it though. Since the intent is to avoid a potential breaking change, we would have to set it to false by default and then either switch it to true or remove the input on a major version upgrade.

Take your time – I am in no rush and just happy this is considered for integration. Waiting another few days and fixing any outstanding issues in the meanwhile is not a problem for me and I'll gladly try to incorporate any changes you would like to have made.
Spontaneously, I am not even sure removing the mvn-toolchain boolean flag is ultimately necessary based on current integration complexity of this whole feature within the setup task vs. the inherit (perceived) benefit of additional flexibility it offers to consumers/users. Inverting the boolean flag – assuming false is the initial default – to push users towards a "better" future with less custom code to maintain on their end sounds like a good future step, though.

src/setup-java.ts Outdated Show resolved Hide resolved
@dmitry-shibanov
Copy link
Contributor

Could you please sync with the main branch ?

@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 18, 2022

Could you please sync with the main branch ?

As far as I can currently tell the branch builds on top of the current main branch. The GitHub UI also says "This branch is 1 commit ahead of actions:main." the one commit being my commit introducing this feature.

If you introduce any new commits to main before this is merged I'll incorporate those changes of course.

@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 26, 2022

@brcrista @dmitry-shibanov I'll be switching to default: false soon based on the discussion above. Additionally, I noticed there are a couple of PRs such as #368 you are currently working on that will have some impact here. Please let me know when I can expect that to be merged and I'll update my PR to incorporate this.

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from 4043b21 to b7f3671 Compare Aug 29, 2022
src/constants.ts Outdated Show resolved Hide resolved
@brcrista
Copy link
Contributor

I've been thinking about this and haven't come to a decision yet ... I'm leaning towards doing it though. Since the intent is to avoid a potential breaking change, we would have to set it to false by default and then either switch it to true or remove the input on a major version upgrade.

Ok, so I consider defaulting mvn-toolchain to true to be a breaking change because it will cause the toolchains file to be overwritten by side effect, and it alters the behavior of Maven.

I think we can just remove the mvn-toolchain input and make the next release once this PR goes in a major-version release.

@dmitry-shibanov are you planning to merge #368 first? It seems like this PR will need some reaction work to add all Java versions to the Maven toolchain file.

@Okeanos
Copy link
Contributor Author

Okeanos commented Aug 31, 2022

I've been thinking about this and haven't come to a decision yet ... I'm leaning towards doing it though. Since the intent is to avoid a potential breaking change, we would have to set it to false by default and then either switch it to true or remove the input on a major version upgrade.

Ok, so I consider defaulting mvn-toolchain to true to be a breaking change because it will cause the toolchains file to be overwritten by side effect, and it alters the behavior of Maven.

I think we can just remove the mvn-toolchain input and make the next release once this PR goes in a major-version release.

@dmitry-shibanov are you planning to merge #368 first? It seems like this PR will need some reaction work to add all Java versions to the Maven toolchain file.

In that case I'll wait for #368 to be merged and make the necessary changes for that alongside removing the mvn-toolchains input + adding mvn-toolchain-id to the action.yml.

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from b7f3671 to 473afa6 Compare Sep 8, 2022
@Okeanos
Copy link
Contributor Author

Okeanos commented Sep 8, 2022

@brcrista @dmitry-shibanov I updated the PR for setup-java to support Toolchains now that #368 (install multiple JDKs at once) has been merged.

Based on the previous findings:

  • actions.yml documentation added
  • mvn-toolchain boolean flag removed
  • formatting etc. fixed (🤞 )

Compared to the previous iteration I did some additional changes for better compatibility with exiting setup options:

  • users can now specify a mvn-toolchain-vendor to override the distribution input value that was previously used at all times. A useful example as to why this is necessary is included in the Advanced Usage documentation based on the jdkfile-self-supplied-JDK-option.
  • mvn-toolchain-vendor can only be specified one time and effectively is a fancy name for distribution for multi-line input values of java-version
  • mvn-toolchain-id now accepts multi-line input and will defer to java-versions, i.e. either a specific ID is given for every java version specified or the id value(s) are ignored completely. Fewer edge cases this way.

If there is anything else you would like me to change, let me know and I'll have a look.

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from 473afa6 to b1de5e2 Compare Sep 9, 2022
@actions actions deleted a comment from kingtaxi51 Sep 9, 2022
@Okeanos Okeanos force-pushed the maven-toolchains-support branch from b1de5e2 to 18ea0e9 Compare Sep 23, 2022
import io = require('@actions/io');
import fs = require('fs');
import path = require('path');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to the import * as ... syntax? That avoids some subtle gotchas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing that will throw "Cannot redefine property" errors because of the spyOSHomedir = jest.spyOn(os, 'homedir') in the test setup. Interestingly it works for core (where we do the same thing).
I am not well versed enough in either TypeScript or Jest to propose a better alternative. Any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and fixed this for all except os for now.

src/toolchains.ts Outdated Show resolved Hide resolved
src/toolchains.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@brcrista brcrista left a comment

Choose a reason for hiding this comment

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

A couple minor comments, but nothing blocking merge. @dmitry-shibanov could you or one of your teammates take a look also?

@Okeanos Okeanos force-pushed the maven-toolchains-support branch from 18ea0e9 to 6a9be81 Compare Sep 26, 2022
laeubi
laeubi approved these changes Sep 27, 2022
Copy link

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Go for it!

src/toolchains.ts Outdated Show resolved Hide resolved
src/toolchains.ts Outdated Show resolved Hide resolved
src/toolchains.ts Outdated Show resolved Hide resolved
src/toolchains.ts Outdated Show resolved Hide resolved
* Add (optional) Maven Toolchains Declaration after JDK is installed
* Extract common/shared Maven constants

Resolves actions#276
@Okeanos Okeanos force-pushed the maven-toolchains-support branch from 6a9be81 to eb1418a Compare Sep 27, 2022
@marko-zivic-93 marko-zivic-93 merged commit e150063 into actions:main Sep 28, 2022
178 checks passed
@brcrista
Copy link
Contributor

Thank you @Okeanos !

@laeubi
Copy link

laeubi commented Sep 28, 2022

Very great, when will it be release? 👍

@Okeanos Okeanos deleted the maven-toolchains-support branch Oct 1, 2022
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.

None yet

9 participants