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: process release lts property #16656

Merged
merged 2 commits into from
Nov 2, 2017
Merged

src: process release lts property #16656

merged 2 commits into from
Nov 2, 2017

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Oct 31, 2017 •
edited
Loading

This is a forward-port of 455272a. It is required for LTS release branches to have the correct in-process information when configured with NODE_VERSION_IS_LTS and NODE_VERSION_LTS_CODENAME. It has no impact otherwise, other than a test which already covers up multiple possible name paths.

This should be backported to 8.x asap as it is missing there and as such process.release.lts is undefined in 8.9.0. Myself and @MylesBorins are hoping to get this into the security release later this week (from irc).

This lands cleanly on 8.x.

cc also @nodejs/release

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test, src, lts

CI: https://ci.nodejs.org/job/node-test-pull-request/11121/

@Fishrock123 Fishrock123 added lib / src Issues and PRs related to general changes in the lib or src directory. lts Issues and PRs related to Long Term Support releases. labels Oct 31, 2017
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 31, 2017
@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Oct 31, 2017 •
edited
Loading

(Reminding myself, the first commit has configurable misspelt.)

MylesBorins
MylesBorins approved these changes Oct 31, 2017
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

Were we going to include updates to node_version.h to make sure the LTS related flags are in there?

doc/api/process.md Outdated
@@ -1437,8 +1437,12 @@ tarball.
architecture and version of the current release. This file is used for
compiling Node.js native add-ons. _This property is only present on Windows
builds of Node.js and will be missing on all other platforms._
* `lts` {string} a string label identifying the [LTS][] label for this release.
If the Node.js release is not an LTS release, this will be `undefined`.
* `lts` {string} a string label identifying the [LTS][] label for this release.
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an indent change here or is just github rendering weirdly?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, looks like an indent.

@gibfahn gibfahn mentioned this pull request Nov 2, 2017
Closed
rvagg and others added 2 commits November 2, 2017 09:18
@rvagg @Fishrock123
src: add 'dynamic' process.release.lts property
905cd43 
This makes the process.release.lts property configurable by a constant.

This ref is the original PR to v6.x.
Refs: nodejs#3212

 Conflicts:
        doc/api/process.md

PR-URL: nodejs#16656
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@Fishrock123
test: update process-release for Node 8 Carbon
d4471e0 
PR-URL: nodejs#16656
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@Fishrock123 Fishrock123 merged commit d4471e0 into nodejs:master Nov 2, 2017
@Fishrock123 Fishrock123 deleted the process-release-lts-property branch November 2, 2017 16:27
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Nov 2, 2017
@rvagg @Fishrock123
src: add 'dynamic' process.release.lts property
bf26b96 
This makes the process.release.lts property configurable by a constant.

This ref is the original PR to v6.x.
Refs: nodejs#3212

 Conflicts:
        doc/api/process.md

PR-URL: nodejs#16656
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Nov 2, 2017
@Fishrock123
test: update process-release for Node 8 Carbon
dfac6cc 
PR-URL: nodejs#16656
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@Fishrock123
Copy link
Contributor Author

Thanks. landed also on v8.x-staging as bf26b96 and dfac6cc.

I will follow up with a PR that adds default properties to node_version.h.

@gibfahn gibfahn mentioned this pull request Nov 5, 2017
Merged
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@rvagg @cjihrig
src: add 'dynamic' process.release.lts property
c40b3c6 
This makes the process.release.lts property configurable by a constant.

This ref is the original PR to v6.x.
Refs: nodejs#3212

 Conflicts:
        doc/api/process.md

PR-URL: nodejs#16656
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@Fishrock123 @cjihrig
test: update process-release for Node 8 Carbon
d178c6d 
PR-URL: nodejs#16656
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. lts Issues and PRs related to Long Term Support releases.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Fishrock123 @apapirovski @MylesBorins @cjihrig @gireeshpunathil @gibfahn @nodejs-github-bot @rvagg