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

v5.8.0 proposal #5559

Merged
merged 2 commits into from
Mar 9, 2016
Merged

v5.8.0 proposal #5559

merged 2 commits into from
Mar 9, 2016

Conversation

MylesBorins
Copy link
Contributor

2016-03-08, Version 5.8.0 (Stable). @Fishrock123

Notable changes

  • http_parser:
    • revert d77c3bf which was causing errors inside of http client callbacks to not propagate.
  • path:
    • a fix to a regression found in path.resolve for absolute paths with a single character name as the root directory (Evan Lucas) #5589

@MylesBorins
Copy link
Contributor Author

@rvagg @jasnell @trevnorris @Fishrock123

It was discovered in #5555 that d77c3bf has introduced a pretty nasty regression... specifically that calling to Throw in a Http client callback is not Throwing.

My notes on this can be found --> https://gist.github.com/TheAlphaNerd/6615a27684deb682dfe7

@chrisdickinson was speculating that a fix will likely involve the code found --> #4507

If there is a quick fix that we can get out then we should definitely close this proposal... but if we can't get it out ASAP I think we should move forward with the reversion on v5.x and this release. The regression came in the same release as the fix to the latest openssl bugs, and as such we currently do not have a release on the v5 line that is secure and working.

@mscdex mscdex added meta Issues and PRs related to the general management of the project. v5.x labels Mar 4, 2016
@MylesBorins
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1843/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/101/

edit: Citgm failures are either infra or expected

@trevnorris
Copy link
Contributor

I don't currently know of a quick fix for this. @indutny have an idea?

@jbergstroem
Copy link
Member

Can we please do this properly and land it upstream first?

@MylesBorins
Copy link
Contributor Author

@jbergstroem the idea here was to hotfix this in the v5.x branch so that we can have a working release while not reverting the change on master giving it time to be fixed properly

@jbergstroem
Copy link
Member

(if this is actually an issue upstream, sorry - changes looks related to how we use it)

edit: sorry, not relevant upstream indeed.

@indutny
Copy link
Member

indutny commented Mar 4, 2016

@trevnorris neither do I right now, will take a deeper look.

@evanlucas
Copy link
Contributor

+1 to reverting from v5.x and getting a quick release out.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 7, 2016

#5585 is also a new path-related regression in 5.7 branch.

@Fishrock123
Copy link
Contributor

@rvagg mentioned to me that him and @jbergstroem were reworking the release CI but didn't get it done yesterday, so it might not work for now.

Let's just do a regular release tomorrow; I'm willing to pick it up and do it if you'd like.

@Fishrock123 Fishrock123 modified the milestone: 5.7.2 Mar 7, 2016
@evanlucas
Copy link
Contributor

#5589 is a fix for #5585 that should go in here

@MylesBorins MylesBorins force-pushed the v5.7.2-proposal branch 3 times, most recently from da768c7 to 5de263e Compare March 7, 2016 21:13
@trevnorris
Copy link
Contributor

@thealphanerd fix for http regression: #5591

@jbergstroem
Copy link
Member

@Fishrock123 the new ci should be done by today; already doing test builds.

@Fishrock123
Copy link
Contributor

Here's a list of proposal commits (only waiting for #5591):

branch-diff v5.x master --exclude-label=semver-major,semver-minor,dont-land-on-v5.x --filter-release

  • [735e0df8e4] - benchmark: add util.format benchmark (Evan Lucas) #5360
  • [1bedeeb41d] - benchmark: fix lint errors (Rich Trott) #5517
  • [44c9751ff2] - build: update Node.js logo on Win installer (Robert Jefe Lindstaedt) #5531
  • [17924703d6] - build: correctly detect clang version (Stefan Budeanu) #5553
  • [079973b96a] - deps: add V8 DEP trace_event (Ali Ijaz Sheikh) #4722
  • [89f234300a] - deps: edit v8 gitignore to allow trace_event copy (Ali Ijaz Sheikh) #4722
  • [069e02ab47] - deps: upgrade to V8 4.9.385.18 (Ali Ijaz Sheikh) #4722
  • [8af4bb86c0] - dgram: default send address to 127.0.0.1 or ::1 (Matteo Collina) #5493
  • [96af4741cd] - doc: document directories in test directory (Michael Barrett) #5557
  • [1d9a2b28bb] - doc: add info to docs on how to submit docs patch (Sequoia McDowell) #4591
  • [449684752c] - doc: fix typo in fs.symlink (Michaël Zasso) #5560
  • [831b30eeb7] - doc: improve unhandledException doc copy (James M Snell) #5287
  • [7ae3dfb153] - doc: update link green to match homepage (silverwind) #5548
  • [4c724dd439] - doc: update V8 URL (Craig Akimoto) #5530
  • [953173b0d7] - lib: wrap tick_processor scripts in IIFE (Ali Ijaz Sheikh) #4722
  • [3d3b45ae95] - path: fix normalize for absolutes (Evan Lucas) #5589
  • [ad0ce57432] - src: fix NewInstance deprecation warnings (Michaël Zasso) #4722
  • [67b5a8a936] - src: replace usage of deprecated ForceSet (Michaël Zasso) #5159
  • [cddd5347f6] - src: replace deprecated ProcessDebugMessages (Michaël Zasso) #5159
  • [d515a3f4b4] - src: replace usage of deprecated GetDebugContext (Michaël Zasso) #5159
  • [4417f99432] - src: replace usage of deprecated SetMessageHandler (Michaël Zasso) #5159
  • [8894f2745f] - src: replace deprecated CancelTerminateExecution (Michaël Zasso) #5159
  • [f50ef1a46f] - src: replace deprecated TerminateExecution (Michaël Zasso) #5159
  • [ac32340997] - src: replace usage of deprecated HasOwnProperty (Michaël Zasso) #5159
  • [a729208808] - src: replace usage of deprecated Has (Michaël Zasso) #5159
  • [3d7fd9aa22] - src: replace usage of deprecated GetEndColumn (Michaël Zasso) #5159
  • [d45045f318] - src: replace usage of deprecated Delete (Michaël Zasso) #5159
  • [16b0a8c1ac] - src: replace usage of deprecated CompileUnbound (Michaël Zasso) #5159
  • [023c317173] - src: replace usage of deprecated Compile (Michaël Zasso) #5159
  • [79d7475604] - src: fix TryCatch deprecation warnings (Michaël Zasso) #4722
  • [81f1732a05] - src: replace deprecated String::NewFromOneByte (Michaël Zasso) #4722
  • [061ebb39c9] - test: add test-npm-install to parallel tests suite (Myles Borins) #5166
  • [eb3f04d81d] - test: remove broken debugger scenarios (Rich Trott) #5532
  • [e38e2a6012] - test: bug repro for vm function redefinition (cjihrig) #5528
  • [01f82f0685] - test: fix proxy tab-completion test (Ali Ijaz Sheikh) #4722
  • [9968941797] - test: fix tests after V8 upgrade (Michaël Zasso) #4722
  • [5d7265f601] - test: prevent flakey test on pi2 (Trevor Norris) #5537
  • [7d3a7ea0d7] - test: check memoryUsage properties (Wyatt Preul) #5546
  • [3eff42bbca] - tools: reduce verbosity of cpplint (Sakthipriyan Vairamani) #5578
  • [810aa9f6c3] - tools: enable no-self-assign ESLint rule (Rich Trott) #5552
  • [32e1f9d0b5] - tools: support testing known issues (cjihrig) #5528
  • [6d22003757] - tools: enable linting for benchmarks (Rich Trott) #5517
  • [66ea32d6d1] - tools: enable no-extra-parens in ESLint (Rich Trott) #5512
  • [c490b8ba54] - util: improve format() performance further (Brian White) #5360
  • [8d72b0d291] - util: improve util.format performance (Evan Lucas) #5360

@MylesBorins
Copy link
Contributor Author

@Fishrock123 would you like to take over this release? Initially this was going for a quick knee jerk release last week, but since we've delayed until tuesday it would make sense to me for you to take over

@rvagg
Copy link
Member

rvagg commented Mar 7, 2016

looks like we might be missing some dont-land-on-v5.x for V8 commits:

[079973b] - deps: add V8 DEP trace_event (Ali Ijaz Sheikh) #4722
[89f2343] - deps: edit v8 gitignore to allow trace_event copy (Ali Ijaz Sheikh) #4722
[069e02a] - deps: upgrade to V8 4.9.385.18 (Ali Ijaz Sheikh) #4722

/cc @ofrobots

the rest look good to me

@MylesBorins
Copy link
Contributor Author

@Fishrock123 we likely should include the latest npm fix as well

edit: I just landed this

@Fishrock123
Copy link
Contributor

Yeah I'll take it over for tomorrow (pending the http bugfix), no worries.

@ofrobots
Copy link
Contributor

ofrobots commented Mar 8, 2016

079973b, 89f2343 and 069e02a should NOT be landed on v5.x along with 01f82f0 and 9968941. If it makes things simpler you may want to skip everything from #4722.

@Fishrock123
Copy link
Contributor

@ofrobots cool, thanks. If you tag it with the dont-land-on-* labels our tooling will automatically skip it. :)

@ronkorving
Copy link
Contributor

Does the V8 bump to 4.9 not mean we bump minor?

@rvagg
Copy link
Member

rvagg commented Mar 8, 2016

there is no v8 bump, we're committed to keeping v8 stable on v5, that should change in v6 but I think we need to do some documentation work on this cause it mostly exists in people's heads

Fishrock123 added a commit that referenced this pull request Mar 8, 2016
Notable changes

* path:
  * a fix to a regression found in path.resolve for absolute paths with a single character name as the root directory (Evan Lucas) #5589

PR-URL: #5559
@Fishrock123 Fishrock123 changed the title V5.7.2 proposal v5.8.0 proposal Mar 8, 2016
@Fishrock123
Copy link
Contributor

@rvagg
Copy link
Member

rvagg commented Mar 8, 2016

osx should be happier now if you want to run CI again

@Fishrock123
Copy link
Contributor

Sure, why not. Just finished the changelog, will also be making release builds soon.

CI: https://ci.nodejs.org/job/node-test-commit/2493/

Notable changes:

* child_process: “send()” now accepts an options parameter (cjihrig)
#5283
  - Currently the only option is “keepOpen”, which keeps the underlying
socket open after the message is sent.
* constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts)
#5463
* Fixed two regressions which originated in v5.7.0:
  - http: Errors inside of http client callbacks now propagate
correctly (Trevor Norris) #5591
  - path: Fixed normalization of absolute paths (Evan Lucas)
#5589
* repl: “start()” no longer requires an options parameter (cjihrig)
#5388
* util: Improved “format()” performance 50-300% (Evan Lucas)
#5360

PR-URL: #5559
@Fishrock123
Copy link
Contributor

@rvagg
Copy link
Member

rvagg commented Mar 9, 2016

@jbergstroem check out @Fishrock123's release build attempt @ https://ci-release.nodejs.org/job/iojs+release/461/ - it's assigned to @thealphanerd doing v5 which actually occured @ https://ci-release.nodejs.org/job/iojs+release/462/, got any clue how this might get mixed up?

@MylesBorins
Copy link
Contributor Author

@rvagg 461 and 462 are v4.4.0 builds. it looks like @Fishrock123's build is 463

@rvagg
Copy link
Member

rvagg commented Mar 9, 2016

@thealphanerd right, but @Fishrock123 swears that he started 461 4 hours ago but now there's no evidence of his name or a v5 build anywhere until his 453 which was only started a little before my comment.

@MylesBorins
Copy link
Contributor Author

Here are the parameters, and the commit is definitely the one from v4.4.0

Perhaps by some odd co-incidence we started our jobs at the same time?

I did start a second job 462... so everything seems to add up on my end

@Fishrock123
Copy link
Contributor

Weird, at least the new build started.

I'm going to blame this on some Jenkins failure. Here's what I think happened:

  1. I put everything in and hit 'build' - I will note it took a minute.
  2. It changed pages without actually starting it at the same time as Myles started his.
  3. I thought his was mine because Jenkins appeared to accept mine and that was started within the timeframe as the only running CI.

@Fishrock123 Fishrock123 merged commit 8165570 into v5.x Mar 9, 2016
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 9, 2016
Notable changes:

* child_process: “send()” now accepts an options parameter (cjihrig)
nodejs#5283
- Currently the only option is “keepOpen”, which keeps the underlying
socket open after the message is sent.
* constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts)
nodejs#5463
* Fixed two regressions which originated in v5.7.0:
  - http: Errors inside of http client callbacks now propagate
correctly (Trevor Norris) nodejs#5591
  - path: Fixed normalization of absolute paths (Evan Lucas)
nodejs#5589
* repl: “start()” no longer requires an options parameter (cjihrig)
nodejs#5388
* util: Improved “format()” performance 50-300% (Evan Lucas)
nodejs#5360

PR-URL: nodejs#5559
Fishrock123 added a commit that referenced this pull request Mar 9, 2016
Fishrock123 added a commit to nodejs/nodejs.org that referenced this pull request Mar 9, 2016
@Fishrock123
Copy link
Contributor

All done!

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.