From 8b45c5d26a829bcd3280401dbc1874bcd1302289 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Fri, 15 Sep 2023 14:55:17 +0200
Subject: [PATCH] src: improve error message when ICU data cannot be
 initialized

Previously when we fail to initialize ICU data, the error message is

```
could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir
parameters)
```

This patch updates it to something similar to:

```
U_FILE_ACCESS_ERROR: Could not initialize ICU. Check the directory
specified by NODE_ICU_DATA or --icu-data-dir contains icudt73l.dat
and it's readable
```

Where the expected data file name is the same as U_ICUDATA_NAME.

PR-URL: https://github.com/nodejs/node/pull/49666
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/node.cc                        | 11 ++++++++---
 src/node_i18n.cc                   | 23 ++++++++++++++---------
 src/node_i18n.h                    |  2 +-
 test/parallel/test-icu-data-dir.js | 30 ++++++++++++++++++------------
 4 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/src/node.cc b/src/node.cc
index 17f3829d743941..89e0e5524c2102 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -916,9 +916,14 @@ static ExitCode InitializeNodeWithArgsInternal(
 
     // Initialize ICU.
     // If icu_data_dir is empty here, it will load the 'minimal' data.
-    if (!i18n::InitializeICUDirectory(per_process::cli_options->icu_data_dir)) {
-      errors->push_back("could not initialize ICU "
-                        "(check NODE_ICU_DATA or --icu-data-dir parameters)\n");
+    std::string icu_error;
+    if (!i18n::InitializeICUDirectory(per_process::cli_options->icu_data_dir,
+                                      &icu_error)) {
+      errors->push_back(icu_error +
+                        ": Could not initialize ICU. "
+                        "Check the directory specified by NODE_ICU_DATA or "
+                        "--icu-data-dir contains " U_ICUDATA_NAME ".dat and "
+                        "it's readable\n");
       return ExitCode::kInvalidCommandLineArgument;
     }
     per_process::metadata.versions.InitializeIntlVersions();
diff --git a/src/node_i18n.cc b/src/node_i18n.cc
index 372df8d029fc4f..d45325954d9807 100644
--- a/src/node_i18n.cc
+++ b/src/node_i18n.cc
@@ -54,20 +54,21 @@
 #include "util-inl.h"
 #include "v8.h"
 
-#include <unicode/utypes.h>
 #include <unicode/putil.h>
+#include <unicode/timezone.h>
 #include <unicode/uchar.h>
 #include <unicode/uclean.h>
+#include <unicode/ucnv.h>
 #include <unicode/udata.h>
 #include <unicode/uidna.h>
-#include <unicode/ucnv.h>
-#include <unicode/utf8.h>
-#include <unicode/utf16.h>
-#include <unicode/timezone.h>
 #include <unicode/ulocdata.h>
+#include <unicode/urename.h>
+#include <unicode/ustring.h>
+#include <unicode/utf16.h>
+#include <unicode/utf8.h>
+#include <unicode/utypes.h>
 #include <unicode/uvernum.h>
 #include <unicode/uversion.h>
-#include <unicode/ustring.h>
 
 #ifdef NODE_HAVE_SMALL_ICU
 /* if this is defined, we have a 'secondary' entry point.
@@ -569,8 +570,7 @@ ConverterObject::ConverterObject(
   }
 }
 
-
-bool InitializeICUDirectory(const std::string& path) {
+bool InitializeICUDirectory(const std::string& path, std::string* error) {
   UErrorCode status = U_ZERO_ERROR;
   if (path.empty()) {
 #ifdef NODE_HAVE_SMALL_ICU
@@ -583,7 +583,12 @@ bool InitializeICUDirectory(const std::string& path) {
     u_setDataDirectory(path.c_str());
     u_init(&status);
   }
-  return status == U_ZERO_ERROR;
+  if (status == U_ZERO_ERROR) {
+    return true;
+  }
+
+  *error = u_errorName(status);
+  return false;
 }
 
 void SetDefaultTimeZone(const char* tzid) {
diff --git a/src/node_i18n.h b/src/node_i18n.h
index f32ade831b17a0..e516282865fb18 100644
--- a/src/node_i18n.h
+++ b/src/node_i18n.h
@@ -38,7 +38,7 @@
 namespace node {
 namespace i18n {
 
-bool InitializeICUDirectory(const std::string& path);
+bool InitializeICUDirectory(const std::string& path, std::string* error);
 
 void SetDefaultTimeZone(const char* tzid);
 
diff --git a/test/parallel/test-icu-data-dir.js b/test/parallel/test-icu-data-dir.js
index f1da0dc685ae9d..0a3166180be3f4 100644
--- a/test/parallel/test-icu-data-dir.js
+++ b/test/parallel/test-icu-data-dir.js
@@ -1,27 +1,33 @@
 // Flags: --expose-internals
 'use strict';
 const common = require('../common');
+const { spawnSyncAndExit } = require('../common/child_process');
 const { internalBinding } = require('internal/test/binding');
-const os = require('os');
 
 const { hasSmallICU } = internalBinding('config');
 if (!(common.hasIntl && hasSmallICU))
   common.skip('missing Intl');
 
-const assert = require('assert');
-const { spawnSync } = require('child_process');
-
-const expected =
-    'could not initialize ICU (check NODE_ICU_DATA or ' +
-    `--icu-data-dir parameters)${os.EOL}`;
-
 {
-  const child = spawnSync(process.execPath, ['--icu-data-dir=/', '-e', '0']);
-  assert(child.stderr.toString().includes(expected));
+  spawnSyncAndExit(
+    process.execPath,
+    ['--icu-data-dir=/', '-e', '0'],
+    {
+      status: 9,
+      signal: null,
+      stderr: /Could not initialize ICU/
+    });
 }
 
 {
   const env = { ...process.env, NODE_ICU_DATA: '/' };
-  const child = spawnSync(process.execPath, ['-e', '0'], { env });
-  assert(child.stderr.toString().includes(expected));
+  spawnSyncAndExit(
+    process.execPath,
+    ['-e', '0'],
+    { env },
+    {
+      status: 9,
+      signal: null,
+      stderr: /Could not initialize ICU/
+    });
 }