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

installer: install ICU header files #12217

Closed
wants to merge 3 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 4, 2017

Also add a smoke test to ensure that linking to the bundled ICU works.

(Perhaps this should be two commits.)

CI: https://ci.nodejs.org/job/node-test-pull-request/7205/

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 4, 2017
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. With system the system provided headers.

tools/install.py Outdated
icu_path = variables.get('icu_path')
if icu_path:
icu_headers_path = icu_path + '/source/common/unicode'
subdir_files(icu_headers_path, 'include/node/unicode/', action)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also install i18n/unicode and (possibly?) io/unicode.

@srl295 What does ICU consider public API and what internal?

@bnoordhuis
Copy link
Member Author

Windows link errors...

binding.obj : error LNK2001: unresolved external symbol ulocdata_getCLDRVersion_58 [c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win2012r2\test\addons\icu-binding\build\binding.vcxproj]
binding.obj : error LNK2001: unresolved external symbol u_versionToString_58 [c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win2012r2\test\addons\icu-binding\build\binding.vcxproj]

@srl295 Is that an ICU bug? ICU seems to be quite diligent in tagging everything dllexport but it doesn't do it for functions mangled with U_ICU_ENTRY_POINT_RENAME().

@mscdex mscdex added the i18n-api Issues and PRs related to the i18n implementation. label Apr 4, 2017
@bnoordhuis
Copy link
Member Author

Switched to TimeZone::getTZDataVersion() (which is marked dllexport) and added i18n/unicode to the list of directories to install. CI: https://ci.nodejs.org/job/node-test-pull-request/7223/

@srl295 Can you PTAL?

@bnoordhuis
Copy link
Member Author

God, Windows... I'm going to guess i18n only uses dllexport when the right U_*_IMPLEMENTATION define is set at node build time, otherwise this shouldn't happen:

binding.obj : error LNK2001: unresolved external symbol "__declspec(dllimport) public: static char const * __cdecl icu_58::TimeZone::getTZDataVersion(enum UErrorCode &)" 

Will dig further.

@srl295
Copy link
Member

srl295 commented Apr 5, 2017

@bnoordhuis yes, U_*_IMPLEMENTATION needs to be set. let me find the link…

OK. for a shlib you need to set U_*_IMPLEMENTATION as in https://github.com/nodejs/node/blob/master/tools/icu/icu-generic.gyp#L496

Within node, since it's static, I would set U_STATIC_IMPLEMENTATION https://github.com/nodejs/node/blob/master/tools/icu/icu-generic.gyp#L46

OK, I'm confused…  Node is built statically, including ICU. How is the linking supposed to work?

I think in icu-generic.gyp you need to change in both places U_STATIC_IMPLEMENTATION (export nothing) to U_COMBINED_IMPLEMENTATION (export everything as one library)

@BridgeAR
Copy link
Member

Ping @bnoordhuis

@BridgeAR BridgeAR added the stalled Issues and PRs that are stalled. label Sep 8, 2017
@BridgeAR
Copy link
Member

@bnoordhuis does this still need much work? It is stalled since quite a while and I would otherwise close this.

@srl295
Copy link
Member

srl295 commented Sep 12, 2017

so, what I said in #12217 (comment)

I think in icu-generic.gyp you need to change in both places U_STATIC_IMPLEMENTATION (export nothing) to U_COMBINED_IMPLEMENTATION (export everything as one library)

if that is set, it might work on Windows.

@BridgeAR
Copy link
Member

Closing this due to the long inactivity and no response. @bnoordhuis please reopen if you would like to pursue this further.

@BridgeAR BridgeAR closed this Sep 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants