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

[JENKINS-56018] ATH Docker image should not be using Module patches starting from 2.163 #478

Merged
merged 12 commits into from Feb 13, 2019

Conversation

@MRamonLeon
Copy link
Contributor

MRamonLeon commented Feb 11, 2019

See [JENKINS-56018] ATH Docker image should not be using Module patches starting from 2.163.

As of 2.163 the modules are not needed anymore. We change the documentation to reflect that, but also we write down how you need to run ATH in previous versions, as ATH could be used with whatever version.

To make it easy, we stay downloading the jaxb libraries to let just specify them in Jenkins < 2.163. In other way, the user will need to download them by him/herself.

WARNING: To be merged when Jenkins 2.164 is out.

@jenkinsci/java11-support @olivergondza @raul-arabaolaza

docs/JAVA11.md Outdated Show resolved Hide resolved
Copy link
Member

batmat left a comment

Given where we are now, I'm quite positive we should move everything convoluted in a addendump part.
I don't really think people would/should want to try anything < 2.164 on Java 11 if they don't know fully what they're doing.

So let's have docs explaining what people still need to do with Jenkins versions onward, and document (or not) the [2.155,2.163( specifics to keep the docs leaner.


Most likely, you will need to add some modules left apart from Java 9, like `JAVA_OPTS="--add-modules java.sql"` used by _pipeline-maven-plugin_ for example. It depends on the Jenkins version and the plugins to test. You can find the most updated documentation on how to run Jenkins on Java 11 at: https://jenkins.io/doc/administration/requirements/jenkins-on-java-11. Basically:

For Jenkins [2.155, 2.163), add modules, _enable-future-java_ and dependencies from tests:

This comment has been minimized.

Copy link
@batmat

batmat Feb 11, 2019

Member

I would simply strongly recommend avoiding these versions on Java 11. Only "experts" should try them (or we put this in some addendum, not in the main docs flow IMO).


For Jenkins [2.155, 2.163), add modules, _enable-future-java_ and dependencies from tests:
```bash
ath-user@1803848e337f:~/ath-sources$ env JAVA_OPTS=\"${JAVA_OPTS} -p /home/ath-user/jdk11-libs/jaxb-api.jar:/home/ath-user/jdk11-libs/javax.activation.jar --add-modules java.xml.bind,java.activation -cp /home/ath-user/jdk11-libs/jaxb-impl.jar:/home/ath-user/jdk11-libs/jaxb-core.jar\" JENKINS_OPTS=\"--enable-future-java\" ./run.sh ...

This comment has been minimized.

Copy link
@batmat

batmat Feb 11, 2019

Member

Following my previous comment, I would say we should now put this only as an addendum or so, and not use --enable-future-java (or possibly simpler -e JENKINS_ENABLE_FUTURE_JAVA=true.

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

MRamonLeon commented Feb 11, 2019

Regarding @batmat comments, I think the documentation about how to run jenkins on java 11 is short enough to keep the instructions depending on the Jenkins version. There are already people testing Jenkins < 2.164 on java11 and this kind of documentation could be very valuable for them.

In the other hand, if we want to avoid testing Jenkins < 2.164 on Java 11 due to the complexity, we should avoid talking about that in the rest of documentation already published. Because if we state it works on 2.155 and someone comes to test ATH in such version, the instructions should be clear enough.

I'm open to reduce the documentation as soon as we do the same in the other docs.

Copy link
Member

alecharp left a comment

I generally agree with @batmat. We should stick to latest weekly available as simpler to use. It's generally discourage to build plugins for weekly releases anyway.

@@ -4,18 +4,33 @@ To run the tests using a Java 11 virtual machine:

1. On the host, launch the container with Java11 VM:


This comment has been minimized.

Copy link
@alecharp

alecharp Feb 11, 2019

Member

not needed ;)

.gitignore Show resolved Hide resolved
@raul-arabaolaza

This comment has been minimized.

Copy link
Contributor

raul-arabaolaza commented Feb 11, 2019

I strongly disagree to remove doc related to "old" versions of core, there are people building things on top of LTS and those "old" versions and more than probably they would like to start testing with java 11 as soon as possible. Removing doc and saying "you should be using this version of the core" is going to make things harder for them... unless the whole point here is that java 11 testing is meaningless unless you are on core 2.164

@alecharp

This comment has been minimized.

Copy link
Member

alecharp commented Feb 11, 2019

@raul-arabaolaza I strongly disagree.

We don't plan to remove how to use ATH with any LTS (there is no LTS with Java 11 yet). But we don't recommand to use any Jenkins version prior to 2.164 because it is way easier to use than the [2.155,2.163] versions. So, yes for the moment it means we recommend to use only 1 version of Jenkins core to run the tests on Java 11.

But it's for the good of the developers experience.

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

MRamonLeon commented Feb 12, 2019

As commented, because the statement is that the recommended version to test Java11 is 2.164, I keep the documentation simple, and I link to the Jenkins documentation to get further information on how to run Jenkins on Java 11

@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

MRamonLeon commented Feb 12, 2019

Removed the jaxb params and --enable-future-java in the Jenkinsfile

@batmat batmat self-requested a review Feb 13, 2019
@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

MRamonLeon commented Feb 13, 2019

I think the comments are addressed @alecharp @batmat , but the CI is throwing:

workflow-aggregator plugin not installed
...
workflow-support version 3.1 is missing. To fix, install version 3.1 or later.

https://ci.jenkins.io/job/Core/job/acceptance-test-harness/view/change-requests/job/PR-478/4/testReport/plugins/AnalysisCollectorPluginTest/java_11_split0___should_compute_annotations_on_workflow/

I need to review that, any idea?

@batmat
batmat approved these changes Feb 13, 2019
Copy link
Member

batmat left a comment

I'm not going to block this given this is a dev tool, but many of my comments are still unaddressed AFAICT, and I still think the docs are wrong currently:
image

  • "most likely" about the modules is already history
  • I feel strongly at least we should have put 2.164+ first, and put the rest as clear addendum, telling people that running anything before 2.164 on Java 11 is most likely going to be a pure waste of time, if they don't know exactly what and why they're doing it.
@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

MRamonLeon commented Feb 13, 2019

@batmat Have you refreshed? I do think I already changed it.

@olivergondza

This comment has been minimized.

Copy link
Member

olivergondza commented Feb 13, 2019

Thank you Ramon, this makes things cleaner.

@olivergondza olivergondza merged commit 8a91d94 into jenkinsci:master Feb 13, 2019
1 check failed
1 check failed
continuous-integration/jenkins/pr-merge This commit has test failures
Details
@MRamonLeon

This comment has been minimized.

Copy link
Contributor Author

MRamonLeon commented Feb 15, 2019

Thank you @olivergondza. Can we have a docker image with these changes? It would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.