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

Try a different strategy for Java versions #1627

Merged
merged 3 commits into from
Sep 22, 2024
Merged

Conversation

gbrail
Copy link
Collaborator

@gbrail gbrail commented Sep 13, 2024

Always build using Java 11, and rely on Gradle to make that happen. Support tests running on different versions via an environment variable.

This is an experiment to see what GitHub Actions does.

@gbrail gbrail force-pushed the greg-build-versions branch 2 times, most recently from 16a0de4 to 2dbb80d Compare September 13, 2024 01:16
@p-bakker
Copy link
Collaborator

Not been able yet to try this out in my setup to might have a m moment over the weekend

@gbrail
Copy link
Collaborator Author

gbrail commented Sep 13, 2024 via email

@p-bakker
Copy link
Collaborator

Little later, but got around to it now.

The altered setup results in all projects being setup by Gradle to use Java 11 now in Eclipse, including the test project.

So the test project will compile with Java 11, but when runnign the tests, it'll run with another Java version if RHINO_TEST_JAVA_VERSION is set, but only if the tests are executed through Gradle.

Personally I setup (JUnit) Test Configurations in Eclipse manually and when I do that I specify the Java version to use anyway, so that works as well.

So I think this PR works as intended :-)

Only question I have left is whether the usage of java > toolchain requires anything else from devs checking out Rhino? For me it worked out of the box, but I've done some stuff with toolchains in the past, so maybe I've already got some stuff configured on my machine and therefor it worked straight away

@p-bakker
Copy link
Collaborator

Another Q: when running the Test262 Test Suite with the -DupdateTest262properties parameter, should it maybe also block if not running on Java 11, because if run with a newer Java version, it may mark tests as passing, while they wouldn't on Java 11, for example tests in the area of language/identfiiers/...

@gbrail
Copy link
Collaborator Author

gbrail commented Sep 16, 2024

OK -- this is an entirely different approach now, inspired by this article, which points out, among other things, that running Gradle and compiling on the latest JDK is a good idea:

https://jakewharton.com/gradle-toolchains-are-rarely-a-good-idea/

The idea is this:

  • Build and compile with whatever Java version the user has, whether it's from the path or from JAVA_HOME
  • Set the "--release" flag on the compiler so that we build for the right bytecode version
  • Use the RHINO_TEST_JAVA_VERSION to test with each JVM

To make this work, since "google-java-format" (used by Spotless) is still sensitive, I had to upgrade it to the latest version, which does not run on Java 11. And the new version requires that we reformat some stuff, so the yak is being shaved here.

So the upside of all this is the following:

  • You can build Rhino with whatever Java you have
  • However, to properly build and test it you need at least Java 17
  • We will test with older Java versions to ensure compatibility before release

@gbrail gbrail marked this pull request as ready for review September 20, 2024 04:50
@gbrail
Copy link
Collaborator Author

gbrail commented Sep 20, 2024

The latest version will fail to run the tests if "updateTest262properties" is set and we're not testing on Java 11. Given that, I'd like to go ahead and merge this as letting people use Java 21-based build tools is a nice improvement.

@p-bakker
Copy link
Collaborator

Unfortunately haven't gotten around to testing it, but just looking at the changes it looks good to me!

Always build using Java 11, and rely on Gradle to make that happen.
Support tests running on different versions via an environment variable.
These are the changes driven by spotless when we upgrade
google-java-format from 1.10.0, which is very old, to 1.23.0, which is
the latest.
Rely on the RHINO_TEST_JAVA_VERSION environment variable to cause tests
to run on a different Java version, using Gradle toolchains.

The "updateTest262properties" flag will now not allow the tests to run
unless testing with Java 11.
@gbrail
Copy link
Collaborator Author

gbrail commented Sep 22, 2024

Merging this -- it means that we can now all build with newer Java versions but still should use Java 11 before updating test262 -- if that causes confusion we may have to address it in the test262 infrastructure.

@gbrail gbrail merged commit c97c4aa into master Sep 22, 2024
1 check passed
@gbrail gbrail deleted the greg-build-versions branch September 22, 2024 05:45
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.

2 participants