vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532402335
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
I agree, this would be the right solution. I didn't do it initially because
1) I was worried about breaking existing users and 2) I didn't know how far the
scope of this PR should go. In hindsight, point 1 is moot because this package
wasn't previously importable anyway, so even with adding the namespace existing
consumers would still see the un-namespaced targets. For point 2, IMO best
practice would be to update all the examples/code within the repo to use the
namespaced target consistently whether they're getting nanoarrow via
FetchContent or `find_package`. This would be especially important for tools
that rely on the former as a fallback for the latter, either manually or via
something like CPM. However, I think that's probably out of scope for this PR
and can be done in some future work. Would you agree?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
