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

add a node suffix to V8's version number #15698

Closed
targos opened this issue Sep 30, 2017 · 5 comments
Closed

add a node suffix to V8's version number #15698

targos opened this issue Sep 30, 2017 · 5 comments
Labels
discuss Issues opened for discussions and feedbacks. v8 engine Issues and PRs related to the V8 dependency.
Milestone

Comments

@targos
Copy link
Member

targos commented Sep 30, 2017

About one year ago, we tried to add the possibility on V8 to have a custom suffix appended to the version string. This would allow us to apply patches to stable V8 versions without conflicting with the original version number and keep track of our own changes. The idea is to define this suffix in common.gypi (example).
For example we can choose to use .node.X or -node.X and increment X every time we apply a non-official patch to deps/v8. The full version string would look like 6.1.534.42.node.12.

Unfortunately, the change failed to land at that time because of an issue with how V8 is updated in Chromium. I changed the code a little bit this week and it should be good to go. The CL has already been reviewed and accepted: https://chromium-review.googlesource.com/c/v8/v8/+/690475

@nodejs/v8 and @nodejs/tsc please give your opinion on the approach. If you are OK with it, I will land the change in V8 and backport it here so we can have it on time for v9.0.0.

@targos targos added discuss Issues opened for discussions and feedbacks. v8 engine Issues and PRs related to the V8 dependency. labels Sep 30, 2017
@targos targos added this to the 9.0.0 milestone Sep 30, 2017
@mcollina
Copy link
Member

mcollina commented Sep 30, 2017 via email

@addaleax
Copy link
Member

Thanks for looking after this – I think I’d prefer -node.X as the suffix :)

@ofrobots
Copy link
Contributor

Thanks for getting this put together! I think this would be semver major because I suspect people try to parse the process.versions.v8 string.

@targos
Copy link
Member Author

targos commented Sep 30, 2017

I think this would be semver major because I suspect people try to parse the process.versions.v8 string.

I agree. This is why I would like it to be ready for v9.

targos added a commit to targos/node that referenced this issue Oct 5, 2017
After this commit, `process.versions.v8` will look like:
"6.0.287.53-node.0".
The goal is that everytime we apply a non-official patch to `deps/v8`,
we increment our own number instead of V8's patch level.
This number must be set back to 0 after major V8 updates.

Closes: nodejs#15698
@targos
Copy link
Member Author

targos commented Oct 5, 2017

PR: #15785

@targos targos closed this as completed in 70832bc Oct 18, 2017
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
After this commit, `process.versions.v8` will look like:
"6.0.287.53-node.0".
The goal is that everytime we apply a non-official patch to `deps/v8`,
we increment our own number instead of V8's patch level.
This number must be set back to 0 after major V8 updates.

Fixes: nodejs/node#15698
PR-URL: nodejs/node#15785
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
After this commit, `process.versions.v8` will look like:
"6.0.287.53-node.0".
The goal is that everytime we apply a non-official patch to `deps/v8`,
we increment our own number instead of V8's patch level.
This number must be set back to 0 after major V8 updates.

Fixes: nodejs/node#15698
PR-URL: nodejs/node#15785
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants