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
add Crc32c to tiledb build via ExternalProject_Add #3455
Conversation
This pull request has been linked to Shortcut Story #20447: Add Crc32 to build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after addressing these issues, and a rebase:
- Minor nits inline
- I don't think this needs to be user-visible at all (bootstrap). We can turn it on automatically when GCS is enabled, in the future.
cmake/Modules/FindCrc32c_EP.cmake
Outdated
# Finds the Crc32c library, installing with an ExternalProject as necessary. | ||
# This module defines: | ||
# - Crc32c_INCLUDE_DIR, directory containing headers | ||
# - Crc32c_LIBRARIES, the Magic library path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nig: Magic -> Crc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addition of the link test is currently subject to same flags as building being activated by bootstrap[.ps1]
If I take away those switches/visibility, how do you want to handle building/link-test enabling?
(The code would be present, but no way to activate it...?)
[edit - Occurs to me 'later' with variable left in place, can invoke cmake directly, although this will require knowledge of any (additional) vars and cmake parameters (on windows, -X A64? others?) that may have to be specified to obtain other 'normal' default settings that may be required (several cmake variables are defaulted to ON by bootstrap.ps1).]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of enabling via cmake -D...
, but you're right, we don't invoke it directly. For now let's prefix the option with _
so we can use it in CI and remove later once it is implicitly enabled/tested via -DTILEDB_GCS=ON
.
…nt build of crc32 until gcs eventually starts to need
add Crc32c to tiledb build via ExternalProject_Add
(to be merged after #3454, abseil/absl added to build)
TYPE: FEATURE
DESC: add Crc32c to tiledb build via ExternalProject_Add