icu-generic.gyp refactor - merge similar actions #12218

refack · 2017-04-04T19:22:25Z

Spin off of #11217

After we exorcise the voodoo, it's time to merge some very similar duplicated code
(After @bnoordhuis's observation, the trim and code-gen actions were parameterized and merged)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows [commit guidelines][]

Affected core subsystem(s)

build, intl

refack · 2017-04-06T03:10:57Z

@nodejs/collaborators whomever can CI this, please. I think is finally ready.

Trott · 2017-04-06T03:45:29Z

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

refack · 2017-04-07T04:19:45Z

Refactored parameter selection completely
Ready for another CI...

Trott · 2017-04-07T04:51:06Z

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

bnoordhuis

This looks much more complicated than I would have thought necessary. Why is that?

bnoordhuis · 2017-04-07T08:05:01Z

configure

@@ -1033,6 +1033,7 @@ def glob_to_var(dir_base, dir_sub, patch_dir):
  return list

 def configure_intl(o):
+  config_vars = o['variables']


This introduces a lot of churn for no good reason. Please don't do that, no reviewer likes a noisy diff.

~~😛😛😛😛😛😛😛😛😛😛😛😛~~

Sorry about that, it was the "old me", that tried to be cheeky.
Anyway I reverted, as requested.

refack · 2017-04-07T10:08:49Z

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

YAAAAAASSSSSSSS

refack · 2017-04-07T10:19:59Z

configure

  # this is the input '.dat' file to use .. icudt*.dat
  # may be little-endian if from a icu-project.org tarball
-  o['variables']['icu_data_in'] = icu_data_in
-  # this is the icudt*.dat file which node will be using (platform endianness)
-  o['variables']['icu_data_file'] = icu_data_file


'icu_data_file' is not used anywhere anymore so I ☠️ed it

Well it's back

refack · 2017-04-07T10:28:39Z

@bnoordhuis reduced churn (although I personally like "churn" if it's towards a cleaner code)

This looks much more complicated than I would have thought necessary. Why is that?

I took me too long because I assumed the 4 paths (win +/- small +/-) had more in common then what turned out. I now assume they just diverged over the years. For instance we need to repack for endiness only on non-windows. and genccode can only output .c files on non-windows, while it can directly compile to an .o file on windows.
IMHO the simplest/robust way was to parameterize the tasks, and set the parameters in a 4 clause condition. That way I was able to make the output to be as close as I could to the "baseline"

refack · 2017-04-07T10:44:01Z

Also

'action': [ 'python', 'icutrim.py', '-P', '<(PRODUCT_DIR)/.', # '.' suffix is a workaround against GYP assumptions :(
                '-D', '<(icu_data_in)', '-T', '<(icudata_trim_temp)',
                '--delete-tmp', ...

took some time to figure out since

I had to fight GYP not to make a mess of arguments that end with \
Then I realized --delete-tmp deletes the whole content of the directory -T points to before it operates, so we need a special <(icudata_trim_temp) directory and then copy the files back to where other actions can see them.
Also that the .gyp file is in tools/icu while the code is in deps/icu-small makes the whole thing cumbersome for a human to reason about.

~~P.S. I can put some of these comment it the code.~~ Have put them in...

gibfahn · 2017-04-07T10:55:43Z

(although I personally like "churn" if it's towards a cleaner code)

Me too. If you're doing that you can do it as a separate commit (called squash! refactor code for readability or something), then reviewers can look at the other changes separately, and it can be squashed before merging.

refack · 2017-04-07T11:01:46Z

(called squash! refactor code for readability or something)

Good tip. Next time.

srl295 · 2017-04-08T18:25:11Z

Nononono. Icu_data_file is used client side!! El abr. 7, 2017 4:02 AM -0700, Refael Ackermann <notifications@github.com>, escribió:

…

> > (called squash! refactor code for readability or something) > Good tip. Next time. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub (#12218 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AA0Ms0Qfn7f7LiepCMZx7GIiofq_Sbflks5rthe7gaJpZM4MzT6O).

refack · 2017-04-08T18:26:31Z

Nononono. Icu_data_file is used client side!!

who is the client?

refack · 2017-04-08T18:33:17Z

Nononono. Icu_data_file is used client side!!

@srl295 like native addons?
If it's used anywhere I'll use it in icu-generic.gyp to actually set the name of the output 🤔...

srl295 · 2017-04-08T18:45:17Z

Add on packages.. use it to find the file. Please leave the variables in the hash.. sorry it's complex we can hash it out if you want. Thanks for trying to simplify El abr. 8, 2017 11:33 AM -0700, Refael Ackermann <notifications@github.com>, escribió:

…

> > Nononono. Icu_data_file is used client side!! > @srl295 (https://github.com/srl295) like native addons? If it's used anywhere I'll use it in icu-generic.gyp to actually set the name of the output 🤔... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#12218 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AA0Ms2LBgfWXbWGCECr61gF7cesWfVPVks5rt9MNgaJpZM4MzT6O).

refack · 2017-04-08T18:46:44Z

Add on packages.. use it to find the file. Please leave the variables in the hash.. sorry it's complex we can hash it out if you want. Thanks for trying to simplify

Got you! so I'll use it in icu-generic.gyp to actually set the name of the output 👍

srl295 · 2017-04-09T00:00:45Z

Please accept my +1 for the sentiment. Real review when I get a chance. I'd best explain it all over $Beverage .. El abr. 8, 2017 3:46 PM -0700, Refael Ackermann <notifications@github.com>, escribió:

…

@refack commented on this pull request. In configure (#12218 (comment)): > # this is the input '.dat' file to use .. icudt*.dat # may be little-endian if from a icu-project.org tarball - o['variables']['icu_data_in'] = icu_data_in - # this is the icudt*.dat file which node will be using (platform endianness) - o['variables']['icu_data_file'] = icu_data_file Well it's back — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#12218 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AA0Ms55NAcWqykKPSCemEoX-7qHKXHd4ks5ruA5AgaJpZM4MzT6O).

refack · 2017-04-09T00:04:10Z

@srl295 tell me more about it, I don't get it. That file is not vendored by node-gyp for example...

also tweak `configure` around icu config

bnoordhuis · 2017-05-15T11:55:31Z

@refack Are you still working on this?

refack · 2017-05-15T13:46:25Z

@bnoordhuis It's stable.
I tried to address your comment with some comments in the code.
IMHO this can land.
Next step is to do what you did in #12231 and move all the logic to a .py file.

refack · 2017-05-15T13:51:15Z

New CI: https://ci.nodejs.org/job/node-test-commit/9893/

P.S. since it's just a .gyp instead of trying to review the code, I can provide the before and after output variations (±windows × ±icu-small × make/msvs/ninja) for comparison ( react with 👍 If you want it )

refack · 2017-05-30T13:03:55Z

Cross-Ref: #7254

refack · 2017-06-13T21:15:37Z

@srl295 I see you're around.
Does it make sense to preprocess icudata and commit the processed files (trimmed/full * LE/BE)?

BridgeAR · 2017-08-26T12:17:31Z

Ping

refack · 2017-08-30T21:28:26Z

Closing in favor of a better idea.

nodejs-github-bot added build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Apr 4, 2017

refack force-pushed the icudata-refactor branch from 9f0291f to 1e33ea1 Compare April 4, 2017 21:34

gibfahn mentioned this pull request Apr 5, 2017

build: refactor icu-generic.gyp - kill path voodoo #11217

Closed

2 tasks

refack force-pushed the icudata-refactor branch 3 times, most recently from 53ff842 to e1e34c0 Compare April 6, 2017 03:09

refack changed the title ~~icu-generic.gyp refactor~~ icu-generic.gyp refactor - merge similar actions Apr 6, 2017

refack force-pushed the icudata-refactor branch 3 times, most recently from 0c5478e to 2670dff Compare April 7, 2017 04:13

bnoordhuis requested changes Apr 7, 2017

View reviewed changes

refack force-pushed the icudata-refactor branch from 2670dff to 029a116 Compare April 7, 2017 10:15

refack commented Apr 7, 2017

View reviewed changes

refack force-pushed the icudata-refactor branch from 029a116 to c50ae16 Compare April 7, 2017 11:00

refack force-pushed the icudata-refactor branch from c50ae16 to ddc7ecf Compare April 8, 2017 22:44

refack mentioned this pull request Apr 10, 2017

The future of GYP in Node.js nodejs/CTC#2

Closed

refack force-pushed the icudata-refactor branch from ddc7ecf to 0c5478e Compare April 14, 2017 03:55

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

refack force-pushed the icudata-refactor branch from 0c5478e to d3f2700 Compare April 15, 2017 13:05

build: refactor icu-generic.gyp - merge similar actions

9dbb03c

also tweak `configure` around icu config

refack force-pushed the icudata-refactor branch from d3f2700 to 9dbb03c Compare April 25, 2017 12:07

refack mentioned this pull request May 30, 2017

Speedup canned small-icu build #7254

Closed

6 tasks

refack mentioned this pull request May 31, 2017

Preprocessing step for ICU 59.1 on z (was: “some platforms”) #13304

Closed

refack mentioned this pull request Jul 19, 2017

Cannot build on windows #14337

Closed

refack closed this Aug 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

icu-generic.gyp refactor - merge similar actions #12218

icu-generic.gyp refactor - merge similar actions #12218

refack commented Apr 4, 2017

refack commented Apr 6, 2017

Trott commented Apr 6, 2017

refack commented Apr 7, 2017 •

edited

Loading

Trott commented Apr 7, 2017

bnoordhuis left a comment

bnoordhuis Apr 7, 2017

refack Apr 7, 2017 •

edited

Loading

refack May 15, 2017

refack commented Apr 7, 2017

refack Apr 7, 2017

refack Apr 8, 2017

refack commented Apr 7, 2017 •

edited

Loading

refack commented Apr 7, 2017 •

edited

Loading

gibfahn commented Apr 7, 2017

refack commented Apr 7, 2017

srl295 commented Apr 8, 2017 via email

refack commented Apr 8, 2017

refack commented Apr 8, 2017

srl295 commented Apr 8, 2017 via email

refack commented Apr 8, 2017

srl295 commented Apr 9, 2017 via email

refack commented Apr 9, 2017

bnoordhuis commented May 15, 2017

refack commented May 15, 2017

refack commented May 15, 2017

refack commented May 30, 2017

refack commented Jun 13, 2017

BridgeAR commented Aug 26, 2017

refack commented Aug 30, 2017

icu-generic.gyp refactor - merge similar actions #12218

icu-generic.gyp refactor - merge similar actions #12218

Conversation

refack commented Apr 4, 2017

Checklist

Affected core subsystem(s)

refack commented Apr 6, 2017

Trott commented Apr 6, 2017

refack commented Apr 7, 2017 • edited Loading

Trott commented Apr 7, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Apr 7, 2017

Choose a reason for hiding this comment

refack Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

refack May 15, 2017

Choose a reason for hiding this comment

refack commented Apr 7, 2017

refack Apr 7, 2017

Choose a reason for hiding this comment

refack Apr 8, 2017

Choose a reason for hiding this comment

refack commented Apr 7, 2017 • edited Loading

refack commented Apr 7, 2017 • edited Loading

gibfahn commented Apr 7, 2017

refack commented Apr 7, 2017

srl295 commented Apr 8, 2017 via email

refack commented Apr 8, 2017

refack commented Apr 8, 2017

srl295 commented Apr 8, 2017 via email

refack commented Apr 8, 2017

srl295 commented Apr 9, 2017 via email

refack commented Apr 9, 2017

bnoordhuis commented May 15, 2017

refack commented May 15, 2017

refack commented May 15, 2017

refack commented May 30, 2017

refack commented Jun 13, 2017

BridgeAR commented Aug 26, 2017

refack commented Aug 30, 2017

refack commented Apr 7, 2017 •

edited

Loading

refack Apr 7, 2017 •

edited

Loading

refack commented Apr 7, 2017 •

edited

Loading

refack commented Apr 7, 2017 •

edited

Loading