Conversation
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
needs-ci
| 'action_name': 'make_temporal_zoneinfo_cpp', | ||
| 'inputs': [ | ||
| '<(V8_ROOT)/tools/include-file-as-bytes.py', | ||
| '../../deps/crates/vendor/zoneinfo64-v0_3/src/data/zoneinfo64.res', |
There was a problem hiding this comment.
This path may change after git node v8 major if zoneinfo64 receives a version bump in the Rust deps.
Also, will this fail in the case where a slim tarball is being used with shared Temporal?
There was a problem hiding this comment.
This path may change after
git node v8 majorif zoneinfo64 receives a version bump in the Rust deps.
Sure, in this case the path would need to be updated. Are you saying that so we take some preemptive action?
Also, will this fail in the case where a slim tarball is being used with shared Temporal?
Probably, worth noting that slim tarball is not something we distribute, and that limits what build flag can be used (e.g. you have to pass a bunch of --shared-• flags), so it's not really a concern to break that IMO. What would be alternative anyway?
There was a problem hiding this comment.
Sure, in this case the path would need to be updated. Are you saying that so we take some preemptive action?
Just thinking in terms of maintainer quality of life, that's all – we automated this in crates.gyp as part of the upgrade process, might be worth thinking about.
The other thing is that the bundled .res with the zoneinfo64 crate is advertised as for testing only – it's probably fine, but there are no promises that it'll be kept up-to-date. An alternative would be using the ICU source itself, much as GN does, eg. 3d85ecd.
There was a problem hiding this comment.
So you're thinking we should move the rule to crates.gyp? That might work.
Worth keeping in mind that currently the build is plain broken, trying to build Node.js with --without-intl --v8-enable-temporal-support does not compile. So IMO this PR is a net improvement over the status quo, and we can make improvements in follow up PRs. (Or drop support for --without-intl if we don't care about making it work)
There was a problem hiding this comment.
I think this could be moved to crates.gyp and, or move the path to be a gyp variable, so that the variable could be updated by a script.
There was a problem hiding this comment.
An alternative would be using the ICU source itself, much as GN does, eg. 3d85ecd.
Thinking about it, I am kinda leaning more towards the "store a copy of tzdata's zoneinfo in /tools/icu" approach – seems like it'd be less hassle from a gyp standpoint, and would also be guaranteed to be kept up-to-date.
4 participants
Refs: #62676