ls-hpack icon indicating copy to clipboard operation
ls-hpack copied to clipboard

The library generated by the CMakeLists file rules may have import problems.

Open CandyMi opened this issue 5 years ago • 9 comments

A little suggestion about CMakeLists File:

  • Is it possible to use a macro to decide whether to ignore RPATH?

  • To determine whether xxhash can consider using find_package?

CandyMi avatar Nov 05 '20 18:11 CandyMi

Thank you for the suggestions. The current setup works well for us. Could you please elaborate:

  1. What is RPATH and why should one ignore it (or not)?
  2. I don't think xxhash has been packaged for any platform, it's too small -- or has it?

dtikhonov avatar Nov 12 '20 03:11 dtikhonov

xxhash has been packaged in Fedora since Fedora 31, some three years ago. https://src.fedoraproject.org/rpms/xxhash

xxhash has been packaged in Debian since Debian 9 Stretch, released almost two years ago. https://tracker.debian.org/pkg/xxhash

(This is not a complete list, but merely a 2 out of 2 positive hit for presence of xxhash packages)

gstrauss avatar Nov 12 '20 07:11 gstrauss

First, If you need to rely on third-party libraries, then RPATH should add some search paths, such as: /usr/local or /usr/local/opt, etc.

Secondly, since we all think that xxhash is already a mature product. This should be considered to be included in the find_package scope instead of copy alone

CandyMi avatar Nov 12 '20 07:11 CandyMi

I stand corrected! Indeed, xxhash is packaged for Ubuntu as well.

Regarding xxhash being a mature product: the API may be the same, but ls-hpack relies on particular values of xxhash internally. How can we guarantee that the installed version of xxhash will produce the same values?

And I still don't know what RPATH is.

Let's do this: if you have a specific improvement in mind, please propose a PR.

dtikhonov avatar Nov 12 '20 17:11 dtikhonov

Regarding xxhash being a mature product: the API may be the same, but ls-hpack relies on particular values of xxhash internally.

ls-hpack/deps/xxhash/xxhash.[ch] appears to come from https://github.com/Cyan4973/xxHash near d8c04c12 (and then someone edited, or someone's editor modified some formatting in ls-hpack/deps/xxhash/xxhash.[ch]) Other than some formatting changes, the code is identical, and commit d8c04c12 is very, very, very old (23 Nov 2014).

Newer versions of xxhash move the implementation into xxhash.h, and work with ls-hpack in my tests against xxhash tag: v0.8.0

@dtikhonov is there a unit test in ls-hpack which can be used to validate the hard-coded hash values in ls-hpack?

gstrauss avatar Nov 12 '20 20:11 gstrauss

-Wl,-rpath specifies that the dynamic library will be searched for dependent library path only when it is run or imported.

Previously, I suggested to use find_package to check dependent libraries (xxhash), which is to use -Wl,-rpath.

The package management used by different operating systems and its installation path will also be different, I would think that using the rpath specification is the easiest choice.

Please tell me if there is a better way, because I will only use this way. :)

CandyMi avatar Nov 13 '20 02:11 CandyMi

@CandyMi please re-read the posts by @dtikhonov in which he clearly states that this is not his area of expertise. He politely requested that you propose a patch.

gstrauss avatar Nov 13 '20 03:11 gstrauss

is there a unit test in ls-hpack which can be used to validate the hard-coded hash values in ls-hpack?

test/test_hpack.c should do that. To check, I just changed the seed, recompiled, and all tests failed.

dtikhonov avatar Nov 17 '20 04:11 dtikhonov

Slightly off-topic, but important if using a different library version of xxHash: FYI: I updated deps/xxhash/xxhash.[ch] to versions from xxHash tag v0.8.0. make test passes all tests, so the hash calculations must produce the same results. (However, that I was able to run the tests only after I commented out some code in auxilary program bin/calc-xxh.c, which no longer compiles with xxHash v0.8.0 due to XXH32_state_t no longer being a complete public type.)

gstrauss avatar Nov 17 '20 14:11 gstrauss