Skip to content
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

deps: document which ICU version we work with #19657

Closed
srl295 opened this issue Mar 28, 2018 · 13 comments
Closed

deps: document which ICU version we work with #19657

srl295 opened this issue Mar 28, 2018 · 13 comments
Assignees
Labels
doc Issues and PRs related to the documentations. i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented Mar 28, 2018

in nodejs/Intl#35 I wrote:

I think node will work with a pretty wide range of ICUs at this point. It might be worth actually testing this, and making sure configure complains if the ICU is too old.

Probably ICU4C 58.2 is the minimum for master ( ad72142 ish ) at this point. It would be 57 if #19656 were solved.

Note that the backlevel ICU versions are relevant to packagers trying to use the pre-installed ICU from the system or other packaging. For example, Ubuntu stretch (at least on raspbian) has ICU 57.1 installed.

must be 99 ways to overengineer this

  • Just document the ICU version
    • in configure with the relevant options?
    • in tools/icu/README.md (doesn't seem helpful)
    • in doc/api/intl.md (best option?)
    • in the wiki?
  • warn in configure if the version is too old
  • error in configure if the version is too old
    • … with an option to override?
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 28, 2018
@ryzokuken
Copy link
Contributor

@nodejs/documentation this has been open for quite a while. PTAL.

@ryzokuken
Copy link
Contributor

/cc @vsemozhetbyt

@ryzokuken ryzokuken added the doc Issues and PRs related to the documentations. label Jun 7, 2018
@vsemozhetbyt
Copy link
Contributor

Sorry, I do not have the required knowledge to handle this(

@ryzokuken
Copy link
Contributor

@vsemozhetbyt that's okay, I just wanted to ask if you think we should mention the version in the docs. I could help you with the actual change? Or I could make it myself, but would need the docs teams' help in order to keep it updated.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 8, 2018

I have no opinion on this.
Let's also cc @nodejs/intl

@ryzokuken
Copy link
Contributor

I'd rather cc @nodejs/i18n. intl has been archived in favor of i18n.

@ryzokuken
Copy link
Contributor

ryzokuken commented Jun 8, 2018

That said, they must've pinged intl because docs didn't exist, but this looks like 100% a docs thingie.

@ryzokuken ryzokuken removed the i18n-api Issues and PRs related to the i18n implementation. label Jun 8, 2018
@srl295
Copy link
Member Author

srl295 commented Jun 8, 2018

  • doc/api/intl.md should say 'ICU 57.1 is the current minimum version'
  • configure should probably warn (not fail) if the minimum seems to be too old.

actually even better would be that configure --help prints out the minimum recommended version ( as part of the intl options help text) and doc/api/intl.md tells you to look there.

@srl295
Copy link
Member Author

srl295 commented Jun 8, 2018

How does this look?

$ ./configure --help
…
    --with-icu-source=WITH_ICU_SOURCE
                        Intl mode: optional local path to icu/ dir, or
                        path/URL of the icu4c source archive. v57.x or later
                        recommended.
$ ./configure --with-icu-source=https://ssl.icu-project.org/files/icu4c/49.1.1/icu4c-49_1_1-src.tgz
creating icu_config.gypi
Deleting old ICU source: deps/icu
 <https://ssl.icu-project.org/files/icu4c/49.1.1/icu4c-49_1_1-src.tgz>
 Fetch: . 18.6MB total, 18.6MB downloaded
 Extracting tarfile: deps/icu-tmp/icu4c-49_1_1-src.tgz
* Using ICU in deps/icu
WARNING: icu4c v49.x may be too old, v57.x or later is recommended.
creating icu_config.gypi
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': [],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'asan': 0,
                 'build_v8_with_gn': 'false',
                 'coverage': 'false',
…
…
creating config.gypi
creating config.mk
WARNING: warnings were emitted in the configure phase

@srl295
Copy link
Member Author

srl295 commented Jun 8, 2018

☝️ how does srl295@d67d9d0 look?

@srl295 srl295 self-assigned this Jun 8, 2018
@srl295
Copy link
Member Author

srl295 commented Jun 8, 2018

@ryzokuken OK to readd the Intl tag?

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Sep 28, 2018
@jasnell
Copy link
Member

jasnell commented Oct 19, 2018

Ping @ryzokuken @srl295 ... any progress on this?

@srl295
Copy link
Member Author

srl295 commented Oct 19, 2018

@jasnell @ryzokuken @nodejs/intl any comment on

srl295@d67d9d0 - does it seem to be a good direction?

srl295 added a commit to srl295/node that referenced this issue Oct 23, 2018
Fixes: nodejs#19657

PR-URL: nodejs#23766
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
targos pushed a commit that referenced this issue Oct 24, 2018
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 26, 2018
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
rvagg pushed a commit that referenced this issue Nov 28, 2018
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
Fixes: #19657

PR-URL: #23766
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests

4 participants