build: make gyp user defined variables lowercase by danbev · Pull Request #16238 · nodejs/node

danbev · 2017-10-16T17:40:14Z

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.

Checklist

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

Affected core subsystem(s)

build

danbev · 2017-10-16T17:40:50Z

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

bnoordhuis

Technically LGTM but note that the changes to common.gypi are inherited by native modules so they could break things downstream. See also nodejs/node-gyp#1118.

danbev · 2017-10-17T05:48:15Z

Technically LGTM but note that the changes to common.gypi are inherited by native modules so they could break things downstream.

Thanks, I was not aware of that. I'll watch nodejs/node-gyp#1118 and see what happens with it.

BridgeAR · 2017-11-22T19:14:03Z

This needs a rebase

BridgeAR · 2017-12-06T04:17:20Z

Shall we make this semver-major to be on the safe side?

BridgeAR · 2017-12-12T19:53:47Z

I decided to run CITGM to see if this breaks anything there
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1146/

addaleax · 2017-12-13T05:50:12Z

@danbev This doesn’t appear to let ./configure pass anymore … could you take a look?

danbev · 2017-12-13T07:51:12Z

@danbev This doesn’t appear to let ./configure pass anymore … could you take a look?

Absolutely, might not be until tomorrow but I'll take a look.

danbev · 2017-12-14T06:38:56Z

@danbev This doesn’t appear to let ./configure pass anymore … could you take a look?

I've tried this locally (./configure --debug && make -j8 test) and not getting an error. I've started a CI run to see if this is a local setting/configuration that I'm not taking into account.

~~@addaleax Did you simply run ./configure and that fails for you? Could you post the error you are seeing? Thanks~~

danbev · 2017-12-14T09:18:39Z

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

addaleax · 2017-12-14T11:11:19Z

CI again since the last one had quite a few infra failures: https://ci.nodejs.org/job/node-test-commit/14827/

maclover7 · 2017-12-19T00:53:57Z

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

BridgeAR · 2018-01-19T16:11:14Z

~~And another one https://ci.nodejs.org/job/node-test-pull-request/12621/~~

BridgeAR · 2018-01-19T16:11:25Z

@danbev would you be so kind and rebase?

danbev · 2018-01-20T05:53:08Z

@BridgeAR Same here, I hope to revisit this next week (though I've got a sick kid here and I might not be working at all next week by the looks of it :( )

BridgeAR · 2018-01-31T01:45:43Z

Ping @danbev

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.

danbev · 2018-01-31T06:03:24Z

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

BridgeAR · 2018-02-01T08:52:30Z

Landed in ac7f1e3

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#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-02-20T23:18:45Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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#16238 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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: #18899 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>

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#16238 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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>

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>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 16, 2017

bnoordhuis approved these changes Oct 16, 2017
View reviewed changes

danbev force-pushed the lowercase-user-vars branch from 3fbfa1e to d125e15Compare November 23, 2017 09:01

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2017

BridgeAR approved these changes Dec 6, 2017
View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086ddCompare December 8, 2017 16:37

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2018

maclover7 force-pushed the master branch from bb5575a to 993b716Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952Compare January 26, 2018 22:36

danbev force-pushed the lowercase-user-vars branch from 1bb202e to dd15bd0Compare January 31, 2018 06:02

jasnell approved these changes Jan 31, 2018
View reviewed changes

BridgeAR closed this Feb 1, 2018

MylesBorins added backported-to-v9.x and removed backported-to-v9.x labels Feb 20, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2018

addaleax mentioned this pull request Feb 21, 2018
[v9.x] build: make gyp user defined variables lowercase #18899
Closed

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 21, 2018

addaleax mentioned this pull request Feb 27, 2018
v9.7.0 proposal #19040
Closed

danbev deleted the lowercase-user-vars branch February 28, 2018 07:16

This was referenced May 17, 2018
won't link on OS X with --enable-static #20763
Closed
[v8.x backport] 16238 and 18576 #20797
Closed

MylesBorins mentioned this pull request Jul 9, 2018
v8.12.0 proposal #21593
Merged

Uh oh!

Uh oh!

Conversation

danbev commented Oct 16, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Oct 16, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented Oct 17, 2017

Uh oh!

BridgeAR commented Nov 22, 2017

Uh oh!

BridgeAR commented Dec 6, 2017

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

danbev commented Dec 13, 2017

Uh oh!

danbev commented Dec 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbev commented Dec 14, 2017

Uh oh!

addaleax commented Dec 14, 2017

Uh oh!

maclover7 commented Dec 19, 2017

Uh oh!

BridgeAR commented Jan 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jan 19, 2018

Uh oh!

danbev commented Jan 20, 2018

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

danbev commented Jan 31, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

danbev commented Dec 14, 2017•
edited
Loading

BridgeAR commented Jan 19, 2018•
edited
Loading