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.

Sign up for GitHub

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

Jump to bottom

src: remove defunct timer_wrap file #21777

Closed
wants to merge 1 commit into from
Closed

src: remove defunct timer_wrap file #21777

wants to merge 1 commit into from

Conversation

maclover7
Copy link
Contributor

Unused since the excellent refactoring in 2930bd1,
which also removed src/timer_wrap.cc from node.gyp. If you try and
get at the binding on a v11.0-pre build, you'll get an error, since the
file is no longer in the GYP build. I believe this file can be removed now, but please let me know if I missed something :)

Jonathans-MBP:node jon$ ./node -v
v11.0.0-pre
Jonathans-MBP:node jon$ ./node -e "process.binding('timer_wrap')"
internal/bootstrap/loaders.js:81
        mod = bindingObj[module] = getBinding(module);
                                   ^

Error: No such module: timer_wrap
    at process.binding (internal/bootstrap/loaders.js:81:36)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:562:27)
    at startup (internal/bootstrap/node.js:248:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)

cc @apapirovski

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@maclover7 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/256/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jul 12, 2018
@maclover7
Copy link
Contributor Author

(Tagging as semver-major since that's what the original PR was)

@maclover7 maclover7 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 12, 2018
@addaleax addaleax added dont-land-on-v8.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 12, 2018
addaleax
addaleax approved these changes Jul 12, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Yes, if it’s not in node.gyp it can be removed :)

And it doesn’t have to be semver-major. We usually call those commits out explicitly in the change log as API changes, but this PR doesn’t really affect our visible API behaviour.

@maclover7
src: remove defunct timer_wrap file
8658963 
Unused since the excellent refactoring in 2930bd1,
which also removed `src/timer_wrap.cc` from `node.gyp`. If you try and
get at the binding on a v11.0-pre build, you'll get an error, since the
file is no longer in the GYP build.

```
Jonathans-MBP:node jon$ ./node -v
v11.0.0-pre
Jonathans-MBP:node jon$ ./node -e "process.binding('timer_wrap')"
internal/bootstrap/loaders.js:81
        mod = bindingObj[module] = getBinding(module);
                                   ^

Error: No such module: timer_wrap
    at process.binding (internal/bootstrap/loaders.js:81:36)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:562:27)
    at startup (internal/bootstrap/node.js:248:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
```

Refs: nodejs#20894
@maclover7
Copy link
Contributor Author

maclover7 commented Jul 14, 2018 •
edited
Loading

Rebased, CI: https://ci.nodejs.org/job/node-test-pull-request/15879/

EDIT by @maclover7: OSX failure is known flake (#21724), rebuilding Windows due to infrastructure failure: https://ci.nodejs.org/job/node-test-commit-windows-fanned/19306/

EDIT 2 by @maclover7: "Rebuilt" node-test-commit at https://ci.nodejs.org/job/node-test-commit/19804/ is all green, landing

@maclover7
Copy link
Contributor Author

Landed in 2faab11

@maclover7 maclover7 closed this Jul 15, 2018
@maclover7 maclover7 deleted the jm-rm-timerwrap branch July 15, 2018 13:31
maclover7 added a commit that referenced this pull request Jul 15, 2018
@maclover7
src: remove defunct timer_wrap file
2faab11 
Unused since the excellent refactoring in 2930bd,
which also removed `src/timer_wrap.cc` from `node.gyp`. If you try and
get at the binding on a v11.0-pre build, you'll get an error, since the
file is no longer in the GYP build.

```
Jonathans-MBP:node jon$ ./node -v
v11.0.0-pre
Jonathans-MBP:node jon$ ./node -e "process.binding('timer_wrap')"
internal/bootstrap/loaders.js:81
        mod = bindingObj[module] = getBinding(module);
                                   ^

Error: No such module: timer_wrap
    at process.binding (internal/bootstrap/loaders.js:81:36)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:562:27)
    at startup (internal/bootstrap/node.js:248:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
```

PR-URL: #21777
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #20894
@apapirovski
Copy link
Member

Thanks @maclover7. If anyone's curious: what happened here is that git lost track of the fact that timers.cc and timer_wrap.cc were the same file because they completely diverged after the removal of the license header. I missed that when landing.

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

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@maclover7 @nodejs-github-bot @apapirovski @jasnell @addaleax @lpinca @TimothyGu @cjihrig @devsnek @BridgeAR