-
Notifications
You must be signed in to change notification settings - Fork 52
HBASE-29354 Jetty12 dependencies compiled with JDK17 violate hbase-thirdparty bytecode restrictions #139
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
HBASE-29354 Jetty12 dependencies compiled with JDK17 violate hbase-thirdparty bytecode restrictions #139
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,21 +127,21 @@ | |
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-ee</artifactId> | ||
| <version>${jetty-12-plus.version}</version> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-api</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-xml</artifactId> | ||
| <version>${jetty-12-plus.version}</version> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-ee</artifactId> | ||
| <version>${jetty-12-plus.version}</version> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-api</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-xml</artifactId> | ||
| <version>${jetty-12-plus.version}</version> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.slf4j</groupId> | ||
|
|
@@ -200,7 +200,7 @@ | |
| produce. See below for how to exclusion of transitive dependencies. | ||
| --> | ||
| <exclude>org.slf4j:slf4j-api</exclude> | ||
| <!-- On the "next" build, exclude a lingering shaded jar if it exists (user did not `clean`). | ||
| <!-- On the "next" build, exclude a lingering shaded jar if it exists (user did not `clean`). | ||
| Maven will happily pick up the previous shaded jar and try to include that in the N+1th build | ||
| if we don't exclude it. This will result in a failure in the ServicesResourceTransformer claiming | ||
| that we've already packaged a services file once. --> | ||
|
|
@@ -218,6 +218,23 @@ | |
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| <!-- This module relocates Jetty 12+ EE8 dependencies compiled with JDK 17, hence, overriding the project-level | ||
| restriction on maxJdkVersion of 'compileSource'! See HBASE-29354--> | ||
| <plugin> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here we do not need to compile this, just relocate the pre compiled .class file, which means we do not need to compile this module with JDK17 right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we do not compile with jdk 17 here. I had even tested out most flows on master even without this change, and things looked good. Given we wont use these modules for branch 2, shouldnt we be fine with this? If you think this is not an acceptable approach and we need to think deeper on this, should I drop jetty 12 modules from our main pom temporarily? Or i could rever the jetty change also. We need release for branch 2.x. This is blocking the upcoming release!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could revert the jetty changes first, make thirdparty 4.1.11 a regular release so we can make the 2.6.x and 2.5.x release, and then back to the jetty 12 related things for 3.x. WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure sounds good to me to unblock, let me revert jetty commit, we can come back here once done with 2.x release.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better also mention this on the discussion thread on mailing list.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes sure let me put up over there. Also, I have raised PR to revert at #140
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done! |
||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-enforcer-plugin</artifactId> | ||
| <configuration> | ||
| <rules> | ||
| <enforceBytecodeVersion> | ||
| <maxJdkVersion>17</maxJdkVersion> | ||
| <ignoreOptionals>true</ignoreOptionals> | ||
| <ignoredScopes> | ||
| <ignoredScope>test</ignoredScope> | ||
| </ignoredScopes> | ||
| </enforceBytecodeVersion> | ||
| </rules> | ||
| </configuration> | ||
| </plugin> | ||
| </plugins> | ||
| </build> | ||
| </project> | ||
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.
unrelated spotless issue, can also fix as another commit