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.

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

deps: add back no-op harmony shipping flags #8445

Merged
merged 1 commit into from Sep 13, 2016

Conversation

Projects
None yet
8 participants
@ofrobots
Contributor

ofrobots commented Sep 8, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps:v8

Description of change

This PR adds back the shipping harmony flags (i.e. no-ops) that were removed in V8 5.1. While uses of V8 flags is generally not supported by Node, in this case a trivial change reduces minor hardship in the ecosystem by adding back these flags (and making them do nothing).

Fixes: #8388
Ref: #8395
R=@nodejs/v8
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/3967/
CI: https://ci.nodejs.org/job/node-test-pull-request/4033/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/310/

@ofrobots ofrobots referenced this pull request Sep 8, 2016

Closed

src: add no-op for --harmony-proxies flag #8395

3 of 3 tasks complete
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 8, 2016

Member

LGTM pending green CIs… thanks for putting this together!

Member

addaleax commented Sep 8, 2016

LGTM pending green CIs… thanks for putting this together!

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 8, 2016

Member

LGTM

tested locally to confirm that the noop flags work as expected

Member

MylesBorins commented Sep 8, 2016

LGTM

tested locally to confirm that the noop flags work as expected

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 8, 2016

Contributor

Note that I have not added back the --harmony_modules flag. It was removed in V8 5.1, but it was not a 'shipping', but rather an 'in progress' feature. Since modules require embedder support anyway, I don't think much, if anything, is dependent on that flag.

Contributor

ofrobots commented Sep 8, 2016

Note that I have not added back the --harmony_modules flag. It was removed in V8 5.1, but it was not a 'shipping', but rather an 'in progress' feature. Since modules require embedder support anyway, I don't think much, if anything, is dependent on that flag.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 8, 2016

Member

This LGTM for v6, but I want to make sure it's absolutely clear that these won't be in v7. We should likely have a documented policy that V8 flags removed within a major would only be noop'ed in that major, and removed completely in the next major.

Member

jasnell commented Sep 8, 2016

This LGTM for v6, but I want to make sure it's absolutely clear that these won't be in v7. We should likely have a documented policy that V8 flags removed within a major would only be noop'ed in that major, and removed completely in the next major.

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Sep 8, 2016

Member

LGTM. Thanks @ofrobots!

Member

evanlucas commented Sep 8, 2016

LGTM. Thanks @ofrobots!

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 8, 2016

Contributor

We should likely have a documented policy that V8 flags removed within a major would only be noop'ed in that major, and removed completely in the next major.

I think our policy should be that use of V8 flags is not supported. This is a one-off change, and not intended to set precedent that we want to be playing with V8 flags.

Contributor

ofrobots commented Sep 8, 2016

We should likely have a documented policy that V8 flags removed within a major would only be noop'ed in that major, and removed completely in the next major.

I think our policy should be that use of V8 flags is not supported. This is a one-off change, and not intended to set precedent that we want to be playing with V8 flags.

@thetutlage thetutlage referenced this pull request Sep 8, 2016

Closed

fresh setup - error #216

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 12, 2016

Contributor

@bnoordhuis you had left a -1 on #8395 on the idea of floating a patch. Would you be okay with this change?

Contributor

ofrobots commented Sep 12, 2016

@bnoordhuis you had left a -1 on #8395 on the idea of floating a patch. Would you be okay with this change?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 12, 2016

Member

LGTM

Member

targos commented Sep 12, 2016

LGTM

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 13, 2016

Member

A grumbling LGTM.

Member

bnoordhuis commented Sep 13, 2016

A grumbling LGTM.

deps: add back no-op harmony shipping flags
Add back the no-op harmony shipping flags that were removed in V8 5.1
to increase compatibility with V8 5.0 that we had been shipping before
v6.5.0. These flags do nothing.

Fixes: #8388
Ref: #8395
PR-URL: #8445
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: thealphanerd - Myles Borins <myles.borins@gmail.com>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

@ofrobots ofrobots merged commit 9c460d7 into nodejs:v6.x-staging Sep 13, 2016

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Sep 13, 2016

Contributor

Thanks. Landed on v6.x-staging as 9c460d7.

Contributor

ofrobots commented Sep 13, 2016

Thanks. Landed on v6.x-staging as 9c460d7.

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