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

src: add NODE_VERSION_IS_LTS to node_version.h #16697

Merged
merged 1 commit into from
Nov 20, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Nov 2, 2017

This is defined in LTS releases, but should really be defined in master too.

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

cc/ @nodejs/lts

@jasnell
Copy link
Member

jasnell commented Nov 2, 2017

FWIW, when these were first introduced, the decision was to not have them in master at all. For the codename, the idea was that if the macro is missing, the value of the process property is undefined rather than an empty string, so someone would be able to determine if a release is LTS simply by checking for the presence of the property (without checking the value).

@gibfahn
Copy link
Member Author

gibfahn commented Nov 2, 2017

Hmm, okay, so that's to make things easier for core collaborators? I could be wrong, but I don't see how that makes sense.

If you don't define in master, you can do #ifdef NODE_VERSION_IS_LTS to check if it's defined, but if you define it to 0 or 1, you can just do #if NODE_VERSION_IS_LTS, which is true if it's 1, and false if it's 0.

In fact AFAICT NODE_VERSION_IS_LTS is only used in one place in the source, and that already uses #if not #ifdef:

node/src/node.cc

Lines 3172 to 3175 in 3a977fc

#if NODE_VERSION_IS_LTS
READONLY_PROPERTY(release, "lts",
OneByteString(env->isolate(), NODE_VERSION_LTS_CODENAME));
#endif

@gibfahn gibfahn added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 2, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

As a separate commit or separate PR, could you add a note to doc/releases.md about these values. That document already talks about updating the other values in this file.

@gibfahn
Copy link
Member Author

gibfahn commented Nov 2, 2017

As a separate commit or separate PR, could you add a note to doc/releases.md about these values. That document already talks about updating the other values in this file.

Yep, I'm working on an update to include all the stuff that isn't in that doc that you need to do for a first LTS release, including this!

@jasnell
Copy link
Member

jasnell commented Nov 3, 2017

heh, I didn't argue that it still makes sense ;-) I just explained the history ;-) ... adding these is fine I think

@rvagg
Copy link
Member

rvagg commented Nov 4, 2017

I'm not fussed either way but I think I can confirm that this isn't going to break anything if introduced. The only other place I'm aware of it being used is in dist-indexer which makes index.json and index.tab in the download directories and that'll still work fine after this.

iirc pulling in release commits back to master can be a pain when you have to reset src/node_version.h. I seem to remember occasionally having trouble with these missing lines. I'm not sure this will make much of a difference; although it could introduce the possibility of accidentally turning it to 1 and not noticing because a switch from 0 to 1 in a commit is much easier to miss than adding a whole new line or two.

@gibfahn
Copy link
Member Author

gibfahn commented Nov 13, 2017

This is defined in LTS releases, but should really be defined in master
too.

PR-URL: nodejs#16697
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn gibfahn merged commit e51216d into nodejs:master Nov 20, 2017
@gibfahn gibfahn deleted the node_version-lts branch November 20, 2017 14:34
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This is defined in LTS releases, but should really be defined in master
too.

PR-URL: #16697
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants