fix cmake export for 3rd party importers on linux/unix #45
Conversation
|
Hi Robert. Thanks for the PR and sorry for the delayed response. I've reviewed your PR and have a few questions (in the comments). Our plan right now is to incorporate your changes into our internal development branch for inclusion in our 2.2 release. |
| @@ -187,11 +187,11 @@ nvcompStatus_t nvcompBitcompCompressAsync( | |||
| return nvcompErrorInternal; | |||
| if (bitcompSetStream(handle, stream) != BITCOMP_SUCCESS) | |||
| return nvcompErrorInvalidValue; | |||
| if (bitcompCompressLossless(handle, in_ptr, out_ptr) != BITCOMP_SUCCESS) | |||
| if (bitcompCompressLossless(handle, in_ptr, (char*)out_ptr) != BITCOMP_SUCCESS) | |||
There was a problem hiding this comment.
The function signature for bitcompCompressLossless takes in a void*. Why do you need the cast? This applies to the other cast changes as well.
There was a problem hiding this comment.
I don't remember why I made this change. The best I can guess is that the output pointer was previously a char*, but I can't find it now in the few minutes that I have before my flight.
| ) | ||
| if(UNIX) | ||
| target_include_directories(nvcomp PUBLIC | ||
| "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>" |
There was a problem hiding this comment.
Can you confirm that this is your required change? The rest of the cmake changes are optional, right?
There was a problem hiding this comment.
@eschmidt-nvidia It's been a while since I wrote this patch so my memory is a bit foggy. As I recall, this isn't the only required change. Specifically, including GNUInstallDirs where it was previously included meant that some of the variables were not defined in the right place
The CMAKE_EXPORT_COMPILE_COMMANDS is optional, but enables some tooling.
|
@eschmidt-nvidia Thanks for looking at the patch. I think everyone has been busy. I am just glad you eventually got around to it 😄 . |
|
@eschmidt-nvidia I just wanted to ping you after the holidays and check on the status of this. |
Hi Robert, I’ve included this in an internal PR targeting 2.2. Plan is to include this in the 2.2 release later this month / early Feb. I’ll have a release candidate for you to try out by the end of the week. |
|
This PR has been labeled |
|
Just waiting for things to merge. Commenting to remove the stale tag. |
|
Included in v2.2 |
Previously, the installed include directory was not added to the PUBLIC interface of the installed library preventing users from using find_package(nvcomp); target_link_libraries(foo PRIVATE nvcomp::nvcomp) if the library was installed to a non-standard include location (i.e. with
spackor~/.local/include)