[v8.x backport] 16238 and 18576 by yhwang · Pull Request #20797 · nodejs/node

yhwang · 2018-05-17T05:51:06Z

Backport #16238 and #18576 to resolve issue #20763

fix: #20763

yhwang · 2018-05-17T05:54:40Z

Let me run a CI to verify it and wait for the review. I don't know if it's okay to backport 2 PRs as one PR. If not I will create separated PRs for both.

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

MylesBorins · 2018-05-17T06:32:55Z

@yhwang totally fine to backport multiple PRs

do you think this breakage should cause another patch release for next week or could it wait for the next scheduled relase eta 1 month

yhwang · 2018-05-17T14:35:29Z

@MylesBorins actually the --enable-static could be deprecated after 658dd409fd. The reason being is static library is built when doing ./configure. Therefore, there is no need to use --enable-static. I think it's okay for next scheduled release.

MylesBorins · 2018-05-22T15:50:03Z

After landing I'm getting some errors

gyp: Undefined variable OBJ_SUFFIX in /Users/mborins/code/node/v8.x/node.gyp while trying to load /Users/mborins/code/node/v8.x/node.gyp
Error running GYP

Could we get a rebase and manual test to ensure this works

yhwang · 2018-05-22T16:30:04Z

@MylesBorins I thought CI did the job you mentioned. I can do rebase and verify it on ubuntu and windows manually.

[update] I know why you have the error above. because of this change: b2b85e5#diff-259e627be3fafed9e9c10a7fcb26879e

It added a section of new code into node.gyp and use the upper case variables. This new section should be removed. I can remove it in this PR

I mistakenly introduced user defined variables using uppercase characters, reading the gyp documentation they state: "Predefined variables. By convention, these are named with CAPITAL_LETTERS. Predefined variables are set automatically by GYP" and also "By convention, user-defined variables are named with lowercase_letters." This commit renames the user defined variables to lowercase to follow the above mentioned convention. PR_URL: nodejs#20797 Original PR-URL: nodejs#16238 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-05-22T19:16:02Z

@yhwang if you could remove it in this PR that would be rad. Thanks

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#20797 Original PR-URL: nodejs#18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

yhwang · 2018-05-22T20:25:49Z

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

yhwang · 2018-05-22T23:20:40Z

parallel/test-timers-throw-reschedule this test case failed in other v8.x-staging builds. It's not related to this PR.

I mistakenly introduced user defined variables using uppercase characters, reading the gyp documentation they state: "Predefined variables. By convention, these are named with CAPITAL_LETTERS. Predefined variables are set automatically by GYP" and also "By convention, user-defined variables are named with lowercase_letters." This commit renames the user defined variables to lowercase to follow the above mentioned convention. Backport-PR-URL: #20797 PR-URL: #16238 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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. Backport-PR-URL: #20797 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2018-05-23T16:09:35Z

landed in 0082efd...04c166a

I mistakenly introduced user defined variables using uppercase characters, reading the gyp documentation they state: "Predefined variables. By convention, these are named with CAPITAL_LETTERS. Predefined variables are set automatically by GYP" and also "By convention, user-defined variables are named with lowercase_letters." This commit renames the user defined variables to lowercase to follow the above mentioned convention. Backport-PR-URL: #20797 PR-URL: #16238 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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. Backport-PR-URL: #20797 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

I mistakenly introduced user defined variables using uppercase characters, reading the gyp documentation they state: "Predefined variables. By convention, these are named with CAPITAL_LETTERS. Predefined variables are set automatically by GYP" and also "By convention, user-defined variables are named with lowercase_letters." This commit renames the user defined variables to lowercase to follow the above mentioned convention. Backport-PR-URL: #20797 PR-URL: #16238 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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. Backport-PR-URL: #20797 PR-URL: #18576 Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8.x labels May 17, 2018

MylesBorins force-pushed the v8.x-staging branch from 5ac4bac to 591812f Compare May 22, 2018 15:31

yhwang force-pushed the backport-16238-18576-to-v8.x branch from f0baab1 to 4aaa601 Compare May 22, 2018 20:22

MylesBorins closed this May 23, 2018

yhwang deleted the backport-16238-18576-to-v8.x branch May 23, 2018 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[v8.x backport] 16238 and 18576#20797

[v8.x backport] 16238 and 18576#20797
yhwang wants to merge 2 commits intonodejs:v8.x-stagingfrom
yhwang:backport-16238-18576-to-v8.x

yhwang commented May 17, 2018 •

edited

Loading

Uh oh!

yhwang commented May 17, 2018

Uh oh!

MylesBorins commented May 17, 2018

Uh oh!

yhwang commented May 17, 2018

Uh oh!

MylesBorins commented May 22, 2018 •

edited

Loading

Uh oh!

yhwang commented May 22, 2018 •

edited

Loading

Uh oh!

MylesBorins commented May 22, 2018

Uh oh!

yhwang commented May 22, 2018

Uh oh!

yhwang commented May 22, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

yhwang commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhwang commented May 17, 2018

Uh oh!

MylesBorins commented May 17, 2018

Uh oh!

yhwang commented May 17, 2018

Uh oh!

MylesBorins commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhwang commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented May 22, 2018

Uh oh!

yhwang commented May 22, 2018

Uh oh!

yhwang commented May 22, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yhwang commented May 17, 2018 •

edited

Loading

MylesBorins commented May 22, 2018 •

edited

Loading

yhwang commented May 22, 2018 •

edited

Loading