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

v4.2.6 proposal #4788

Merged
merged 4 commits into from
Jan 21, 2016
Merged

v4.2.6 proposal #4788

merged 4 commits into from
Jan 21, 2016

Conversation

MylesBorins
Copy link
Contributor

Notable changes

  • Fix regression in debugger and profiler functionality

Known issues

  • Some problems with unreferenced timers running during beforeExit are still to be resolved. See #1264.
  • Surrogate pair in REPL can freeze terminal. #690
  • Calling dns.setServers() while a DNS query is in progress can cause the process to crash on a failed assertion. #894
  • url.resolve may transfer the auth portion of the url when resolving between two full hosts, see #1435.

Commits

  • [1408f7abb1] - module,src: do not wrap modules with -1 lineOffset (cjihrig) #4298
  • [1f8e1472cc] - test: add test for debugging one line files (cjihrig) #4298

In b799a74 and
dfee4e3 the module wrapping
mechanism was changed for better error reporting. However,
the changes causes issues with debuggers and profilers. This
commit reverts the wrapping changes.

Fixes: #4297
PR-URL: #4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
This commit adds a regression test for debugging of
single line files.

Refs: #4297
PR-URL: #4298
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
Copy link
Contributor Author

/cc @jasnell @rvagg @ofrobots @cjihrig

@MylesBorins
Copy link
Contributor Author

@MylesBorins MylesBorins changed the title V4.2.6 proposal v4.2.6 proposal Jan 21, 2016
@Fishrock123 Fishrock123 added the meta Issues and PRs related to the general management of the project. label Jan 21, 2016
@ofrobots
Copy link
Contributor

LGTM. Thanks for putting this together so fast 🚀!

@rvagg
Copy link
Member

rvagg commented Jan 21, 2016

@ofrobots would you mind explaining the urgency of this here to justify the quick double-release? We can't afford to hand-wave on something like this.

@MylesBorins
Copy link
Contributor Author

Seems to be a failure on BSD in CI... re running

https://ci.nodejs.org/job/node-test-commit-freebsd/1011/

edit: BSD CI is green

@ofrobots
Copy link
Contributor

@rvagg Not being able to debug or profile applications is a pretty big regression, in my opinion. You don't want people to deploy with 4.2.5 in production only to realize later, when they suddenly need to, that debugging and profiling do not work. I agree that 4.2.5 is not DOA.

@rvagg
Copy link
Member

rvagg commented Jan 21, 2016

Not being able to debug or profile applications

That's all I was after, just some justification for this we can point to. I think most of the discussion about this is tucked away in IRC, best to have it explicit on here.

@MylesBorins
Copy link
Contributor Author

Rerunning citgm (lodash not set to flaky): https://ci.nodejs.org/job/thealphanerd-smoker/48/

Rerunning tests on windows fanned that failed: https://ci.nodejs.org/job/node-test-commit-windows-fanned/1059/

Looked like system related problems in first run

edit: windows passes

@MylesBorins
Copy link
Contributor Author

CITGM Passes all platforms but ppcle, with an expected failure of eslint and an unexpected failure of watchify

verbose: npm-test:           | not ok 43 should be equal
verbose: npm-test:           | ---                                               
verbose:                     | operator: equal                                   
verbose:                     | expected: 'BEEP BOOP robot\n'                     
verbose:                     | actual:   'BEEP boop robot\n'                     
verbose:                     | at: Socket.<anonymous>                            
verbose:                     | (/tmp/a028dc09-5ee5-487a-ba92-92c38905d20e/watchi���
verbose:                     | fy/test/bin_ignore_watch_default.js:67:9)   

@MylesBorins
Copy link
Contributor Author

one more CI for posterity: https://ci.nodejs.org/job/node-test-pull-request/1344/

@jasnell
Copy link
Member

jasnell commented Jan 21, 2016

This LGTM.

Once we get this one spun out (as soon as possible today hopefully) we need to take a look at the process a bit. In order to prevent this from happening again, we need to make sure that everyone is appropriately applying the lts-watch labels when PRs are either opened or landed. There are so many commits that it's difficult to keep track of which ones need to be pulled back and which do not. In the case where a single set of changes and fixes are spread over multiple PRs and commits, it becomes even more important to track. I do see that I was mentioned in the discussion thread for #4298 but I'm a bit backlogged on my github notifications and missed it. Likely the best workflow is to go ahead and mark a PR with lts-watch even if it's not clear that it should land. That will at least put it on the radar when a new LTS release is being prepared. Ideally, the lts-watch would be applied either by the person opening the PR or the person landing the PR if it hasn't already been applied.

Another point ... I think it would be helpful if we had a more robust/formal way of linking to dependent commits included as part of the Commit Metadata. Perhaps adding something like Depends-On: https://github.com/nodejs/node/pull/9999 along side the PR-URL, etc. This would allow those of us backporting commits to more easily and reliably follow the trail of certain commits. I know it can be difficult to know exactly which past commits a new commit is dependent on, but in the case of #4298 it's pretty straightforward.

@MylesBorins
Copy link
Contributor Author

only failure on CI this round was test-http-exit-delay which there is currently a proposal to remove

#4786

I think it is safe to call that a good CI run... still investigating the citgm failure on ppc.

@MylesBorins
Copy link
Contributor Author

Rerunning citgm just on ppc for watchify

https://ci.nodejs.org/job/thealphanerd-smoker-ppc/1/

Passes

Notable changes:

* Fix regression in debugger and profiler functionality

PR-URL: #4788
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
Copy link
Contributor Author

Started release CI job: https://ci.nodejs.org/job/iojs+release/364/

@MylesBorins MylesBorins merged commit 1c4ea61 into v4.x Jan 21, 2016
@brycebaril
Copy link
Contributor

Probably too late due to already being in 4.2.5 as well but 0ae90ec probably should have been a Semver minor (Adds deprecation message)

@rvagg rvagg deleted the v4.2.6-proposal branch January 21, 2016 22:16
MylesBorins pushed a commit that referenced this pull request Jan 21, 2016
Notable changes:

* Fix regression in debugger and profiler functionality

PR-URL: #4788
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
pesho added a commit to pesho/docker-node that referenced this pull request Jan 22, 2016
pesho added a commit to pesho/docker-official-images that referenced this pull request Jan 22, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Notable changes:

* Fix regression in debugger and profiler functionality

PR-URL: nodejs#4788
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
RichardScothern pushed a commit to RichardScothern/official-images that referenced this pull request Jun 14, 2016
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.

7 participants