8000 src: process release lts property by Fishrock123 · Pull Request #16656 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    src: process release lts property #16656

    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
    Merged
    Fishrock123 merged 2 commits into from
    Nov 2, 2017
    Merged

    src: process release lts property #16656

    Fishrock123 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
    Copy link
    Contributor
    @MylesBorins MylesBorins Oct 31, 2017

    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
    @gibfahn gibfahn Nov 1, 2017

    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.

    @Fishrock123 Fishrock123 force-pushed the process-release-lts-property branch from a84754a to a808d96 Compare November 1, 2017 18:24
    @Fishrock123 Fishrock123 force-pushed the process-release-lts-property branch from a808d96 to 10fbcef Compare November 1, 2017 18:26
    @gibfahn gibfahn mentioned this pull request Nov 2, 2017
    Closed
    @Fishrock123 Fishrock123 force-pushed the process-release-lts-property branch from 10fbcef to 7f73d81 Compare November 2, 2017 16:14
    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 <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    @Fishrock123
    test: update process-release for Node 8 Carbon
    d4471e0 
    PR-URL: nodejs#16656
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    @Fishrock123 Fishrock123 force-pushed the process-release-lts-property branch from 7f73d81 to d4471e0 Compare November 2, 2017 16:26
    @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 <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    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 <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    @Fishrock123
    Copy link
    Contributor Author
    Fishrock123 commented Nov 2, 2017

    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 <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    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 <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
    @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

    @cjihrig cjihrig cjihrig approved these changes

    @gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

    +3 more reviewers

    @gibfahn gibfahn gibfahn left review comments

    @apapirovski apapirovski apapirovski approved these changes

    @MylesBorins MylesBorins MylesBorins approved these changes

    Reviewers whose approvals may not affect merge requirements

    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
    0