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.

Sign up for GitHub

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

Jump to bottom

build: fix icu support when target arch != host #12

Closed
wants to merge 2 commits into from
Closed

build: fix icu support when target arch != host #12

wants to merge 2 commits into from

Conversation

misterdjules
Copy link

Without this change, I get the following error when running configure with --dest-cpu=ia32 on a x64 system:

➜  node git:(srl-v0.12-autoicu) ✗ ./configure --dest-cpu=ia32 --with-intl=small-icu
creating  ./icu_config.gypi
* Using ICU in ./deps/icu
creating  ./icu_config.gypi
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': ['OPENSSL_NO_SSL2=1'],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'clang': 1,
                 'host_arch': 'x64',
                 'icu_data_file': 'icudt54l.dat',
                 'icu_data_in': '../../deps/icu/source/data/in/icudt54l.dat',
                 'icu_endianness': 'l',
                 'icu_gyp_path': 'tools/icu/icu-generic.gyp',
                 'icu_path': './deps/icu',
                 'icu_small': 'true',
                 'icu_ver_major': '54',
                 'node_install_npm': 'true',
                 'node_prefix': '',
                 'node_shared_cares': 'false',
                 'node_shared_http_parser': 'false',
                 'node_shared_libuv': 'false',
                 'node_shared_openssl': 'false',
                 'node_shared_v8': 'false',
                 'node_shared_zlib': 'false',
                 'node_tag': '',
                 'node_use_dtrace': 'true',
                 'node_use_etw': 'false',
                 'node_use_mdb': 'false',
                 'node_use_openssl': 'true',
                 'node_use_perfctr': 'false',
                 'openssl_no_asm': 0,
                 'python': '/usr/bin/python',
                 'target_arch': 'ia32',
                 'uv_library': 'static_library',
                 'uv_parent_path': '/deps/uv/',
                 'uv_use_dtrace': 'true',
                 'v8_enable_gdbjit': 0,
                 'v8_enable_i18n_support': 1,
                 'v8_no_strict_aliasing': 1,
                 'v8_optimized_debug': 0,
                 'v8_random_seed': 0,
                 'v8_use_snapshot': 'true',
                 'want_separate_host_toolset': 1}}
creating  ./config.gypi
creating  ./config.mk
gyp: Dependency '/Users/JulienGilli/dev/node/tools/icu/icu-generic.gyp:icuuc#target' not found while trying to load target /Users/JulienGilli/dev/node/deps/v8/tools/gyp/v8.gyp:v8_base#target
Error running GYP
➜  node git:(srl-v0.12-autoicu) ✗

The change in this PR fixes it, but I'm not sure it's the proper way to do it.

srl295 and others added 2 commits December 8, 2014 16:15
@srl295
build: i18n: Autodownload ICU, Add a test.
dc2100e 
This is to implement
nodejs#7676 (comment)

* make `--with-intl=none` the default
 * Download, verify (md5), unpack ICU's zip if not there
 * update docs
* add a test

There's a "list" of URLs being used, but right now only the
first is picked up. The logic works something like this:

* if there is no directory `deps/icu`,
 * if no zip file (currently `icu4c-54_1-src.zip`),
  * download zip file (icu-project.org -> sf.net)
 * verify the MD5 sum of the zipfile
  * if bad, print error and exit
 * unpack the zipfile into `deps/icu`
* if `deps/icu` now exists, use it, else fail with help text

Also:
* refactor some code into tools/configure.d/nodedownload.py
* add `intl-none` option for `vcbuild.bat`

To rebuild `deps/icu-small` - (not currently checked in)
```
bash tools/icu/prepare-icu-source.sh
```

Also:
Reduce space by about 1MB with ICU 54 (over without this patch).
Also trims a few other source files, but only conditional on the exact ICU
version used. This is to future-proof - a file that is unneeded now may
be needed in future ICUs.
build: fix icu support when target arch != host
932708d
@misterdjules
Copy link
Author

I haven't tested cross compiling for ia32 on a x64 host on Windows, and I haven't tested cross-compiling for x64 on a ia32 host on any system.

The only cross-compilation configurations I have tested so far are target == ia32 && host == x64 on MacOS X and Linux.

I'll keep you posted when I have some time to test more configurations.

@misterdjules misterdjules mentioned this pull request Dec 9, 2014
Closed
5 tasks
@srl295
Copy link
Owner

srl295 commented Dec 9, 2014

@misterdjules first, thanks for the report and creating the PR. I don't think it can fix exactly that way, but I will take a look.

@srl295 srl295 self-assigned this Dec 9, 2014
@srl295 srl295 added this to the v0.12 milestone Dec 9, 2014
@srl295 srl295 added the bug label Dec 9, 2014
srl295 referenced this pull request Dec 9, 2014
@srl295
build: i18n: WIP on srl295/node#12
4da9c3d 
(work in progress - attempt to add dummy `#host` toolset
remapping to `icutools`)
@srl295
Copy link
Owner

srl295 commented Dec 9, 2014

@misterdjules I really don't understand this or why your patch 'fixes' it. icu-generic.gyp:icuuc#target' not found is the error, but you fixed it by adding #host. @bnoordhuis any ideas?

@srl295
Copy link
Owner

srl295 commented Dec 9, 2014

Also I note that --dest-cpu=ia32 also builds the host tools with i386.

srl295 referenced this pull request Dec 9, 2014
@srl295
build: i18n: WIP on srl295/node#12
aab55c8 
(work in progress - attempt to add dummy `#host` toolset
remapping to `icutools`)
@misterdjules
Copy link
Author

@srl295 Indeed, the error message doesn't make sense to me too, and if I rerun the configure command within your branch, I get the error message that I would expect:

  node git:(dc2100e) ✗ ./configure --dest-cpu=ia32 --with-intl=small-icu    
creating  ./icu_config.gypi
* Using ICU in ./deps/icu
creating  ./icu_config.gypi
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': ['OPENSSL_NO_SSL2=1'],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'clang': 1,
                 'host_arch': 'x64',
                 'icu_data_file': 'icudt54l.dat',
                 'icu_data_in': '../../deps/icu/source/data/in/icudt54l.dat',
                 'icu_endianness': 'l',
                 'icu_gyp_path': 'tools/icu/icu-generic.gyp',
                 'icu_path': './deps/icu',
                 'icu_small': 'true',
                 'icu_ver_major': '54',
                 'node_install_npm': 'true',
                 'node_prefix': '',
                 'node_shared_cares': 'false',
                 'node_shared_http_parser': 'false',
                 'node_shared_libuv': 'false',
                 'node_shared_openssl': 'false',
                 'node_shared_v8': 'false',
                 'node_shared_zlib': 'false',
                 'node_tag': '',
                 'node_use_dtrace': 'true',
                 'node_use_etw': 'false',
                 'node_use_mdb': 'false',
                 'node_use_openssl': 'true',
                 'node_use_perfctr': 'false',
                 'openssl_no_asm': 0,
                 'python': '/usr/bin/python',
                 'target_arch': 'ia32',
                 'uv_library': 'static_library',
                 'uv_parent_path': '/deps/uv/',
                 'uv_use_dtrace': 'true',
                 'v8_enable_gdbjit': 0,
                 'v8_enable_i18n_support': 1,
                 'v8_no_strict_aliasing': 1,
                 'v8_optimized_debug': 0,
                 'v8_random_seed': 0,
                 'v8_use_snapshot': 'true',
                 'want_separate_host_toolset': 1}}
creating  ./config.gypi
creating  ./config.mk
gyp: Dependency '/Users/JulienGilli/dev/node/tools/icu/icu-generic.gyp:icui18n#host' not found while trying to load target /Users/JulienGilli/dev/node/deps/v8/tools/gyp/v8.gyp:v8_base#host
Error running GYP
➜  node git:(dc2100e) ✗

I don't know why I got the error message that mentions #target in the first place. It is possible that I had some local modifications when I copy/pasted the error message, or that I copy/pasted the wrong error message.

Did you run this command on your machine? If so, what error message do you get?

@srl295
Copy link
Owner

srl295 commented Dec 9, 2014

@misterdjules yes I get the same message. I set up a new branch https://github.com/srl295/node/tree/srl-v0.12-FixToolsetAgain for just this, and reverted my srl-v0.12-autoicu branch to not have any fixes for this issue #12

@srl295
Copy link
Owner

srl295 commented Dec 10, 2014

also see nodejs#7676 (comment) for the previous toolset issues. edit @misterdjules your PR unfortunately brings back these errors:

tools/icu/icudata.target.mk:26: warning: overriding commands for target `/Users/srl/src/node/out/Release/obj/gen/icudt54l.dat'
tools/icu/icudata.host.mk:26: warning: ignoring old commands for target `/Users/srl/src/node/out/Release/obj/gen/icudt54l.dat'
tools/icu/icudata.target.mk:52: warning: overriding commands for target `/Users/srl/src/node/out/Release/obj/gen/icudt54.dat'
tools/icu/icudata.host.mk:52: warning: ignoring old commands for target `/Users/srl/src/node/out/Release/obj/gen/icudt54.dat'
tools/icu/icudata.target.mk:78: warning: overriding commands for target `/Users/srl/src/node/out/Release/obj/gen/icudt54_dat.c'
tools/icu/icudata.host.mk:78: warning: ignoring old commands for target `/Users/srl/src/node/out/Release/obj/gen/icudt54_dat.c'

@srl295 srl295 mentioned this pull request Dec 10, 2014
Closed
@misterdjules misterdjules mentioned this pull request Dec 12, 2014
Closed
srl295 referenced this pull request Dec 13, 2014
@srl295
build: i18n: WIP on srl295/node#12
f5a59b1 
(work in progress - attempt to add dummy `#host` toolset
remapping to `icutools`)
srl295 referenced this pull request Dec 13, 2014
@srl295
build: i18n: WIP on srl295/node#12
72d8358 
(work in progress - attempt to add dummy `#host` toolset
remapping to `icutools`)
@srl295 srl295 closed this in b048092 Dec 13, 2014
@srl295 srl295 mentioned this pull request Jan 5, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@srl295 srl295

Labels
bug
Projects
None yet
Milestone
v0.12
Development

Successfully merging this pull request may close these issues.
2 participants
@misterdjules @srl295