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: refactor icu-generic.gyp - kill path voodoo #11217

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Feb 7, 2017 •
edited
Loading

  1. Intention was to get PRODUCT_DIR so no need to do path voodoo
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build, intl

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Feb 7, 2017
@mhdawson
Copy link
Member

mhdawson commented Feb 7, 2017

@srl295 as change is in ICU gyp file

@bnoordhuis
Copy link
Member

I think you can take this one step further and merge them with the other icutrim and genccode targets, they look functionally identical now.

This was referenced Feb 20, 2017
Closed
Closed
Closed
indutny
indutny approved these changes Feb 23, 2017
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

Other than what @bnoordhuis said - LGTM.

srl295
srl295 approved these changes Feb 23, 2017
View reviewed changes
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.

LGTM thanks!

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Feb 23, 2017
@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

@refack Will you make the simplification that @bnoordhuis suggested or should we merge as is?

@refack
Copy link
Contributor Author

refack commented Mar 27, 2017

@fhinkel Give me a day to try and grok the rules.

@refack
Copy link
Contributor Author

refack commented Mar 27, 2017 •
edited
Loading

@fhinkel, I broke... 😞
I refactored the conditions a bit, but I can't find a way to merge them...
I did find that they were actually broken because of excessive escaping so I added 'msvs_quote_cmd': 0 NM

@refack refack force-pushed the patch-2 branch 3 times, most recently from a9b48ea to dbbe7da Compare March 28, 2017 03:38
@refack refack mentioned this pull request Mar 28, 2017
Closed
3 tasks
@refack
Copy link
Contributor Author

refack commented Mar 28, 2017

You are all evil people, leaving me alone with this gyp.
It just sitting there... 😝 Mocking me... 🤖 "I'm so full of duplication",😈 "I'm so fragile I will take down your entire build"... 🔨

But I beat it. Well, just a bit.
Have fun reviewing...

@refack refack force-pushed the patch-2 branch 9 times, most recently from d86a373 to b21a3e3 Compare March 30, 2017 13:01
@refack
Copy link
Contributor Author

refack commented Mar 30, 2017 •
edited
Loading

finally green on windows
Build status

@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

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

@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

Lots of red in CI on this one

@refack
Copy link
Contributor Author

refack commented Apr 4, 2017

I've seen... I hate GYP... so frustrating...
I'm spinning off second part of the PR. Since the first part got the LGTM's and should be fairly stable, and it will unblock @indutny 's gyp.js efforts.

Need a new CI job just for df15e3b0277

@refack refack changed the title build: refactor icu-generic.gyp - kill path voodoo & merge actions build: refactor icu-generic.gyp - kill path voodoo Apr 4, 2017
@gibfahn
Copy link
Member

gibfahn commented Apr 4, 2017 •
edited
Loading

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

If you keep commenting asking for CI, it's easy enough to run (I'd rather run the CI than have to deal with GYP 😨 )

@refack refack mentioned this pull request Apr 4, 2017
Closed
2 tasks
@refack
build: fix path voodoo in icu-generic.gyp
f3c7c16 
Intention was to get to `PRODUCT_DIR` so no need to do path voodoo
Also added `'msvs_quote_cmd': 0` and more precise quoting
@refack
Copy link
Contributor Author

refack commented Apr 5, 2017

It's green. Quick land this, before GYP changes it's mind.

@gibfahn
Copy link
Member

gibfahn commented Apr 5, 2017

@refack am I right in thinking that this change is what @srl295, @jasnell and @indutny approved, and that the changes you added later are now in #12218?

If so then as long as @bnoordhuis doesn't object we can get this landed.

@refack
Copy link
Contributor Author

refack commented Apr 5, 2017

@gibfahn yes. What's left is what was here 10 days ago. The hard changes went to #12218.
This one seems like a minor change but it blocks gyp.js indutny/gyp.js#34

@refack refack force-pushed the patch-2 branch 2 times, most recently from a502a99 to f3c7c16 Compare April 5, 2017 21:02
@gibfahn gibfahn self-assigned this Apr 5, 2017
@gibfahn
Copy link
Member

gibfahn commented Apr 5, 2017 •
edited
Loading

One more Ci for luck and I'll land:

CI: https://ci.nodejs.org/job/node-test-commit/8920/ EDIT: That failed on macOS (connection issue)

CI Reloaded: https://ci.nodejs.org/job/node-test-commit/8921/

CI: Best of Three: https://ci.nodejs.org/job/node-test-commit/8923/

@gibfahn
Copy link
Member

gibfahn commented Apr 6, 2017

Landed in e139dae

@gibfahn gibfahn closed this Apr 6, 2017
gibfahn pushed a commit that referenced this pull request Apr 6, 2017
@refack @gibfahn
build: fix path voodoo in icu-generic.gyp
e139dae 
Intention was to get to `PRODUCT_DIR` so no need to do path voodoo
Also added `'msvs_quote_cmd': 0` and more precise quoting

PR-URL: #11217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
@refack refack deleted the patch-2 branch April 6, 2017 16:54
@refack refack mentioned this pull request Apr 10, 2017
Closed
@refack
Copy link
Contributor Author

refack commented Apr 10, 2017

I was looking at this, reading the comments top to bottom, and was overjoyed again to see

Landed in e139dae

italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@refack @italoacasas
build: fix path voodoo in icu-generic.gyp
d4d6986 
Intention was to get to `PRODUCT_DIR` so no need to do path voodoo
Also added `'msvs_quote_cmd': 0` and more precise quoting

PR-URL: nodejs#11217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
@MylesBorins
Copy link
Contributor

This lands cleanly. Should it be cherry-picked to v6.x?

@refack
Copy link
Contributor Author

refack commented Apr 19, 2017

It's almost cosmetic, mainly to solve indutny/gyp.js#34.
If there's any risk, skip it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srl295 srl295 srl295 approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

Assignees

@gibfahn gibfahn

Labels
i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.