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

fix: make process.uptime() return the correct time #19436

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@DeerMichel
Copy link
Member

commented Jul 24, 2019

Description of Change

Fixes #19425.

The issue was rooted in an upstream change: nodejs/node#26016 (found its way to electron in #17752). Previously, the process-relative uptime was set in a method which got called by both node::Start (used by nodejs) and node::Init (used by embedders, deprecated) init methods. After the change, it is set directly in node::Start and thereby not when using node::Init. This PR just reverts that.

Since i don't see any difference in either way of setting the variable, we might wanna upstream this patch so other embedders can benefit from it as well.

cc @MarshallOfSound @codebytere

Checklist

Release Notes

Notes: Fixed process.uptime() returning the wrong time.

@DeerMichel DeerMichel requested a review from electron/wg-upgrades as a code owner Jul 24, 2019

@nornagon

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

@DeerMichel before we merge this, do you want to try submitting a PR to nodejs/node to see if anyone from the node maintainer group has thoughts about it?

@DeerMichel

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Yep, sounds good to me :) --> nodejs/node#28849

@DeerMichel DeerMichel referenced this pull request Jul 24, 2019
2 of 2 tasks complete

@electron-cation electron-cation bot removed the new-pr 🌱 label Jul 25, 2019

@DeerMichel

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@nornagon my node PR got merged 🎉. So can we keep the patch here until we reached the current upstream version?

@nornagon
Copy link
Member

left a comment

yup sgtm

@codebytere

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@DeerMichel needs rebase ☝️

@DeerMichel DeerMichel force-pushed the intern/process-uptime branch from 74cbc05 to 7013160 Jul 29, 2019

@DeerMichel

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

done

@MarshallOfSound
Copy link
Member

left a comment

Also now that this is fixed we should re-enable the test/parallel/test-process-uptime node test

@DeerMichel DeerMichel force-pushed the intern/process-uptime branch 2 times, most recently from ac60f36 to ebfefbf Jul 29, 2019

Micha Hanselmann

@DeerMichel DeerMichel force-pushed the intern/process-uptime branch from ebfefbf to 09691d1 Jul 29, 2019

@DeerMichel DeerMichel requested a review from MarshallOfSound Jul 30, 2019

@jkleinsc jkleinsc merged commit 6e367da into master Jul 30, 2019

13 of 15 checks passed

Backportable? - 5-0-x Backport Failed
Details
Backportable? - 6-0-x Backport Failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190729.41 succeeded
Details
electron-arm64-testing Build #20190729.41 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jul 30, 2019

Release Notes Persisted

Fixed process.uptime() returning the wrong time.

@jkleinsc jkleinsc deleted the intern/process-uptime branch Jul 30, 2019

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

I was unable to backport this PR to "5-0-x" cleanly;
you will need to perform this backport manually.

@trop

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/5-0-x label Jul 30, 2019

@codebytere

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

@DeerMichel could you please open manual backports for this to 5/6?

@DeerMichel

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

oh missed this, sure...

@trop

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

A maintainer has manually backported this PR to "5-0-x", please check out #19566

@trop

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #19567

@trop trop bot added in-flight/6-0-x and removed needs-manual-bp/6-0-x labels Aug 1, 2019

DeerMichel pushed a commit that referenced this pull request Aug 1, 2019
MarshallOfSound added a commit that referenced this pull request Aug 1, 2019

@trop trop bot added merged/6-0-x and removed in-flight/6-0-x labels Aug 1, 2019

@sofianguy sofianguy added this to Fixed in 6.0.1 in 6.0.x Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.