Skip to content

Build with icu#81

Merged
akoeplinger merged 4 commits intodotnet:dotnet/mainfrom
steveisok:use-icu
Mar 24, 2024
Merged

Build with icu#81
akoeplinger merged 4 commits intodotnet:dotnet/mainfrom
steveisok:use-icu

Conversation

@steveisok
Copy link
Copy Markdown
Member

Repos who consume the node transport package seem to need icu support, so enable.

Repos who consume the node transport package seem to need icu support, so enable.
@steveisok steveisok requested review from akoeplinger and lewing March 21, 2024 22:45
@akoeplinger
Copy link
Copy Markdown
Member

akoeplinger commented Mar 22, 2024

did you check the size increase? there is also a small-icu option that might be enough for us since it supports the RegExp: https://nodejs.org/api/intl.html#options-for-building-nodejs

@steveisok steveisok changed the title Build with full icu Build with icu Mar 22, 2024
@akoeplinger
Copy link
Copy Markdown
Member

Looks like the win arm64 build is failing:

icutrim
'....\artifacts\obj\node\Release\icupkg' is not recognized as an internal or external command,
operable program or batch file.
Options: {'toolpath': '..\..\artifacts\obj\node\Release\', 'datfile': '../../deps/icu-tmp/icudt73l.dat', 'filterfile': 'icu_small.json', 'tmpdir': '..\..\artifacts\obj\node\Release\obj/global_intermediate/icutmp', 'deltmpdir': 1, 'outfile': 'icudt73l.dat', 'verbose': 1, 'locales': 'en,root', 'endian': 'little'}
icu_small.json: icutrim.py config: Trim down ICU to just a certain locale set, needed for node.js use.
FAILED: ....\artifacts\obj\node\Release\icupkg -tl ../../deps/icu-tmp/icudt73l.dat ....\artifacts\obj\node\Release\obj/global_intermediate/icutmp\icudt73l.dat
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(254,5): error MSB8066: Custom build for '....\deps\icu-tmp\icudt73l.dat;....\artifacts\obj\node\Release\obj\global_intermediate\icutmp\icudt73l.dat' exited with code 1. [D:\a_work\1\s\tools\icu\icudata.vcxproj]

Maybe we're missing python?

@lewing
Copy link
Copy Markdown
Member

lewing commented Mar 23, 2024

@ilonatommy is icupkg part of icu or something specific to node?

cc @radekdoulik

@akoeplinger
Copy link
Copy Markdown
Member

@akoeplinger
Copy link
Copy Markdown
Member

akoeplinger commented Mar 23, 2024

I think I know what's going on:

The x64 build has this:

     icupkg.vcxproj -> ..\..\artifacts\obj\node\Release\\icupkg.exe

while the arm64 one has this:

   icupkg_host.vcxproj -> ..\..\artifacts\obj\node\Release\\icupkg_host.exe

so basically it's building for the (x64) host which makes sense, so we likely just need a tweak in the script that's calling the tool.

@akoeplinger
Copy link
Copy Markdown
Member

I pushed a workaround, turns out gyp does some magic with the paths so <(PRODUCT_DIR)/icupkg<(EXECUTABLE_SUFFIX) is either icupkg.exe or icupkg_host.exe, we can just pass that to the script.

@akoeplinger akoeplinger merged commit 93f3f41 into dotnet:dotnet/main Mar 24, 2024
@steveisok steveisok deleted the use-icu branch March 24, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants