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

Create toolchains.xml dynamically based on envs #136

Closed
wants to merge 6 commits into from

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Apr 9, 2024

Fixes #89

The GitHub runners contain the JAVA_HOME_ envs by default (even without setup-java), so we can use the envs to create the toolchain.xml dynamically.

@hfhbd hfhbd force-pushed the envToolchains branch 4 times, most recently from a5d1937 to 32e30a2 Compare April 9, 2024 19:48
@bigdaz
Copy link
Member

bigdaz commented Apr 9, 2024

Thanks for the PR! Quick question prior to a full review: have you checked that it is OK to omit the '' element from the entries in toolchains.xml?

@bigdaz
Copy link
Member

bigdaz commented Apr 9, 2024

Also, can you please push this without the changes to the dist directory? That will make it easier to merge.

(Long term we need to find a better way to manage the dist directory, perhaps via a workflow that runs automatically on pushes to main.)

@hfhbd
Copy link
Contributor Author

hfhbd commented Apr 9, 2024

the '' element

I guess you mean the vendor? I didn't run a test but I looked up the jdk maven toolchain specification which requires only the jdk home. But after thinking about it again, I guess! it would be needed/helpful for Gradle Toolchains which also uses the vendor.
So I will add the default vendor (Eclipse) back.

@bigdaz
Copy link
Member

bigdaz commented Apr 9, 2024

the '' element

I guess you mean the vendor? I didn't run a test but I looked up the jdk maven toolchain specification which requires only the jdk home. But after thinking about it again, I guess! it would be needed/helpful for Gradle Toolchains which also uses the vendor. So I will add the default vendor (Eclipse) back.

Hah, yes I typed <vendor/> but I guess that got stripped!

I suspect that Gradle is only using the env vars to locate the JDK, and then executing java --version to determine the rest. But I'm not sure, but shouldn't be too hard to confirm.

Ideally, we'd omit anything redundant (like "Eclipse"), especially if it's hard-coded and possibly incorrect.

@hfhbd
Copy link
Contributor Author

hfhbd commented Apr 9, 2024

Yes, you are right, Gradle just extracts the env entries and calls java --version by itself, so I will drop it.

@hfhbd
Copy link
Contributor Author

hfhbd commented Apr 10, 2024

Closed by #150

@hfhbd hfhbd closed this Apr 10, 2024
@hfhbd hfhbd deleted the envToolchains branch April 10, 2024 18:30
@bigdaz
Copy link
Member

bigdaz commented Apr 10, 2024

Thanks for the contribution @hfhbd!
I've been making a bunch of changes to the CI workflow, and in order to test these I created and merged a new PR (#150) with these changes.

That said, there's one small improvement that would be nice.
Currently, the logic for merging the generated toolchains.xml file with an existing one is in the GradleUserHomeCache.registerToolchains() method.
Instead, it would be good to have a separate method that did this work, with a little unit test coverage.

If you're willing to help out, this would be a nice experiment to see how the new CI process works with a forked repository. Ideally, you'll be able to enable the actions for your fork and everything should pass in your repo. Then the workflows should be applied in this repo once your PR is submitted and the workflows approved. I'm sure there will be some things not working, but hopefully the process will be smoother.

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.

Environment variables defined in toolchains.xml are incorrect on ARM runners
2 participants