-
Notifications
You must be signed in to change notification settings - Fork 321
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
Build with Tycho 3.0.4 and set up Maven toolchains #2223
Conversation
At the least the GH workflows in my fork currently fail while launching a Tycho-surfire test runtime. |
The failures in this PR are because the GH workflows and Jenkins-Pipeline use the specification from the main branch and not from this PR. This PR would also benefit from #2224. |
I had a very quick look, but I seem to understand it fails because the latest tp requires Java 17 but it's using Java 11. In that respect, it might be useful to archive log files in GitHub actions. |
@HannesWell I noticed that tycho-surefire actually works like default surefire if you leave it at the SYSTEM settings and configure it like described here: https://maven.apache.org/guides/mini/guide-using-toolchains.html so |
Thanks for that remark.
Maybe that's because one is inheriting from the maven plugin mojo? I havn't check this case but noticed this in the past in some places. I already wondered if Tycho should provide the more flexible configuration like the maven plugins do, so if you want to harmonize that, that's great. For the compiler plugin it is not so important because you usually either want to use the BREE JDK or is fine with the SYSTEM JDK. But why would one want to compile with Java-15, if the BREE is 11 and SYSTEM is 17? OK, maybe if one has a multi-release jar? But for running tests it can IMO make sense to for example test Java-11, 12, 13, etc. if one really wants to do that. So for the tycho-surefire-plugin I can to the following as well, although it isn't a documented property?
|
Currently MR-Jar uses the highest JVM and the
yes
As said, for running thest one should use
|
So you cannot use a different JDK for the java files of each contained release?
But that would also configure the toolchain used for all other plug-ins, like the m-compiler-p too, wouldn't it? Please note that there are also some jar typed project's in Eclipse that don't use the tycho-compiler-plugin. With maven-surefire-plugin:jdkToolchain one can pick freely any jdk from the toolchain to be used as JVM of a test runtime. And the same would be nice for tycho-surefire too. |
@LorenzoBettini could you please replay the Jenkins build of this PR with the changes I made to the Jenkinsfile. |
Well that's the purpose of the toolchains :-) But the documentation says "For selection of toolchain for use by the project" so probably this can be configured on a project basis at laest the code talks about a "context toolchain".
I think this is currently not supported but could be archived I think, you probably want to provide an integration-test to demonstrate the desired behavior there is already one that checks for the usage of a toolchains the |
@HannesWell I restarted the Jenkinsjob but it still fails when building with 4.23 and Java 11. I don't see any trace of Please, let me stress this again: by default, I think no toolchain must be used (Java 17). When we build with Java 11 and older target platforms we need the toolchain so that Java 17 is used to run the build (required by Tycho) and Java 11 is used for compiling and running the tests. I mean, I guess all the toolchain-related plugins and configurations should be put in a profile. I think it's already like that, but I just wanted to make sure. |
Indeed, I see in the LOG
so it does not take your changes... |
You need not restart the build but copy the file contents of the Jenkinsfile, then use the replay option and insert the Jenkinsfile contents there. |
@laeubi thank you for the tip! :) @HannesWell in the Jenkins job I see this error:
can it be an error in the Jenkinsfile groovy? Moreover, I don't understand what is being done in the workflow of GitHub Actions. In those workflows, we want to build and test with Java 17. There's no need to also use Java 11. If you only wanted to test the toolchain with Java 11, you might create another workflow in your fork where you install Java 11, use the options for enabling the toolchain and, most of all, use an older target platform. Something along the lines I've done in my fork https://github.com/LorenzoBettini/xtext/blob/lb_tycho_3/.github/workflows/maven-java11.yml |
Jenkinsfile
Outdated
choice(name: 'JDK_VERSION', description: 'Which JDK should be used?', choices: [ | ||
'temurin-jdk11-latest', 'temurin-jdk17-latest' | ||
]) | ||
choice(name: 'JDK_VERSION', choices: [ '11', '17' ], description: 'Which JDK version should be used?') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell I think I understand the error in the Jenkinsfile: you renamed our build parameters. I can start the PR with your Jenkinsfile, but the build parameters are not changed (and I don't think I can change them before I start your PR). Is there any specific reason why you changed them? We already have the function to retrieve the Java version.
I'll see whether I can convince Jenkins to use the new parameters to build your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help.
I did it mainly to reduce the number of times 'temurin-jdk...' is written in the workflow.
But I can revert that and use the method you suggested where needed.
IIRC you should be able to use the new Parameter of click 'build with Parameters' in the Jenkins UI after you have replayed with my changes. But I'm not sure about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I tried in the last experiment. The problem is Jenkins is overloaded and it takes ages before the job starts... when it started last time, where I think I managed to set the parameters correctly, it failed because it couldn't clone (maybe because you force-pushed and things changed during the job trying to start)... I tried to start the job again... let's see...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand. Thanks a lot for your support and help and sorry for the interference.
952e01f
to
6276e27
Compare
@HannesWell , could you please have a look at/answer this last part? |
@HannesWell looks like I finally managed to run your PR, but now it fails early... because you're too into M2E ;) ;)
I'll try again fixing the Jenkinsfile |
@HannesWell I managed to start the PR job where I selected the older TP and Java 11. https://ci.eclipse.org/xtext/job/xtext/view/change-requests/job/PR-2223/16/console Tycho surefire seems to correctly take jdk11, while maven-surefire still uses jdk17 (the job will fail later with xtend tests with OOM error because to speed up things I've used a different POD) |
I guess the problem with Maven surefire is that you pass However, differently from Tycho surefire, Maven surefire does not have a property for specifying the toolchain: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jdkToolchain |
Freudian Typo 😄 Thanks for fixing that.
I also tested a bit more in the workflows of my clone and from that it looks like it is more the other way round, that tycho-surefire does not have such property like
My intention is, because toolchains are already set up now, to always use the JDK-11 to compile the Xtext projects and let the JDK_VERSION property in the Jenkinsfile only influence the JDK used in the test runtimes of maven/tycho-surefire (by default 17). In GH workflows only Java-17 is used in the test-runtimes. Of course we could continue to use JDK-17 to compile xText projects and use the option release=11. But since both JDKs are already set up, I would say that we can simply always use JDK-11 for compilation. More tomorrow, it is too late for more. :) |
If the goal is to really execute the test on Java 11 and Java 17, then you can also add another execution of the tycho-surefireplugin that uses |
we want the AND not to be a "in the same build" |
@HannesWell , I think you're over-engineering here and I don't think that's what we want. Moreover, I would do things gradually. I'd leave alone GitHub Actions for the moment, where we can simply use Java 17 and the latest platform. As I said, we only need a way to run the build always with Java 17 (required by Tycho 3), but, for older target platforms, we'd like to use Java 11 to run tests in some jobs. Nothing more. To summarize, and maybe @cdietrich can confirm that, we DON'T want to ALWAYS use JDK 11 to compile. In my view, I'd have a profile where JDK11 is used from the toolchain to run Maven surefire tests and Tycho surefire tests, NOT by relying on properties passed from outside (even because the two plugins don't have such properties). If I understand it correctly, in that profile (you already have that) we just enable the toolchains plugins and configure maven-surefire and tycho-surefire appropriately (the latter doesn't even seem to require a configuration, since it's working now even though the property we passed is non-existent). Let's keep things simple ;) |
as long as we make sure we dont use java 12+ api i am fine with always compiling with J17 (i understood the -release option does this) |
If one want, a profile can be activated with a property so instead of trying to configure things via properties, use profiles activate by one. |
Yes, that's what I said: when we build with older platforms, we enable the toolchain to ensure that Java 11 is used to run the tests. With recent platforms, we use Java 17 (as we already do) The Note that I'm not completely excluding configuring the compiler to use JDK 11 when using the toolchain; I said we don't want always to do that. "Always = on every build = standard Maven run without profiles". |
@HannesWell I'm going to create a branch on top of yours to do some experiments (this will also allow me to test Jenkins better) |
@HannesWell , I'm doing a few experiments on a branch on top of yours. This is how I changed the Jenkinsfile: f82ef03 The POM has been changed accordingly: e6e649a then, I saw the message from @laeubi and I modified tycho compiler and surefire like that: a174c9a Looking at the log of the Jenkins job, when running with Java 11, I can see that maven and tycho both use JDK11. Now, I have to verify what's happening with JDK 17 (I only need to check that the empty argument in the Jenkinsfile does not generate any error). When using JDK 17, no toolchain is used. |
General guide for Maven toolchains: https://maven.apache.org/guides/mini/guide-using-toolchains.html Using the Maven-Toolchain-Plugin and its 'toolchain' goal the maven-compiler-plugin is configured to use a JDK from the toolchain that matches the specified release version. Configure the Tycho-Compiler-Plugin to use the JDK in the toolchain that matches the Bundle-RequiredExecutionEnvironment: https://tycho.eclipseprojects.io/doc/3.0.4/tycho-compiler-plugin/compile-mojo.html#useJDK Configure the Maven-Surefire-Plugin to use a specific JDK version from the toolchain to launch a test runtime: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jdkToolchain Configure the Tycho-Surefire-Plugin to use a specific JDK version from the toolchain to launch a test runtime: https://tycho.eclipseprojects.io/doc/3.0.4/tycho-surefire-plugin/test-mojo.html#useJDK About the toolchains.xml generated by the setup-java Github action and the exported JAVA_HOME_ environment variables: - https://github.com/actions/setup-java/#install-multiple-jdks - https://github.com/actions/setup-java/blob/main/docs/advanced-usage.md#Modifying-Maven-Toolchains Fixes eclipse-xtext#2216
Or the toolchain mechanism will make the compilation fail. Moreover, I don't think we need to use tycho-compiler-jdt for compiling our maven plugins
@HannesWell I created the branch https://github.com/eclipse/xtext/tree/hanneswell_tycho_3_0 on top of yours; I have already rebased that to main and remove the changes to GitHub Actions workflows (as I said, we don't need toolchains there). On Jenkins, everything seems to work fine even when the toolchain for JDK 11 is enabled. |
@LorenzoBettini Understand. I changed this PR to the minimum and as you requested only use the toolchain when Java-11 is requested. This means, if Java-17 is requested basically everything runs just like now and all projects are compiled with JDK-17 (and release=11 option) and all test runtimes use that JDK-17. If Java-11 is requested the only the That the simplest approach, i.e. the one with the least changes I found. Alternatively if you want that, if JDK_VERSION 11 is selected, JDK-17 is used to compile (with release option) and JDK-11 to run the tests, I could adapt the profile to select jdk version 17 as general toolchain (through Furthermore the GitHub workflow is now left unchanged and I reverted the change in the |
The I hope that addresses all of your remarks, but please let me know if I missed something. |
@HannesWell I meant that my branch was already working.. if you haven't rebased on top of that, it won't work because you didn't take other adjustments to our maven plugins. |
You are refering to the removal of the maven-compiler-plugin configuration in the Thanks for that hint! |
@HannesWell , please, rebase on top of my branch, don't squash my commits, and don't touch the Jenkinsfile or I'll get mad once again to test your PR on Jenkins. We'll clean the Jenkinsfile later |
... with the current state testing this PR on Jenkins should be much simpler, you just have to replay with the current content. The parameters are not changed compared to the main branch anymore. Furthermore I didn't squashed your commit, I cherry-picked them one by one and checked what is relevant and what is not, so that the back and forth does not end up in the git history. Sorry for making you mad by trying to help Xtext. I just want to respond, that it is not motivating to contribute to a project if being told that contributions are making the maintainers mad. |
@HannesWell , yes, please, rebase on top of my branch; you can also change the Jenkinsfile because in any case my branch is not on master so I'd have to change it with replay anyway. I'd like you to rebase on top of my branch so that I can see in the history a working but not optimal solution and then your changes to optimize/minimize the POMs. sorry if I sounded rude, I didn't mean to, I'm at the end of a long day with temperature. For sure, I didn't express myself correctly. For sure, without your work, help and contribution, toolchains would have never gotten into Xtext. |
ok, just did it. The result is the same when comparing the tip of the branch and main, as yesterday evening so you should see the difference. Eventually, i would be happy, if the commit-history is cleaned up before merge.
No problem, can happen. Didn't took it personally. Fully understand that the work on the pipeline can be hard and frustrating. I would do all the Replays myself, but only Committers are permitted to do that. |
The good news is the matrix build over all JDK and TP versions looks promising. On ubuntu all combinations, except for the incompatible combination of JDK-17 and latest tp, succeeded. |
@HannesWell I tested on Jenkins the two main scenarios and they work as expected! I thus merged the PR (actually rebased it). I then noted that you wanted to clear the commit history, but I'm perfectly fine with the current history, even taking the very first experiments. The build matrix in your branch should, of course, explicitly exclude the combination JDK11 and latest tp because that won't work as expected. It would be nice to have such a matrix in this repository, but due to the limitations of the usage of GitHub Actions shared by all Eclipse repositories, it wouldn't be feasible. Maybe just for Ubuntu and not macOS... maybe just nightly. We can investigate that in the future. Again, many thanks for your valuable contribution! |
Thank you for your support and all the Jenkins pipeline tests!
Lets do it here #2618 :) |
This PR migrates the xtext Maven+Tycho build to use Tycho 3.0.4 and sets up Maven toolchains. By using Maven toolschains one can ensure that all Plugins that only require Java-11 are build with a Java-11 JDK while at the same time the build is running in a Java-17 JRE (which is required since Tycho 3).
To enable that the
toolchains.xml
specifies a list of available JDK installations, which java version they implement and where they are located. The maven-toolchain-plugin then reads that file and tells all toolchain aware plugins, like themaven-compiler/surefire-plugin
ortycho-compiler/surefire-plugin
(when configured accordingly), where to find the corresponding JDK installation.With this change the
JDK_VERSION
parameter of Xtext's Jenkins-Pipeline only specifies the JDK version used in Test-runtimes (launched by the maven/tyhco-surefire-plugin). The compilation always happens with the Plugin's BREE (which is currently mainly Java-11).General guide for Maven toolchains:
https://maven.apache.org/guides/mini/guide-using-toolchains.html
Using the Maven-Toolchain-Plugin and its 'toolchain' goal the maven-compiler-plugin is configured to use a JDK from the toolchain that matches the specified release version.
Configure the Tycho-Compiler-Plugin to use the JDK in the toolchain that matches the Bundle-RequiredExecutionEnvironment:
https://tycho.eclipseprojects.io/doc/3.0.4/tycho-compiler-plugin/compile-mojo.html#useJDK
Configure the Maven-Surefire-Plugin to use a specific JDK version from the toolchain to launch a test runtime:
https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jdkToolchain
Configure the Tycho-Surefire-Plugin to use a specific JDK version from the toolchain to launch a test runtime:
https://tycho.eclipseprojects.io/doc/3.0.4/tycho-surefire-plugin/test-mojo.html#useJDK
About the toolchains.xml generated by the setup-java Github action and the exported JAVA_HOME_ environment variables:
Fixes #2216