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

meta: update NODE_MODULE_VERSION to 51 #8808

Merged
merged 1 commit into from Sep 29, 2016

Conversation

@MylesBorins
Member

MylesBorins commented Sep 27, 2016

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

src

Description of change
src: update NODE_MODULE_VERSION to 51

When V8 was updated on master to 5.4 there were ABI breaking changes.
In the past we have not landed these types of changes before a release,
and as such have only bumped the NODE_MODULE_VERSION number in the
release commit.

Since we are going to be keeping the V8 5.4 beta on master and in the
v7 betas I think it makes sense for us to bump the module number prior
to a release commit being made. It is possible that this commit should
be reverted prior to v7.0.0 being cut. Alternatively we may want to
modify our release process for V8 to include a NODE_MODULE_VERSION
bump before landing on master when applicable.

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: https://github.com/electron/electron/issues/5851#issuecomment-246920775
Ref: https://github.com/nodejs/node/pull/8317

/cc @nodejs/v8 @nodejs/ctc

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch Sep 27, 2016

@MylesBorins MylesBorins added this to the 7.0.0 milestone Sep 27, 2016

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 27, 2016

You probably didn't mean to check in a.patch.

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch Sep 27, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Sep 27, 2016

thanks for the quick check there. sorry about that

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Sep 27, 2016

As per discussion here: electron/electron#5851 (comment) it would be good to skip to module version 51 for better compatibility with electron.
/cc @addaleax

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch Sep 27, 2016

@MylesBorins MylesBorins changed the title from meta: update NODE_MODULE_VERSION to 49 to meta: update NODE_MODULE_VERSION to 51 Sep 27, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Sep 27, 2016

@ofrobots updated with suggestion

@bnoordhuis

LGTM but can you explain in the commit log why it jumps from 48 to 51?

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch Sep 27, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Sep 27, 2016

updated

I've added the following to the commit message

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: https://github.com/electron/electron/issues/5851#issuecomment-246920775
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Sep 27, 2016

cc @rvagg

@addaleax

LGTM with the tiny nit that I would prefer to have “#8317” written out as a full URL in the commit message.

@addaleax addaleax added addons and removed module labels Sep 27, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 27, 2016

LGTM

1 similar comment
@evanlucas

This comment has been minimized.

Member

evanlucas commented Sep 27, 2016

LGTM

@targos targos added the semver-major label Sep 28, 2016

@targos

This comment has been minimized.

Member

targos commented Sep 28, 2016

LGTM

@targos

targos approved these changes Sep 28, 2016

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch Sep 28, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Sep 28, 2016

I've addressed all nits. I would like to land this today if people don't mind as it is blocking smoke testing of v7 and of master.

@Fishrock123 I know you pinged @rvagg who was likely too busy with the release yesterday to take a look. Was that just for oversight or do you think he would take issue with this?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Sep 28, 2016

I wouldn't mind his input here, he originally lead a lot of these bumps and it's be nice to get his thoughts. I don't think there will be objections, but ¯_(ツ)_/¯

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Sep 28, 2016

I have no problem waiting then. In fact let's put it on the CTC agenda. I think it is worth discussing if we should be skipping NODE_MODULE_VERSION and if we want to change our process.

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch 2 times, most recently Sep 28, 2016

src: update NODE_MODULE_VERSION to 51
When V8 was updated on master to 5.4 there were ABI breaking changes.
In the past we have not landed these types of changes before a release,
and as such have only bumped the NODE_MODULE_VERSION number in the
release commit.

Since we are going to be keeping the V8 5.4 beta on master and in the
v7 betas I think it makes sense for us to bump the module number prior
to a release commit being made. It is possible that this commit should
be reverted prior to v7.0.0 being cut. Alternatively we may want to
modify our release process for V8 to include a NODE_MODULE_VERSION
bump before landing on master when applicable.

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: electron/electron#5851 (comment)
Ref: #8317

PR-URL: #8808
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins force-pushed the MylesBorins:node-modules-49 branch to b5bdff8 Sep 29, 2016

@MylesBorins MylesBorins merged commit b5bdff8 into nodejs:master Sep 29, 2016

jasnell added a commit that referenced this pull request Sep 29, 2016

src: update NODE_MODULE_VERSION to 51
When V8 was updated on master to 5.4 there were ABI breaking changes.
In the past we have not landed these types of changes before a release,
and as such have only bumped the NODE_MODULE_VERSION number in the
release commit.

Since we are going to be keeping the V8 5.4 beta on master and in the
v7 betas I think it makes sense for us to bump the module number prior
to a release commit being made. It is possible that this commit should
be reverted prior to v7.0.0 being cut. Alternatively we may want to
modify our release process for V8 to include a NODE_MODULE_VERSION
bump before landing on master when applicable.

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: electron/electron#5851 (comment)
Ref: #8317

PR-URL: #8808
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@jasnell jasnell referenced this pull request Oct 14, 2016

Closed

v7.0.0 Proposal #9099

@Trott Trott removed the ctc-agenda label Oct 20, 2016

jasnell added a commit to jasnell/node that referenced this pull request Oct 24, 2016

2016-10-25, Version 7.0.0 (Current)
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [nodejs#8946](nodejs#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [nodejs#8169](nodejs#8169).
  * Passing a negative number to allocUnsafe will now throw an error [nodejs#7079](nodejs#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [nodejs#7399](nodejs#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [nodejs#3747](nodejs#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [nodejs#8317](nodejs#8317), [nodejs#8852](nodejs#8852), [nodejs#9253](nodejs#9253).
  * NODE_MODULE_VERSION has been updated to 51 [nodejs#8808](nodejs#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [nodejs#7897](nodejs#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [nodejs#8908](nodejs#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [nodejs#8217](nodejs#8217).
* Punycode
  * The `punycode` module has been deprecated [nodejs#7941](nodejs#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [nodejs#7448](nodejs#7448).

jasnell added a commit that referenced this pull request Oct 25, 2016

2016-10-25, Version 7.0.0 (Current)
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099

jasnell added a commit that referenced this pull request Oct 25, 2016

2016-10-25, Version 7.0.0 (Current)
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099

imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 25, 2016

2016-10-25, Version 7.0.0 (Current)
    Notable Changes:

    * Buffer
      * Passing invalid input to Buffer.byteLength will now throw an error [#8946](nodejs/node#8946).
      * Calling Buffer without new is now deprecated and will emit a process warning [#8169](nodejs/node#8169).
      * Passing a negative number to allocUnsafe will now throw an error [#7079](nodejs/node#7079).
    * Child Process
      * The fork and execFile methods now have stronger argument validation [#7399](nodejs/node#7399).
    * Cluster
      * The worker.suicide method is deprecated and will emit a process warning [#3747](nodejs/node#3747).
    * Deps
      * V8 has been updated to 5.4.500.36 [#8317](nodejs/node#8317), [#8852](nodejs/node#8852), [#9253](nodejs/node#9253).
      * NODE_MODULE_VERSION has been updated to 51 [#8808](nodejs/node#8808).
    * File System
      * A process warning is emitted if a callback is not passed to async file system methods [#7897](nodejs/node#7897).
    * Intl
      * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](nodejs/node#8908).
    * Promises
      * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](nodejs/node#8217).
    * Punycode
      * The `punycode` module has been deprecated [#7941](nodejs/node#7941).
    * URL
      * An Experimental WHATWG URL Parser has been introduced [#7448](nodejs/node#7448).

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

@MylesBorins MylesBorins deleted the MylesBorins:node-modules-49 branch Nov 14, 2017

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