Skip to content
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

Release proposal: v2.3.1 #1996

Closed
wants to merge 1 commit into from
Closed

Release proposal: v2.3.1 #1996

wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 19, 2015

As pointed out by @silverwind, 671e64a contains an important fix for Windows users that we should get out ASAP. Let me know if there objections to a 24-hour window for this, I figure that a patch release with a limited list of commits should be straightforward.

  • [e56758a5e0] - async-wrap: add provider id and object info cb (Trevor Norris) #1896
  • [d5637e67c9] - buffer: fix cyclic dependency with util (Brendan Ashworth) #1988
  • [c207e8d223] - build: fix pkg-config output parsing in configure (Ben Noordhuis) #1986
  • [8d8a26e8f7] - build: don't run lint from test-ci (Johan Bergström) #1965
  • [1ec53c044d] - build: simplify execution of built binary (Johan Bergström) #1955
  • [3beb880716] - crypto: add cert check to CNNIC Whitelist (Shigeki Ohtsu) #1895
  • [6aab2f3b9a] - deps: make node-gyp work with io.js (cjihrig) iojs/io.js#990
  • [3e12561b55] - deps: upgrade to npm 2.11.2 (Rebecca Turner) #1956
  • [1f93b63b11] - doc: change the info to the same as in gitconfig (Christian Tellnes) #2000
  • [0cf94e6856] - doc: mention CI in Collaborator Guide (Rich Trott) #1995
  • [7a3006efe4] - doc: add TOC links to Collaborator Guide (Rich Trott) #1994
  • [30638b150f] - doc: add TSC meeting notes 2015-06-10 (Bert Belder) #1943
  • [c4ec04136b] - doc: reformat authors section (Johan Bergström) #1966
  • [96165f9be2] - doc: minor clarification in the modules API doc. (Сковорода Никита Андреевич) #1983
  • [5c2707c1b2] - doc: benchmark/README.md copyedit (Rich Trott) #1970
  • [74fdf732d0] - doc: copyedit COLLABORATOR_GUIDE.md (Rich Trott) #1964
  • [5fe6e83640] - doc: copyedit GOVERNANCE.md (Rich Trott) #1963
  • [428526544c] - doc: add ChALkeR as collaborator (Сковорода Никита Андреевич) #1927
  • [5dfe0d5d61] - doc: remove irrelevant SEMVER-MINOR & MAJOR (Rod Vagg)
  • [fb8811d95e] - lib,test: fix whitespace issues (Roman Reiss) #1971
  • [a71ee93afe] - module: reduce syscalls during require search (Pierre Inglebert) #1920
  • [671e64ac73] - module: allow long paths for require on Windows (Michaël Zasso)
  • [061342a500] - net: Defer reading until listeners could be added (James Hartig) #1496
  • [0abcf44d6b] - test: add Buffer slice UTF-8 test (Rich Trott) #1989
  • [88c1831ff4] - test: tmpdir creation failures should fail tests (Rich Trott) #1976
  • [52a822d944] - test: fix test-cluster-worker-disconnect (Santiago Gimeno) #1919
  • [7c79490bfb] - test: only refresh tmpDir for tests that need it (Rich Trott) #1954
  • [88d7904c0b] - test: remove test repetition (Rich Trott) #1874
  • [91dfb5e094] - tools: make test-npm work without global npm (Jeremiah Senkpiel) #1926
  • [3777f41562] - tools: enable whitespace related rules in eslint (Roman Reiss) #1971
  • [626432d843] - util: dont repeat isBuffer (Brendan Ashworth) #1988
  • [1d79f572f1] - util: move deprecate() to internal module (Brendan Ashworth) #1988
  • [4b4b1760b5] - v8: cherry-pick uclibc build patch from upstream (Ben Noordhuis) #1974
  • [5d0cee46bb] - vm: remove unnecessary HandleScopes (Ben Noordhuis) #2001
  • [953b3e75e8] - win,node-gyp: enable delay-load hook by default (Bert Belder) iojs/io.js#1433
  • [3806d875d3] - zlib: prevent uncaught exception in zlibBuffer (Michaël Zasso) #1811

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Jun 16, 2015
@silverwind
Copy link
Contributor

Noteable changes besides the npm update should be 671e64a, 3806d87 and 671e64a...626432d.

@Fishrock123
Copy link
Contributor

LGTM

@piscisaureus
Copy link
Contributor

Tangentially related - would it be okay to transliterate Russian names? Or is this super sensitive?
I'm thinking that "Skovoroda Nikita Andreevich" would be easier to read for 90% of the audience than "Сковорода Никита Андреевич". @ChALkeR

@brendanashworth
Copy link
Contributor

@piscisaureus there was conversation about that in #1927, but I don't think a decision was reached.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2015

@piscisaureus It would be ok from my side. If you do that, you can use «Nikita Skovoroda» (first and last name correspondingly). The current variant was copied from my git settings and/or GitHub user profile.

Previous discussion here: #1927.
See commit 4285265, it has the transliterated name in the message.

@bricss
Copy link

bricss commented Jun 17, 2015

Could we also add this to release? #1982

@rvagg
Copy link
Member Author

rvagg commented Jun 17, 2015

that'll have to go in to .mailmap to make it happen @piscisaureus and @ChALkeR

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2015

@bricss That one is not finished yet and has an alternate version in #1998.

It requires further discussion and there is no need to delay v2.3.1 release for that, v2.3.1 fixes more serious bugs. #1982/#1998 atm fixes one module using the API in an incorrect and undocumented way (the other one got patched already, there might be more, but I know of only two for sure).

I would recommend fixing your problem by merging dtabuenc/karma-html-reporter#27 instead, if you want a fix asap.

@bricss
Copy link

bricss commented Jun 17, 2015

@ChALkeR #1982 looks much better then #1998 I guess it would be possible to choose one and merge before release, cause this issue could be very sensitive for a lot of people and these projects.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2015

@bricss, This is not the correct place to discuss which of #1998 and #1982 should be merged.
And I see no reason holding a release that fixes more serious io.js-side issues (namely 3806d87 and 671e64a) that affect a lot of people because of the fix that addresses an incorrect API usage by one (until you name the others) third-party module (not io.js side) is not being reviewed yet.
That one should go into the next patch release, if there is nothing else holding off the release now. It shouldn't block the release.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 17, 2015

@piscisaureus, @rvagg, @brendanashworth

If the question on how you should write my name is holding the release, just remove those two commits from the above list and put them in some later release when a consensus is reached. Those two commits are insignificant, they target the documentation only.

3806d87 is strictly speaking a security fix and should not be delayed,
671e64a fixes a problem that, while being a Windows-only bug, caused three bugreports in io.js to the date.

@bricss
Copy link

bricss commented Jun 17, 2015

«Nikita Skovoroda» sounds pretty good.

@Fishrock123
Copy link
Contributor

Updated the commit list in the OP. @rvagg lmk if you're able to get this out today, otherwise I'll see about doing it later today.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 18, 2015

@Fishrock123 This should not be released as-is with the updated commit list.
Either remove a71ee93 from 2.3.1, or wait until #2011 is merged.
If it was up to me, I would exclude a71ee93 from this release because it should not be delayed.

@Fishrock123
Copy link
Contributor

Either #2011 may have to land (with or without a test?) or a71ee93 may have to be reverted.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 18, 2015

Could we just tag 061342a as 2.3.1? There are three commits after that one — a71ee93 and two documentation-related.

Edit: Ah, each release requires a commit, so we can't. Without making a separate branch, that is.

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

@chrisdickinson thoughts on branching to get a 2.3.1 out? I have no problem with it but perhaps there's something special we need to be considering here?

@Fishrock123 @ChALkeR also an option is forcing a commit in there and forcing a rebase of everything that's in after and have their shas change. I also don't have a great problem with that but then again I'm a git cowboy.

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

... and sorry for being tardy with this, I said I'd release this yesterday but didn't so this is my fault.

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

  • [1f93b63b11] - doc: change the info to the same as in gitconfig (Christian Tellnes) #2000
  • [0cf94e6856] - doc: mention CI in Collaborator Guide (Rich Trott) #1995
  • [a71ee93afe] - module: reduce syscalls during require search (Pierre Inglebert) #1920

there's only these 3 that would be changed, so perhaps not a big deal?

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

Now also:

  • [c5353d7c62] - build: remove lint from test-ci on windows (Johan Bergström) #2004

Imma do a release and force push before any more commits land. Stop me now if you think this is a terrible idea!

CI @ 061342a https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/54/

@silverwind
Copy link
Contributor

I've not been following this closely, but it seems #2011 or #2013 should land for the windows path fix to really be complete. cc @Fishrock123

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

I was going on this:

Either #2011 may have to land (with or without a test?) or a71ee93 may have to be reverted.

Is that not your reading @silverwind?

@silverwind
Copy link
Contributor

Yeah, that's the gist. I think the choice is between a somewhat risky landing of #2011 or #2013, or to delay the release another day until this is sorted out.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 19, 2015

What is happenning now with 2.3.1 brings up a question: why are current patch releases based on the master branch?
The usual way of doing things would be to create a branch for each minor release (including the current one) and cherry-pick fixes to that branch.

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

FYI I've opted to sit on this while we further investigate the zlib bug (wasted a chunk of my afternoon doing so!), should get a release out tomorrow, @Fishrock123 || @chrisdickinson are welcome to get this out if they feel the need when they're around before I show up.

### Notable changes

3806d87 is strictly speaking a security fix and should not be delayed,
671e64a fixes a problem that, while being a Windows-only bug, caused three bugreports in io.js to the date.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug reports

@Fishrock123
Copy link
Contributor

My computer is currently out of service so I'm afraid there's not much I can do to help. :/

Edit: got it working again for a while..

@Fishrock123
Copy link
Contributor

#2013 is a more complete fix for a71ee93 (includes a test for it)

@Fishrock123
Copy link
Contributor

Looks like we are too late on this now, we should not put this out until Monday. ((Weekend))

@rvagg
Copy link
Member Author

rvagg commented Jun 21, 2015

agree on putting it off till the end of the weekend, I'm happy to take on board the suggestion to avoid releasing just before or during a weekend to make it easier for people who work in this area as a day-job and don't want weekend drama

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

This is the Notable Changes I'm going with, critique welcome.

  • module: The number of syscalls made during a require() have been significantly reduced which should lead to a performance improvement (Pierre Inglebert) #1920, (Michaël Zasso) #1991.
  • npm:
  • zlib: A bug was discovered where the process would abort if the final part of a zlib decompression results in a buffer that would exceed the maximum length of 0x3fffffff bytes (~1GiB). This was likely to only occur during synchronous decompression. This is now fixed and will instead result in a thrown RangeError. (Michaël Zasso) #1811.

@targos could you take a look at these since they are mainly yours?

@ChALkeR
Copy link
Member

ChALkeR commented Jun 22, 2015

  • module: The number of syscalls made during a require() have been significantly reduced which should lead to a performance improvement (Pierre Inglebert) #1920, (Michaël Zasso) #1991.

#1991 is a different issue, isn't it?

@ChALkeR
Copy link
Member

ChALkeR commented Jun 22, 2015

#2013 is not merged yet, FYI.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

You're right @ChALkeR, I was mixing up require() speedups, thanks

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

#2013 is landed

CI @ https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/66/

Running smoke tests (again)

@targos
Copy link
Member

targos commented Jun 22, 2015

For the zlib bug, it is incorrect.
The synchronous version is almost unchanged (it was already throwing, there is just a different error message to match the async one).
The async version was throwing inside async code, resulting in an uncaught exception (and potentially process abort). It is now passing an error to the callback instead.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

@targos thanks! What I actually meant to refer to was "buffering" vs "streaming", not "sync" vs "async". I've been tinkering with this bug and so far I'm unable to trigger it under any streaming situation but I'd love to hear if you have different results on this.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

OK, I'm not going to move forward with this release and going to bed instead. There are crypto errors on Windows 2008 that are beyond the random timeouts we might expect. Also, only a few of them are failing with a duration of 60 seconds (the timeout length), so there's something more sinister going on here.

@shigeki could this be to do with the headers work, and/or the CNNIC whitelist cert check?

/cc @nodejs/crypto

not ok 422 - test-https-foafssl.js
  ---
  duration_ms: 60.178
not ok 675 - test-tls-alert.js
  ---
  duration_ms: 60.177
  ...
not ok 698 - test-tls-dhe.js
  ---
  duration_ms: 60.173
  ...
not ok 699 - test-tls-ecdh.js
#c:\workspace\iojs+pr+win\nodes\win2008r2\test\parallel\test-tls-ecdh.js:40
#    if (err) throw err;
#                   ^
#Error: Command failed: C:\Windows\system32\cmd.exe /s /c ""c:\workspace\iojs+pr+win\nodes\win2008r2\Release\openssl-cli.exe" s_client -cipher -ALL:ECDHE-RSA-RC4-SHA -connect 127.0.0.1:12346"
#WARNING: can't open config file: /usr/local/ssl/openssl.cnf
#Loading 'screen' into random state -
#    at ChildProcess.exithandler (child_process.js:196:12)
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at maybeClose (internal/child_process.js:764:16)
#    at Socket.<anonymous> (internal/child_process.js:319:11)
#    at emitOne (events.js:77:13)
#    at Socket.emit (events.js:169:7)
#    at Pipe._onclose (net.js:467:12)
  ---
  duration_ms: 0.496
  ...
not ok 700 - test-tls-ecdh-disable.js
#
#assert.js:89
#  throw new assert.AssertionError({
#        ^
#AssertionError: -1 != -1
#    at c:\workspace\iojs+pr+win\nodes\win2008r2\test\parallel\test-tls-ecdh-disable.js:39:12
#    at ChildProcess.exithandler (child_process.js:203:5)
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at maybeClose (internal/child_process.js:764:16)
#    at Socket.<anonymous> (internal/child_process.js:319:11)
#    at emitOne (events.js:77:13)
#    at Socket.emit (events.js:169:7)
#    at Pipe._onclose (net.js:467:12)
  ---
  duration_ms: 0.499
  ...
not ok 719 - test-tls-no-sslv3.js
#WARNING: can't open config file: /usr/local/ssl/openssl.cnf
#Loading 'screen' into random state -
#assert.js:89
#  throw new assert.AssertionError({
#        ^
#AssertionError: 3221226505 == 1
#    at ChildProcess.<anonymous> (c:\workspace\iojs+pr+win\nodes\win2008r2\test\parallel\test-tls-no-sslv3.js:34:12)
#    at ChildProcess.<anonymous> (c:\workspace\iojs+pr+win\nodes\win2008r2\test\common.js:367:15)
#    at ChildProcess.g (events.js:260:16)
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
  ---
  duration_ms: 0.499
  ...
not ok 730 - test-tls-securepair-server.js
#
#assert.js:89
#  throw new assert.AssertionError({
#        ^
#AssertionError: 1 == 0
#    at process.<anonymous> (c:\workspace\iojs+pr+win\nodes\win2008r2\test\parallel\test-tls-securepair-server.js:127:10)
#    at emitOne (events.js:82:20)
#    at process.emit (events.js:169:7)
  ---
  duration_ms: 0.496
  ...
not ok 732 - test-tls-session-cache.js
#done
#
#assert.js:89
#  throw new assert.AssertionError({
#        ^
#AssertionError: 3221226505 == 0
#    at ChildProcess.<anonymous> (c:\workspace\iojs+pr+win\nodes\win2008r2\test\parallel\test-tls-session-cache.js:92:14)
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
  ---
  duration_ms: 0.495
  ...
not ok 733 - test-tls-set-ciphers.js
#c:\workspace\iojs+pr+win\nodes\win2008r2\test\parallel\test-tls-set-ciphers.js:44
#    if (err) throw err;
#                   ^
#Error: Command failed: C:\Windows\system32\cmd.exe /s /c ""c:\workspace\iojs+pr+win\nodes\win2008r2\Release\openssl-cli.exe" s_client -cipher RC4-MD5 -connect 127.0.0.1:12346"
#WARNING: can't open config file: /usr/local/ssl/openssl.cnf
#Loading 'screen' into random state -
#    at ChildProcess.exithandler (child_process.js:196:12)
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at maybeClose (internal/child_process.js:764:16)
#    at Socket.<anonymous> (internal/child_process.js:319:11)
#    at emitOne (events.js:77:13)
#    at Socket.emit (events.js:169:7)
#    at Pipe._onclose (net.js:467:12)
  ---
  duration_ms: 0.497
  ...

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

https://jenkins-iojs.nodesource.com/job/iojs+pr+win/53/nodes=win2008r2/ is the failing machine, all other slaves are OK, including the Windows 2012 one.

Kicking off another one before I head off: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/67/

@Fishrock123
Copy link
Contributor

@rvagg hmm, new CI doesn't have those.

@rvagg
Copy link
Member Author

rvagg commented Jun 23, 2015

Let's try again then https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/68/

I'm still concerned with those failures but I have zero to go on so if anyone has any clues then please fill us in.

@Fishrock123
Copy link
Contributor

@rvagg still looks green..

@rvagg
Copy link
Member Author

rvagg commented Jun 23, 2015

smoke testing, I've been tuning my smoke test suite and it's coming along nicely!

@rvagg
Copy link
Member Author

rvagg commented Jun 23, 2015

success on most fronts, the outstanding smoke test I still can't make play nicely is npm, details here: npm/npm#8648

pushing ahead with release

rvagg added a commit that referenced this pull request Jun 23, 2015
PR-URL: #1996

Notable changes

* module: The number of syscalls made during a require() have been
  significantly reduced again (see #1801 from v2.2.0 for previous
  work), which should lead to a performance improvement
  (Pierre Inglebert) #1920.
* npm:
  - Upgrade to v2.11.2 (Rebecca Turner) #1956.
  - Upgrade to v2.11.3 (Forrest L Norvell) #2018.
* zlib: A bug was discovered where the process would abort if the
  final part of a zlib decompression results in a buffer that would
  exceed the maximum length of 0x3fffffff bytes (~1GiB). This was
  likely to only occur during buffered decompression (rather than
  streaming). This is now fixed and will instead result in a thrown
  RangeError (Michaël Zasso) #1811.
rvagg added a commit that referenced this pull request Jun 23, 2015
PR-URL: #1996

Notable changes

* module: The number of syscalls made during a require() have been
  significantly reduced again (see #1801 from v2.2.0 for previous
  work), which should lead to a performance improvement
  (Pierre Inglebert) #1920.
* npm:
  - Upgrade to v2.11.2 (Rebecca Turner) #1956.
  - Upgrade to v2.11.3 (Forrest L Norvell) #2018.
* zlib: A bug was discovered where the process would abort if the
  final part of a zlib decompression results in a buffer that would
  exceed the maximum length of 0x3fffffff bytes (~1GiB). This was
  likely to only occur during buffered decompression (rather than
  streaming). This is now fixed and will instead result in a thrown
  RangeError (Michaël Zasso) #1811.
@rvagg
Copy link
Member Author

rvagg commented Jun 23, 2015

@rvagg
Copy link
Member Author

rvagg commented Jun 23, 2015

done, https://iojs.org/dist/latest/

armv6 taking its time as usual, will promote later

@Fishrock123
Copy link
Contributor

Armv6 is up. 🍻

Starefossen pushed a commit to nodejs/docker-iojs that referenced this pull request Jun 24, 2015
PR-URL #70
Related: nodejs/node#1996

Signed-off-by: Hans Kristian Flaatten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants