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: Provide full and correct version in `process.version` #11103

Merged
merged 7 commits into from Nov 15, 2017

Conversation

Projects
None yet
4 participants
@felixrieseberg
Member

felixrieseberg commented Nov 13, 2017

• Outputs the full and correct version from process.version
• Updates the bump_version.py script to not break it in the future

Closes #11101

screen shot 2017-11-12 at 10 29 26 pm

@felixrieseberg felixrieseberg requested a review from electron/reviewers as a code owner Nov 13, 2017

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke
Member

zeke commented Nov 13, 2017

@ckerr

I agree with the assessment of the problems and the goals of this patch, but have a couple of questions --

  1. The semver spec is pretty clear this part of the name is called the 'pre-release version', perhaps we should reflect that here by using ATOM_PRE_RELEASE_VERSION instead of ATOM_LABEL_VERSION

  2. Since this will be commented out for non-prerelease releases, we ought to have an #ifndef fallback just as we do with ATOM_TAG

  3. Related but perhaps out-of-scope for this patch, I'm not sure what role ATOM_TAG plays in this, it doesn't seem to be related to anything in semver. Is this a leftover from the old versioning scheme? If so, can we remove it safely?

  4. Also related but perhaps out-of-scope for this patch, it looks like non-releases would have a name something like 1.8.2-beta1-pre which seems nonsensical. Does -pre have some magic meaning that we're relying on in scripts somewhere?

@vanessayuenn

This comment has been minimized.

Show comment
Hide comment
@vanessayuenn

vanessayuenn Nov 13, 2017

Contributor

Agreed with @ckerr on point #1 and #2. @zcbenz might be able to shed light on #3 and #4.

I also think this is a necessary patch but the change might break things for downstream users if they depend on the version number always being x.x.x. We'll want to make sure to communicate that clearly.

Contributor

vanessayuenn commented Nov 13, 2017

Agreed with @ckerr on point #1 and #2. @zcbenz might be able to shed light on #3 and #4.

I also think this is a necessary patch but the change might break things for downstream users if they depend on the version number always being x.x.x. We'll want to make sure to communicate that clearly.

@felixrieseberg

This comment has been minimized.

Show comment
Hide comment
@felixrieseberg

felixrieseberg Nov 13, 2017

Member

@ckerr @vanessayuenn Good points!

  1. I updated the name of the variable.

  2. I added a fallback

  3. I don't think ATOM_TAG was ever used for anything public, but I could be wrong. It's certainly not used in bump_version.py, so if it turns out that we no longer need it, I'm happy to remove it.

  4. As for pre, I greped the whole codebase and found nothing except for a test in node_spec.js.

As for dealing with downstream fallout, I believe we can rest easy - this change would only affect beta versions, while production versions will continue to report a "simple" version.

Member

felixrieseberg commented Nov 13, 2017

@ckerr @vanessayuenn Good points!

  1. I updated the name of the variable.

  2. I added a fallback

  3. I don't think ATOM_TAG was ever used for anything public, but I could be wrong. It's certainly not used in bump_version.py, so if it turns out that we no longer need it, I'm happy to remove it.

  4. As for pre, I greped the whole codebase and found nothing except for a test in node_spec.js.

As for dealing with downstream fallout, I believe we can rest easy - this change would only affect beta versions, while production versions will continue to report a "simple" version.

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Nov 14, 2017

Member

Felix, IMO if ATOM_TAG and -pre aren't used we should remove them.

Other than that, I'm fine with this patch

Member

ckerr commented Nov 14, 2017

Felix, IMO if ATOM_TAG and -pre aren't used we should remove them.

Other than that, I'm fine with this patch

@felixrieseberg

This comment has been minimized.

Show comment
Hide comment
@felixrieseberg

felixrieseberg Nov 14, 2017

Member

@ckerr Updated once more!

Member

felixrieseberg commented Nov 14, 2017

@ckerr Updated once more!

@ckerr

ckerr approved these changes Nov 15, 2017

@ckerr ckerr merged commit 3a1106d into master Nov 15, 2017

6 of 7 checks passed

ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ckerr ckerr deleted the full-version-string branch Nov 15, 2017

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