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

Fix zstd/zdict include path for java static build #3260

Closed
wants to merge 2 commits into from

Conversation

sagar0
Copy link
Contributor

@sagar0 sagar0 commented Dec 12, 2017

With the ZSTD dictionary generator support added in #3057
PORTABLE=1 ROCKSDB_NO_FBCODE=1 make rocksdbjavastatic fails as it can't find zdict.h. Specifically due to:

#include <zdict.h>

In java static builds zstd code gets directly downloaded from https://github.com/facebook/zstd , and in there zdict.h is under dictBuilder directory. So, I modified libzstd.a target to use make install to collect all the header files into a single location and used that as the zstd's include path.

Test Plan:
PORTABLE=1 ROCKSDB_NO_FBCODE=1 make rocksdbjavastatic
all java tests: PORTABLE=1 ROCKSDB_NO_FBCODE=1 make jtest
Without PORTABLE and ROCKSDB_NO_FBCODE: make rocksdbjavastatic
Dynamic: make rocksdbjava

@sagar0
Copy link
Contributor Author

sagar0 commented Dec 12, 2017

#3057 is not in 5.9 branch. So we don't need this fix backported to 5.9.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

more generally, you could do a DESTDIR="./zstd-$(ZSTD_VER)" make install and then use headers/libraries under that directory the same way we do for system-installed zstd.

@ajkr
Copy link
Contributor

ajkr commented Dec 15, 2017

btw you can assign to me if you want since I broke it. or land as-is is fine too.

@sagar0
Copy link
Contributor Author

sagar0 commented Jan 5, 2018

The current fix works, but I am trying to see how to make use of make install as @ajkr suggested.

@facebook-github-bot
Copy link
Contributor

@sagar0 has updated the pull request.

@sagar0
Copy link
Contributor Author

sagar0 commented Jan 5, 2018

Changed libzstd.a target to use make install inside zstd/lib directory instead of doing a make all. This has the advantage of collecting all header files in one location, and future-proofs us if we end up using any new headers introduced by zstd.

After the build:

svemuri@devbig187 ~/rocksdb/zstd-1.2.0/lib/include (zstd-java-static)  $ ls -l
total 76
-rw-r--r--. 1 svemuri users 11408 Jan  5 13:54 zbuff.h
-rw-r--r--. 1 svemuri users 10727 Jan  5 13:54 zdict.h
-rw-r--r--. 1 svemuri users  2499 Jan  5 13:54 zstd_errors.h
-rw-r--r--. 1 svemuri users 47252 Jan  5 13:54 zstd.h

@adamretter
Copy link
Collaborator

@sagar0 So the destination for make install is inside the temp build folder... right?

@sagar0
Copy link
Contributor Author

sagar0 commented Jan 5, 2018

That's right. DESTDIR will be zstd-1.2.0/lib (.) and all the artifacts will be 'installed' in this directory.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sagar0
Copy link
Contributor Author

sagar0 commented Jan 5, 2018

Thanks @adamretter .

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@sagar0 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sagar0 sagar0 deleted the zstd-java-static branch January 5, 2018 23:52
siying pushed a commit that referenced this pull request Jan 11, 2018
Summary:
With the ZSTD dictionary generator support added in #3057
`PORTABLE=1 ROCKSDB_NO_FBCODE=1 make rocksdbjavastatic` fails as it can't find zdict.h. Specifically due to:
https://github.com/facebook/rocksdb/blob/e3a06f12d27fd50af7b6c5941973f529601f9a3e/util/compression.h#L39
In java static builds zstd code gets directly downloaded from https://github.com/facebook/zstd , and in there zdict.h is under dictBuilder directory. So, I modified libzstd.a target to use `make install` to collect all the header files into a single location and used that as the zstd's include path.
Closes #3260

Differential Revision: D6669850

Pulled By: sagar0

fbshipit-source-id: f8a7562a670e5aed4c4fb6034a921697590d7285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants