configure: fix type of arm_version #14595

hujiajie · 2017-08-02T11:01:59Z

arm_version is compared with an integer 7 in V8's GYP files, instead of the string "7", so it seems that this variable should be an integer when it is representing a version number. Its default value in V8's
standalone.gypi seconds this.

It also seems that GYP can quietly convert an integer-like string to an integer in some scenarios, maybe during the merge of some dictionaries, so luckily no actual problem happens at the moment. However, the subtle difference between 7 and "7" will be observed when a new GYP variable is added to V8's toolchain.gypi like this:

{
  'variables': {
    ...
    'conditions': [
      ...
      ['arm_version==7', {
        'myvar%': 'abc',
      }],
    ],
    ...
  },
  ...
}

Interestingly no implicit conversion happens here when arm_version is "7", so GYP will complain about the undefined variable <(myvar) if it's referenced later on.

Checklist

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

Affected core subsystem(s)

build, arm

arm_version is compared with an integer 7 in V8's GYP files, instead of the string "7", so it seems that this variable should be an integer when it is representing a version number. Its default value in V8's standalone.gypi seconds this. It also seems that GYP can quietly convert an integer-like string to an integer in some scenarios, maybe during the merge of some dictionaries, so luckily no actual problem happens at the moment. However, the subtle difference between 7 and "7" will be observed when a new GYP variable is added to V8's toolchain.gypi like this: { 'variables': { ... 'conditions': [ ... ['arm_version==7', { 'myvar%': 'abc', }], ], ... }, ... } Interestingly no implicit conversion happens here when arm_version is "7", so GYP will complain about the undefined variable <(myvar) if it's referenced later on.

mscdex · 2017-08-02T19:04:02Z

Won't this break any/all existing binding.gyp files out in the wild that currently check against the string version?

Also, it seems like you should be guarding against (in one way or another) undefined variables like that, especially when the variable is defined within a conditional.

hujiajie · 2017-08-03T01:28:55Z

Won't this break any/all existing binding.gyp files out in the wild that currently check against the string version?

I have the same concern, so looking forward to more feedback.

Also, it seems like you should be guarding against (in one way or another) undefined variables like that, especially when the variable is defined within a conditional.

Do you mean something like this:

{
  'variables': {
    ...
    'conditions': [
      ...
      ['arm_version==7', {
        'myvar%': 'abc',
      }],
    ],
    ...
  },
  'target_defaults': {
    ...
    'conditions': [
      ...
      ['arm_version==7', {
        'cflags': [ '<(myvar)' ],
      }],
      ...
    ],
    ...
  },
}

Absolutely later references to myvar should be guarded, though I didn't mention that before for simplicity. However, the two 'arm_version==7' expressions are evaluated to different results, and in fact the case is the origination of my confusion.

refack · 2017-08-03T12:45:52Z

Won't this break any/all existing binding.gyp files out in the wild that currently check against the string version?

I have the same concern, so looking forward to more feedback.

Interestingly from a quick look at a GitHub search it seems it's more common to compare to a number then to a string (clones of V8's toolchain.gypi are very popular) so it seems like this will do more good then harm.

But if we want to be extra conservative you could define a new var { 'arm_version_numeric': 7 }

/cc @bnoordhuis @nodejs/platform-arm

BridgeAR · 2017-08-31T17:27:21Z

@nodejs/build PTAL

BridgeAR · 2017-09-13T19:22:13Z

Again @nodejs/build PTAL

refack · 2017-09-14T13:57:29Z

@nodejs/platform-arm PTAL

BridgeAR · 2017-09-22T21:47:01Z

Ping @nodejs/tsc

BridgeAR · 2017-09-25T20:14:28Z

I am marking this defensively as semver-major.

BridgeAR · 2017-09-28T06:07:45Z

@nodejs/tsc PTAL

BridgeAR · 2017-11-22T18:24:28Z

@nodejs/tsc PTAL!

jasnell · 2017-11-22T18:28:49Z

still LGTM

BridgeAR · 2017-11-22T18:34:46Z

Well, it still needs another LG

Trott · 2017-11-24T13:22:09Z

I'll be surprised if this uncovers any problems, but we should do this for all semver-major changes, so...

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1107/

refack

FWIW

Trott · 2017-11-28T17:42:28Z

Needs one more TSC member approval and the most obvious reviewer is @rvagg so...ping!

mhdawson · 2017-11-28T22:09:19Z

Did somebody review the citgm results to figure out if they are ok or not ?

MylesBorins · 2017-11-29T05:31:47Z

Can you please rebase this against master and then run CITGM again. CITGM was run against a head that was still pre-release

Trott · 2017-12-01T02:50:56Z

Can you please rebase this against master and then run CITGM again.

You can instruct the citgm-smoker job to rebase against origin/master in the form so probably no need to rebase against master in this branch. Or am I missing a subtlety?

Trott · 2017-12-01T03:25:42Z

CITGM with rebase against master (I think--I put origin/master in the REBASE_ONTO field): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1122/

rvagg · 2017-12-02T01:32:03Z

meh, I think I'm -0 on this for now. We're making a pretty safe assumption that (a) we're limited to Cortex-A and (b) ARM will stick with its numbering. But let's take a look at both of those:

It's not entirely implausible that we might see a Cortex-R or Cortex-M version of Node at some point in the future (probably not using V8 I reckon). The standard way of naming architectures is like "ARMv7-A", "ARMv7-R", etc. We're just pulling out the number, we'd need an additional way of identifying variants and arm_version seems like a good place to extend to something like '8R'. Of course an alternative would be arm_variant but that might be stripping too much information out of arm_version because M and R are very different to A and it's the kind of thing you'd want front and centre.
ARM decided to do a minor rebrand with v8 by referring to it as "ARM64" or "AArch64" even though underneath we still use ARMv8. I'm not confident that they won't throw a spanner in the works as soon as v9 since they've boxed themselves in a corner with the unversioned "ARM64" / "AArch64". Then when v10 comes out you have an alphanumeric sorting problem so maybe just throw it up in the air anyway.

But as I said, pretty safe assumption that it won't change in the near future, hence I'm not a full -1 here. I'd probably be happier with arm_version_numeric or similar.

@bnoordhuis any thoughts from you?

MylesBorins · 2017-12-04T20:04:01Z

I've run citgm against the latest v9.x release to compare https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/205/

I'm going to start auditing the failures and figure out if we can get closer to green

BridgeAR · 2017-12-15T23:46:37Z

@MylesBorins what was the result of the audit? Did it look good or not really?

BridgeAR · 2018-01-12T15:36:17Z

Ping @MylesBorins

BridgeAR · 2018-01-19T16:02:40Z

@nodejs/tsc PTAL. This needs another LG and it is somewhat controversial.

joyeecheung · 2018-01-24T12:21:20Z

Considering #14595 (comment) I am -0 towards this change, arm_version_numeric sounds like a better idea if we can update the upstream as well.

Trott · 2018-01-24T14:29:35Z

I'm completely neutral on this. I feel like it's probably OK but I defer to people like @rvagg and @joyeecheung on this sort of stuff. I'm only leaving this comment because I know there's a need for another TSC approval and I wanted to explain why I've not been the one to come along and give it.

mcollina · 2018-01-24T15:06:09Z

I have no ground to make an informed decision here. I do not see how much of the ecosystem could be broken by this PR.
I’m -0, as the potential breakage might be too much.

fhinkel · 2018-01-31T16:49:48Z

I’m -0, as the potential breakage might be too much.

Same.

Fishrock123 · 2018-01-31T17:22:50Z

I think Rod's comment here make sense, also -0.

mhdawson · 2018-01-31T17:29:16Z

This was discussed in the TSC meeting, but people were not comfortable that we have enough data showing that it won't cause breakage for users. Since it does not seem to fix an urgent issue until there is more data/discussion it needs to wait.

BridgeAR · 2018-02-07T02:42:33Z

As most people do not seem comfortable with this I am going to close it.

@hujiajie thanks a lot for your work! It is much appreciated even though this did not land!

If anyone feels like closing this is a mistake, please leave a comment / reopen.

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

BridgeAR requested a review from a team September 24, 2017 15:51

jasnell approved these changes Sep 25, 2017

View reviewed changes

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2017

BridgeAR approved these changes Sep 25, 2017

View reviewed changes

BridgeAR requested a review from a team September 28, 2017 06:07

BridgeAR added the tsc-review label Nov 22, 2017

refack approved these changes Nov 27, 2017

View reviewed changes

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

rvagg mentioned this pull request Dec 19, 2017

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-12-20 nodejs/TSC#447

Closed

BridgeAR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 19, 2018

mhdawson mentioned this pull request Jan 22, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-01-24 nodejs/TSC#475

Closed

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

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

mhdawson mentioned this pull request Jan 29, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-01-31 nodejs/TSC#478

Closed

mhdawson mentioned this pull request Feb 5, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-02-07 nodejs/TSC#484

Closed

BridgeAR closed this Feb 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configure: fix type of arm_version #14595

configure: fix type of arm_version #14595

hujiajie commented Aug 2, 2017 •

edited

Loading

mscdex commented Aug 2, 2017

hujiajie commented Aug 3, 2017

refack commented Aug 3, 2017

BridgeAR commented Aug 31, 2017

BridgeAR commented Sep 13, 2017

refack commented Sep 14, 2017

BridgeAR commented Sep 22, 2017

BridgeAR commented Sep 25, 2017

BridgeAR commented Sep 28, 2017

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

BridgeAR commented Nov 22, 2017

Trott commented Nov 24, 2017

refack left a comment

Trott commented Nov 28, 2017

mhdawson commented Nov 28, 2017 •

edited

Loading

MylesBorins commented Nov 29, 2017

Trott commented Dec 1, 2017

Trott commented Dec 1, 2017

rvagg commented Dec 2, 2017 •

edited

Loading

MylesBorins commented Dec 4, 2017

BridgeAR commented Dec 15, 2017

BridgeAR commented Jan 12, 2018

BridgeAR commented Jan 19, 2018

joyeecheung commented Jan 24, 2018

Trott commented Jan 24, 2018

mcollina commented Jan 24, 2018

fhinkel commented Jan 31, 2018

Fishrock123 commented Jan 31, 2018

mhdawson commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

configure: fix type of arm_version #14595

configure: fix type of arm_version #14595

Conversation

hujiajie commented Aug 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented Aug 2, 2017

hujiajie commented Aug 3, 2017

refack commented Aug 3, 2017

BridgeAR commented Aug 31, 2017

BridgeAR commented Sep 13, 2017

refack commented Sep 14, 2017

BridgeAR commented Sep 22, 2017

BridgeAR commented Sep 25, 2017

BridgeAR commented Sep 28, 2017

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

BridgeAR commented Nov 22, 2017

Trott commented Nov 24, 2017

refack left a comment

Choose a reason for hiding this comment

Trott commented Nov 28, 2017

mhdawson commented Nov 28, 2017 • edited Loading

MylesBorins commented Nov 29, 2017

Trott commented Dec 1, 2017

Trott commented Dec 1, 2017

rvagg commented Dec 2, 2017 • edited Loading

MylesBorins commented Dec 4, 2017

BridgeAR commented Dec 15, 2017

BridgeAR commented Jan 12, 2018

BridgeAR commented Jan 19, 2018

joyeecheung commented Jan 24, 2018

Trott commented Jan 24, 2018

mcollina commented Jan 24, 2018

fhinkel commented Jan 31, 2018

Fishrock123 commented Jan 31, 2018

mhdawson commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

hujiajie commented Aug 2, 2017 •

edited

Loading

mhdawson commented Nov 28, 2017 •

edited

Loading

rvagg commented Dec 2, 2017 •

edited

Loading