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

[REVIEW] Remove unused native deps from java library [skip-ci] #5298

Merged
merged 6 commits into from
May 27, 2020

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented May 27, 2020

This removes nvstrings and nvcategory from what is packaged in the java jar. They are no longer used so we can remove them.

This also adds in packaging of libnvrtc if static linking is enabled. This is not 100% perfect as libnvrtc will do a dlopen on libnvrtc-builtins.so But it is a step in the right direction.

@revans2 revans2 requested a review from a team as a code owner May 27, 2020 14:44
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@revans2
Copy link
Contributor Author

revans2 commented May 27, 2020

Just to be clear I manually ran several tests to verify that nvstring and nvcategory are no longer needed by the java code.

java/pom.xml Outdated
@@ -231,6 +231,45 @@
</plugins>
</build>
</profile>
<profile>
<id>COPY_NVRTC</id>
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we need this change for this PR, or is this change also about static linking and marking dependencies as required and optional? My understanding of static linking is limited so pardon any naivety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes so we can statically link against cudart, but nvrtc which is the runtime compiler we cannot statically link against and it is tied to the cuda release version 10.02, 10.1, etc. This copies it into the jar when we are doing static linking.

@jlowe
Copy link
Member

jlowe commented May 27, 2020

It's important to note that technically libcudf.so still links against libNVCategory and libNVStrings. I get errors when I try to build libcudf.so without first building the nvstrings target. However neither string library appears in the dynamic dependencies of libcudf.so as reported by ldd.

java/pom.xml Outdated Show resolved Hide resolved
@revans2 revans2 merged commit f7470a8 into rapidsai:branch-0.14 May 27, 2020
@revans2 revans2 deleted the fix-deps branch May 27, 2020 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Affects Java cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants