build: Auto-load ICU data from --with-icu-default-data-dir #30825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
build
srl295
requested changes
Dec 6, 2019
bnoordhuis
reviewed
Dec 7, 2019
sgallagher
force-pushed
the
i18n-default-dir
branch
from
b97c862
to
8d43603
Compare
sgallagher
changed the title
|
I updated the patch with the code review recommendations. I also changed the commit message and the first comment on this PR to match it and provide more detail on the problem this is solving. Thank you very much for the review! |
|
Shouldn't this target master? |
sgallagher
force-pushed
the
i18n-default-dir
branch
from
8d43603
to
288502a
Compare
sgallagher
force-pushed
the
i18n-default-dir
branch
from
288502a
to
46bf401
Compare
|
Rebased it to master. I was working on it against v12 originally because a) that's what we have in Fedora and b) the master branch wasn't compiling successfully for me, but that's been fixed in the interim it seems. |
sgallagher
force-pushed
the
i18n-default-dir
branch
from
46bf401
to
e40a677
Compare
|
fyi @nodejs/intl |
bnoordhuis
approved these changes
Dec 12, 2019
When compiled with `--with-intl=small` and `--with-icu-default-data-dir=PATH`, Node.js will use PATH as a fallback location for the ICU data. We will first perform an access check using fopen(PATH, 'r') to ensure that the file is readable. If it is, we'll set the icu_data_directory and proceed. There's a slight overhead for the fopen() check, but it should be barely measurable. This will be useful for Linux distribution packagers who want to be able to ship a minimal node binary in a container image but also be able to add on the full i18n support where needed. With this patch, it becomes possible to ship the interpreter as /usr/bin/node in one package for the distribution and to ship the data files in another package (without a strict dependency between the two). This means that users of the distribution will not need to explicitly direct Node.js to locate the ICU data. It also means that in environments where full internationalization is not required, they do not need to carry the extra content (with the associated storage costs). Refs: nodejs#3460 Signed-off-by: Stephen Gallagher <[email protected]>
sgallagher
force-pushed
the
i18n-default-dir
branch
from
e40a677
to
e610b98
Compare
|
Rebased atop the latest master and made the two minor changes @bnoordhuis requested. |
bnoordhuis
added
author ready
richardlau
approved these changes
Dec 13, 2019
Trott
approved these changes
Dec 13, 2019
|
Landed in d502b83 |
Trott
pushed a commit
that referenced
this pull request
Dec 14, 2019
When compiled with `--with-intl=small` and `--with-icu-default-data-dir=PATH`, Node.js will use PATH as a fallback location for the ICU data. We will first perform an access check using fopen(PATH, 'r') to ensure that the file is readable. If it is, we'll set the icu_data_directory and proceed. There's a slight overhead for the fopen() check, but it should be barely measurable. This will be useful for Linux distribution packagers who want to be able to ship a minimal node binary in a container image but also be able to add on the full i18n support where needed. With this patch, it becomes possible to ship the interpreter as /usr/bin/node in one package for the distribution and to ship the data files in another package (without a strict dependency between the two). This means that users of the distribution will not need to explicitly direct Node.js to locate the ICU data. It also means that in environments where full internationalization is not required, they do not need to carry the extra content (with the associated storage costs). Refs: #3460 Signed-off-by: Stephen Gallagher <[email protected]> PR-URL: #30825 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Dec 17, 2019
When compiled with `--with-intl=small` and `--with-icu-default-data-dir=PATH`, Node.js will use PATH as a fallback location for the ICU data. We will first perform an access check using fopen(PATH, 'r') to ensure that the file is readable. If it is, we'll set the icu_data_directory and proceed. There's a slight overhead for the fopen() check, but it should be barely measurable. This will be useful for Linux distribution packagers who want to be able to ship a minimal node binary in a container image but also be able to add on the full i18n support where needed. With this patch, it becomes possible to ship the interpreter as /usr/bin/node in one package for the distribution and to ship the data files in another package (without a strict dependency between the two). This means that users of the distribution will not need to explicitly direct Node.js to locate the ICU data. It also means that in environments where full internationalization is not required, they do not need to carry the extra content (with the associated storage costs). Refs: #3460 Signed-off-by: Stephen Gallagher <[email protected]> PR-URL: #30825 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Merged
targos
pushed a commit
that referenced
this pull request
Jan 14, 2020
When compiled with `--with-intl=small` and `--with-icu-default-data-dir=PATH`, Node.js will use PATH as a fallback location for the ICU data. We will first perform an access check using fopen(PATH, 'r') to ensure that the file is readable. If it is, we'll set the icu_data_directory and proceed. There's a slight overhead for the fopen() check, but it should be barely measurable. This will be useful for Linux distribution packagers who want to be able to ship a minimal node binary in a container image but also be able to add on the full i18n support where needed. With this patch, it becomes possible to ship the interpreter as /usr/bin/node in one package for the distribution and to ship the data files in another package (without a strict dependency between the two). This means that users of the distribution will not need to explicitly direct Node.js to locate the ICU data. It also means that in environments where full internationalization is not required, they do not need to carry the extra content (with the associated storage costs). Refs: #3460 Signed-off-by: Stephen Gallagher <[email protected]> PR-URL: #30825 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]>
BethGriggs
pushed a commit
that referenced
this pull request
Feb 6, 2020
When compiled with `--with-intl=small` and `--with-icu-default-data-dir=PATH`, Node.js will use PATH as a fallback location for the ICU data. We will first perform an access check using fopen(PATH, 'r') to ensure that the file is readable. If it is, we'll set the icu_data_directory and proceed. There's a slight overhead for the fopen() check, but it should be barely measurable. This will be useful for Linux distribution packagers who want to be able to ship a minimal node binary in a container image but also be able to add on the full i18n support where needed. With this patch, it becomes possible to ship the interpreter as /usr/bin/node in one package for the distribution and to ship the data files in another package (without a strict dependency between the two). This means that users of the distribution will not need to explicitly direct Node.js to locate the ICU data. It also means that in environments where full internationalization is not required, they do not need to carry the extra content (with the associated storage costs). Refs: #3460 Signed-off-by: Stephen Gallagher <[email protected]> PR-URL: #30825 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When compiled with
--with-intl=smalland--with-icu-default-data-dir=PATH, Node.js will use PATH as afallback location for the ICU data.
We will first perform an access check using fopen(PATH, 'r') to
ensure that the file is readable. If it is, we'll set the
icu_data_directory and proceed. There's a slight overhead for the
fopen() check, but it should be barely measurable.
This will be useful for Linux distribution packagers who want to
be able to ship a minimal node binary in a container image but
also be able to add on the full i18n support where needed. With
this patch, it becomes possible to ship the interpreter as
/usr/bin/node in one package for the distribution and to ship the
data files in another package (without a strict dependency
between the two). This means that users of the distribution will
not need to explicitly direct Node.js to locate the ICU data. It
also means that in environments where full internationalization is
not required, they do not need to carry the extra content (with
the associated storage costs).
Related: #3460
Signed-off-by: Stephen Gallagher [email protected]
Attn: @srl295
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes