cmake + icu #2
Open
cmake + icu #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bnoordhuis
commented
Feb 6, 2018
| ${CMAKE_SOURCE_DIR}/tools/icu/icutrim.py | ||
| ${icu_data_in} | ||
| OUTPUT | ||
| ${icu_data_out}) |
There was a problem hiding this comment.
This is the build step that's giving me trouble.
|
OK first thing, for cmake, you might want to see http://bugs.icu-project.org/trac/ticket/7747 (this is on the source side, not data) Specifically, gencmake.py generates IcuPaths.cmake from ICU's dependencies.txt (which has been included in ICU's source for a number of years). Then you can depend on a group such as |
|
It might be that iculslocs is fine, but your ICU build is broken. Opening |
Okay, but what confounds me is that the gyp build doesn't even try to open icudt60l-brkitr, it goes straight for icudt60l (the one in out/Release, not the one in deps/icu-small.) Conversely, the cmake iculslocs dies without even trying to open icudt60l. It looks for Does that reproduce on your end?
As long as it gives node enough control over the build I'm good with using the upstream solution. That hasn't been released though, or has it?
True, but that doesn't worry me unduly, it's not much work. |
PR-URL: nodejs#18605 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
The complicated `awaitDrain` machinery can be made a bit
slimmer, and more correct, by just resetting the value
each time `stream.emit('data')` is called.
By resetting the value before emitting the data chunk, and
seeing whether any pipe destinations return `.write() === false`,
we always end up in a consistent state and don’t need to worry
about odd situations (like `dest.write(chunk)` emitting more data).
PR-URL: nodejs#18516
Fixes: nodejs#18484
Fixes: nodejs#18512
Refs: nodejs#18515
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18516 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18601 Refs: nodejs#18407 Refs: nodejs#18444 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
I'm trying your branch out now.
icu/source/test/depstest/dependencies.txt has been released and gets tested and updates. |
PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
We cannot make uvException a proper class due to compatibility reasons for now, so there is no need to call new since it only returns a newly-created Error. PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#16648 Refs: nodejs#16636 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
google.com added another TXT record which broke this test. This removes the check on the content of the TXT record since that depends on an external state subject to change. PR-URL: nodejs#18547 Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>
This test should exit naturally or will timeout on its own, a separate unrefed timer is not necessary. PR-URL: nodejs#18589 Fixes: nodejs#18587 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#18596 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Assuming less knowledge on the part of the reader, making it easier to get start using Node.js. PR-URL: nodejs#17977 Fixes: nodejs#17970, Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
PR-URL: nodejs#18498 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: nodejs#14674 PR-URL: nodejs#18563 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18578 Refs: v8/v8@6.4.388.41...6.4.388.42 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18562 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Instead of separately calling into C++ from JS to retrieve the Timer.now() value, pass it in as an argument. PR-URL: nodejs#18562 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
This test was added over six years ago, and the behavior seems to have changed since then. When the test was originally written, common.mustCall() did not exist. The only assertion in this test was not actually executing. Instead of an 'error' event being emitted as expected, an 'abort' event was emitted. PR-URL: nodejs#18508 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#18630 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
developer.mozilla.org/en/... -> developer.mozilla.org/en-US/... PR-URL: nodejs#18631 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#18270 Fixes: nodejs#8307 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
@srl295 Were you able to reproduce? |
This helps to show the cause of errors in the CI. PR-URL: nodejs#18507 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
- Throw getPathFromURL() and nullCheck() errors synchronously instead of deferring them to the next tick, since we already throw validatePath() errors synchronously. - Merge nullCheck() into validatePath() - Never throws in `fs.exists()`, instead, invoke the callback with false, or emit a warning when the callback is not a function. This is to bring it inline with fs.existsSync(), which never throws. - Updates the comment of rethrow() - Throw ERR_INVALID_ARG_VALUE for null checks PR-URL: nodejs#18308 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fix a regression introduced by nodejs#18515 that broke the dicer module tests. See: nodejs#18515 PR-URL: nodejs#18615 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
This brings the behaviour in line with the documentation. PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
1. Fixer for crypto-check.js 2. Extends tests PR-URL: nodejs#16647 Refs: nodejs#16636 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
1. Adds fixer method 2. Extends test PR-URL: nodejs#16646 Refs: nodejs#16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#18739 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
When the async uv_fs_* call errors out synchronously in AsyncDestCall, the after callbacks (e.g. AfterNoArgs) would delete the req_wrap in FSReqAfterScope, and AsyncDestCall would set those req_wrap to nullptr afterwards. But when it returns to the top-layer bindings, the bindings all call `req_wrap->SetReturnValue()` again without checking if `req_wrap` is nullptr, causing a segfault. This has not been caught in any of the tests because we usually do a lot of argument checking in the JS layer before invoking the uv_fs_* functions, so it's rare to get a synchronous error from them. Currently we never need the binding to return the wrap to JS layer, so we can just call `req_wrap->SetReturnValue()` to return undefined for normal FSReqWrap and the promise for FSReqPromise in AsyncDestCall instead of doing this in the top-level bindings. PR-URL: nodejs#18811 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Currenlty when configuring --debug-http2 /test/parallel/test-http2-getpackedsettings.js will segment fault: $ out/Debug/node test/parallel/test-http2-getpackedsettings.js Segmentation fault: 11 This is happening because the settings is created with the Environment in PackSettings: Http2Session::Http2Settings settings(env); This will cause the session to be set to nullptr. When the init function is later called the expanded DEBUG_HTTP2SESSION macro will cause the segment fault when the session is dereferenced. PR-URL: nodejs#18815 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#18780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Currently the cctest target depend on the node_core_target_name target. But it is the node_lib_target_name target that compiles the sources now which means that if a source file in src is updated the cctest executable will not be re-linked against it, but will remain unchanged. The code will still be compiled, just not linked which means that if you are debugging you'll not see the changes and also a warning will be displayed about this issue. This commit changes the cctest target to depend on node_lib_target_name. PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Yihong Wang <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
There are currently 3 places in Timers where the exact same code appears. Instead create a helper function that does the same job of setting asyncId & triggerAsyncId, as well as calling emitInit. PR-URL: nodejs#18825 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
When using a debug build (on Windows specifically) the error case for tls_wrap causes an assert to fire because the index being passed is outside the bounds of the vector. The fix is to switch to iterators. PR-URL: nodejs#18830 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#18872 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#18873 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Refs: nodejs#11135 PR-URL: nodejs#18843 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
PR-URL: nodejs#18845 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#18849 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#18850 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This utility is fairly generic and likely useful for more than one test. PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
TTY tests should almost never be placed in `/parallel/`. Skipping TTY tests there due to missing tty fds just means they will never be run, ever, on any system. This moves the tty-get-color-depth test to `/pseudo-tty/` where the test runner will actually make a pty fd. Refs: nodejs#17615 PR-URL: nodejs#18800 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#18566 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
* Sync format schemes with the current doc state. * Lowercase primitive types. * Fix typos and unify the style. * Remove tautological info. PR-URL: nodejs#18874 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Note that the CI run for the exercise can be minimal. PR-URL: nodejs#18846 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#18847 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
Don't print them to stdout because stdout gets redirected to file. Errors were effectively being swallowed since said file was deleted afterwards.
bnoordhuis
added a commit
that referenced
this pull request
Mar 27, 2018
Remove a pointless adapter frame by fixing up the function's formal
parameter count. Before:
frame #0: 0x000033257ea446d5 onParserExecute(...)
frame #1: 0x000033257ea3b93f <adaptor>
frame #2: 0x000033257ea41959 <internal>
frame #3: 0x000033257e9840ff <entry>
After:
frame #0: 0x00000956287446d5 onParserExecute(...)
frame #1: 0x0000095628741959 <internal>
frame #2: 0x00000956286840ff <entry>
PR-URL: nodejs#17693
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
bnoordhuis
added a commit
that referenced
this pull request
Mar 27, 2018
Remove a pointless adapter frame by fixing up the function's formal
parameter count. Before:
frame #0: 0x000033257ea446d5 onParserExecute(...)
frame #1: 0x000033257ea3b93f <adaptor>
frame #2: 0x000033257ea41959 <internal>
frame #3: 0x000033257e9840ff <entry>
After:
frame #0: 0x00000956287446d5 onParserExecute(...)
frame #1: 0x0000095628741959 <internal>
frame #2: 0x00000956286840ff <entry>
PR-URL: nodejs#17693
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@srl295 I'm porting node's build system to cmake and I'd like you to take a look at making icutrim work. I've spent about two days trying to understand what all the scripts do but I'm not any closer.
Build with:
The relevant build script is
cmake/icu/common/CMakeLists.txt;cmake/icu/tools/CMakeLists.txtmight be relevant too. Grep that first file for 'icu_small' to find the command.I'm passing the same arguments as the gyp build but for flabbergasting reasons, the following happens:
icudt60l-brkiterwithures_openDirect()I established 2 and 3 by adding copious printf statements and
strace -f. Yes, strace - I'm that desperate by now.I tried to follow the logic inside ICU but whatever the cause (locale?), it's elusive and eluding. Hope you can help.
cc @mhdawson