Skip to content

Conversation

@pzygielo
Copy link
Contributor

@pzygielo pzygielo commented Jun 4, 2020

@olamy may I ask for review, please?

@khmarbaise khmarbaise self-requested a review June 4, 2020 16:32
Copy link
Member

@khmarbaise khmarbaise left a comment

Choose a reason for hiding this comment

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

It would be great if you could squash the commits into a single one. Apart from that looks fine for me. When you have squashed we can check on ASF CI ...

@khmarbaise
Copy link
Member

@olamy WDYT?

@pzygielo
Copy link
Contributor Author

pzygielo commented Jun 4, 2020

@khmarbaise - commits squashed

@pzygielo
Copy link
Contributor Author

pzygielo commented Jun 5, 2020

  1. so it seems that (failed) 🔴 build 1 ran against 4 commits (it should not matter as contents is the same)
  2. I executed verify -P run-its (before push) locally on linux/jdk8 and as on Jenkins - it's ok in my env
  3. on first read of 1/consoleFull failure is not obvious for me.
    I suspected that new class from jakarta
$ file WebServlet.class 
WebServlet.class: compiled Java class data, version 52.0 (Java 1.8)

could not collaborate with JDK7.:

Caused by: java.lang.UnsupportedClassVersionError: jakarta/servlet/annotation/WebServlet : Unsupported major.minor version 52.0
...
    at org.apache.maven.plugins.war.WarMojo.hasClassInClasspath (WarMojo.java:341)

but a lot more ITs fail with JDK7, not new one only, so I'm a bit confused...


Some ITs fail locally due to no other reason but

Caused by: javax.net.ssl.SSLException: Received fatal alert: protocol_version

for artifacts that are missing in my repository, and maven on JDK7 can't establish connection with central. Setting -Dhttps.protocols=TLSv1.2 doesn't help, but getting dependency/plugin in other way into repository - does.

@pzygielo
Copy link
Contributor Author

pzygielo commented Jun 5, 2020

In blue ocean/pipeline view it's clear that JDK7 is common factor.

@pzygielo pzygielo marked this pull request as draft June 5, 2020 08:13
@hboutemy
Copy link
Member

hboutemy commented Jun 5, 2020

if the new IT requires Java 8, you can mark it with invoker.properties
see for example https://github.com/apache/maven-pmd-plugin/blob/master/src/it/MPMD-302-JDK14/invoker.properties

@pzygielo
Copy link
Contributor Author

pzygielo commented Jun 5, 2020

if the new IT requires Java 8, you can mark it with invoker.properties
see for example https://github.com/apache/maven-pmd-plugin/blob/master/src/it/MPMD-302-JDK14/invoker.properties

Thanks, that's what I did in #9.

@pzygielo
Copy link
Contributor Author

pzygielo commented Jun 5, 2020

@hboutemy - but with your confirmation that it's the way to go I'll squash #9 here and push new commit for review.

@pzygielo pzygielo marked this pull request as ready for review June 5, 2020 16:19
@pzygielo pzygielo requested a review from khmarbaise June 5, 2020 16:19
@hboutemy
Copy link
Member

hboutemy commented Jun 5, 2020

IMHO, no need for UnsupportedClassVersionError check: this won't happen for end-users
limiting the execution of the IT to 8+ JDK versions is sufficient

@pzygielo
Copy link
Contributor Author

pzygielo commented Jun 5, 2020

IMHO, no need for UnsupportedClassVersionError check: this won't happen for end-users
limiting the execution of the IT to 8+ JDK versions is sufficient.

Just to clarify my understanding: if there is jakarta's web servlet annotation in CP (only then plugin would load this class), it means also that project is already compiled with JDK8+. Right? In such case, catching UCVE isn't needed indeed!

- Use MWAR-396_servlet30 as IT for this issue
- Extract method to check for Servlet 3.0+
- Extract method to check for class
- Check for Jakarta annotation as well
- Require at least JDK8 to use jakarta servlet API
Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this.

@olamy olamy merged commit 57589c9 into apache:master Jun 6, 2020
@pzygielo pzygielo deleted the mwar-430 branch June 6, 2020 07:35
@jira-importer
Copy link

Resolve #495

1 similar comment
@jira-importer
Copy link

Resolve #495

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.

5 participants