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

[SPARK-28765][BUILD] Add explict exclusions to avoid JDK11 dependency issue #25481

Closed
wants to merge 2 commits into from
Closed

[SPARK-28765][BUILD] Add explict exclusions to avoid JDK11 dependency issue #25481

wants to merge 2 commits into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Aug 17, 2019

What changes were proposed in this pull request?

This PR adds explicit exclusions to avoid Maven JDK11 dependency issues.

Why are the changes needed?

Maven/Ivy seems to be confused during dependency generation on JDK11 environment.
This is not only wrong, but also causes a Jenkins failure during dependency manifest check on JDK11 environment.

JDK8

$ cd core
$ mvn -X dependency:tree -Dincludes=jakarta.activation:jakarta.activation-api
...
[DEBUG]       org.glassfish.jersey.core:jersey-server:jar:2.29:compile (version managed from 2.22.2)
[DEBUG]          org.glassfish.jersey.media:jersey-media-jaxb:jar:2.29:compile
[DEBUG]          javax.validation:validation-api:jar:2.0.1.Final:compile

JDK11

[DEBUG]       org.glassfish.jersey.core:jersey-server:jar:2.29:compile (version managed from 2.22.2)
[DEBUG]          org.glassfish.jersey.media:jersey-media-jaxb:jar:2.29:compile
[DEBUG]          javax.validation:validation-api:jar:2.0.1.Final:compile
[DEBUG]          jakarta.xml.bind:jakarta.xml.bind-api:jar:2.3.2:compile
[DEBUG]             jakarta.activation:jakarta.activation-api:jar:1.2.1:compile

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Do the following in both JDK8 and JDK11 environment. The dependency manifest should not be changed. In the current master branch, JDK11 changes the dependency manifest.

$ dev/test-dependencies.sh --replace-manifest

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-28765][BUILD] Add explict exclusions to avoid JDK11 dependency… [SPARK-28765][BUILD] Add explict exclusions to avoid JDK11 dependency issue Aug 17, 2019
@dongjoon-hyun
Copy link
Member Author

cc @srowen , @HyukjinKwon , @wangyum .

@SparkQA
Copy link

SparkQA commented Aug 17, 2019

Test build #109260 has finished for PR 25481 at commit c037543.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member

wangyum commented Aug 17, 2019

Is it safe?

        <profile>
            <id>jdk11+</id>
            <activation>
                <jdk>[11,)</jdk>
            </activation>
            <dependencies>
                <dependency>
                    <groupId>jakarta.xml.bind</groupId>
                    <artifactId>jakarta.xml.bind-api</artifactId>
                </dependency>
                <dependency>
                    <groupId>com.sun.xml.bind</groupId>
                    <artifactId>jaxb-osgi</artifactId>
                    <scope>test</scope>
                </dependency>
            </dependencies>
        </profile>

https://github.com/eclipse-ee4j/jersey/blob/2.29/core-server/pom.xml#L229-L245

@dongjoon-hyun
Copy link
Member Author

Oh, thank you for the pointer, @wangyum . I only checked only https://mvnrepository.com/artifact/org.glassfish.jersey.core/jersey-server/2.29 before. I'll take a look at that.

@HyukjinKwon
Copy link
Member

I think it's generally fine if the tests we run pass though.

@dongjoon-hyun
Copy link
Member Author

If we don't use that, yes it is.

For now, it seems that this situation is designed like this. In worst case, it seems that we end up with having manifest per JDK. And, pre-built artifacts per JDK How do you think about this situation? @srowen ?

@dongjoon-hyun
Copy link
Member Author

For now, I'll close this PR. Thank you for review, @HyukjinKwon and @wangyum .

@HyukjinKwon
Copy link
Member

Given the test, this PR still looks good to me.

@SparkQA
Copy link

SparkQA commented Aug 17, 2019

Test build #109277 has finished for PR 25481 at commit 7f24d26.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 17, 2019

Test build #109276 has finished for PR 25481 at commit c037543.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Aug 17, 2019

Well the other solution is to explicitly include these additional libraries as a runtime dependency. Then the dependencies are the same too. We don't need a new profile. That's 'safer' I guess.

This approach is also OK if they really aren't used given how Jersey is used in Spark, but do we know that? Java 9 kicked out the JAXB libraries from the JDK, so it's possible they are there on purpose.

The Java 8 PR builder here passes of course. If it has been manually checked against JDK 11 and tests work, I'm OK with it. We'd know soon if it doesn't, assuming the JDK 11 Jenkins job is in operation. If it fails I think we'd fall back to the other approach above.

Anyway, I'm OK with it if there's any evidence it works on JDK 11, otherwise, might suggest adding the dependencies instead.

@HyukjinKwon
Copy link
Member

It was tested against JDK 11 at #25443 (comment) but yes explicitly including sounds fine to me too

@srowen
Copy link
Member

srowen commented Aug 17, 2019

I'm OK with it then.

@dongjoon-hyun
Copy link
Member Author

Then, thank you, @srowen , @HyukjinKwon , @wangyum .
I'll merge this and follow this explicit testing and exclusion way for these three.
We can switch to explicit inclusion always if required.

@dongjoon-hyun dongjoon-hyun deleted the SPARK-28765 branch August 17, 2019 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants