feat: Throw invalid_time_zone exception from tzdb [4/4] #12475
Conversation
kevinwilfong
requested review from
majetideepak and
assignUser
as code owners
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D70344546 |
|
This pull request was exported from Phabricator. Differential Revision: D70344546 |
…bator#12475) Summary: Pull Request resolved: facebookincubator#12475 Differential Revision: D70344546
kevinwilfong
force-pushed
the
export-D70344546
branch
from
ce13f6f to
916d30e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D70344546 |
…bator#12475) Summary: Pull Request resolved: facebookincubator#12475 Differential Revision: D70344546
kevinwilfong
force-pushed
the
export-D70344546
branch
from
916d30e to
b73e483
Compare
|
This pull request was exported from Phabricator. Differential Revision: D70344546 |
…bator#12475) Summary: Pull Request resolved: facebookincubator#12475 Differential Revision: D70344546
kevinwilfong
force-pushed
the
export-D70344546
branch
from
b73e483 to
d25ff5c
Compare
There was a problem hiding this comment.
There is a bit of an inconsistent use of our custom cmake functions here. They are poorly documented, so it's on me ^^
The difference can only be seen when building with VELOX_MONO_LIBRARY=ON otherwise velox_add_library etc. are transparent wrappers around add_library etc..
When VELOX_MONO_LIBRARY=ON is set velox_add_library combines the sources and includes of all targets created with it into one libvelox.a . So we only want to use velox_add_library for targets that we want to be part of the main velox library itself, which means we don't want to use it for fuzzer, benchmarks, tests and other dev/test utils.
For the stuff in external it depends on if they are velox build time deps and the user can build against libvelox.a without them, then they can just be static external targets which the linker pulls the symbols from we need -> good for binary size.
If symbols from these libraries are included in out public api we will need to install these external targets to the user system, in that case making them part of libvelox might be a convenient shortcut for now. (maybe needs munging of symbols though to avoid clashes but that's not this PR).
Please choose one approach and use it consistently as mixing normal targets with velox_* functions leads to weird results.
| velox_include_directories(velox_external_date SYSTEM PUBLIC velox/external) | ||
| velox_compile_definitions(velox_external_date PRIVATE USE_OS_TZDB) | ||
| add_library(velox_external_date INTERFACE) | ||
| velox_include_directories(velox_external_date INTERFACE velox/external/date) |
There was a problem hiding this comment.
| velox_include_directories(velox_external_date INTERFACE velox/external/date) | |
| target_include_directories(velox_external_date INTERFACE velox/external/date) |
There was a problem hiding this comment.
Thanks for the explanation, I'll update #12422 with your suggestion.
…bator#12475) Summary: Pull Request resolved: facebookincubator#12475 facebookincubator#10654 introduced an invalid_timezone exception to the date library we pulled in. This PR makes an analogous change to the time zone library we pulled in from LLVM. It also restores code/tests that were disabled because this exception was missing. Reviewed By: yuandagits Differential Revision: D70344546
|
This pull request was exported from Phabricator. Differential Revision: D70344546 |
kevinwilfong
force-pushed
the
export-D70344546
branch
from
d25ff5c to
1016752
Compare
|
This pull request has been merged in b12d69f. |
Summary:
https://github.com/facebookincubator/velox/pull/10654 introduced an invalid_timezone exception
to the date library we pulled in. This PR makes an analogous change to the time zone library we
pulled in from LLVM. It also restores code/tests that were disabled because this exception was
missing.
Differential Revision: D70344546