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

CI: retire macOS 10.10 for v10 #1426

Closed
wants to merge 2 commits into from
Closed

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 1, 2018

This is to enable V8 6.8
That in turn will break building v10 on macOS 10.10, but testing showed that the generated binaries are backwards compatible with 10.10 (test suite passes 100%)

Ref: nodejs/node#22040 (comment)
Ref: #1409 (comment)

@refack
Copy link
Contributor Author

refack commented Aug 1, 2018

/CC @nodejs/lts (RE breaking build compatibility mid stream)

@maclover7
Copy link
Contributor

I forget if I commented this in the other version selector PR, but can we add entries here for macOS 10.08 and 10.09 as well?

@refack
Copy link
Contributor Author

refack commented Aug 1, 2018

I forget if I commented this in the other version selector PR, but can we add entries here for macOS 10.08 and 10.09 as well?

I think we could simply retire those workers. I don't think we have any plans to use them in CI.

@mhdawson
Copy link
Member

mhdawson commented Aug 2, 2018

@refack do we have anywhere the benefits to upgrading to 6.8? We know the cost is people will no longer be able to compile on 1010, but having the benefits called out would make the decision easier.

@refack
Copy link
Contributor Author

refack commented Aug 3, 2018

I guess it's the usual "keeping V8 current" concerns.
This is the V8 release post: https://v8project.blogspot.com/2018/06/v8-release-68.html
This is the node issue: nodejs/node#21079

P.S. It actually should not be too hard to patch the code, or workaround the compiler incompatibility, but is it worth it, just to maintain support for compilation on an EOL desktop platform ⚖️

@refack
Copy link
Contributor Author

refack commented Aug 3, 2018

Ping @ljharb for the nvm POV...

@ljharb
Copy link
Member

ljharb commented Aug 3, 2018

umm - so node v10 is supported on Mac OS 10.10, but it's not being tested? That seems like the sort of thing that should be coupled together.

Is there any possibility of compiling v8 6.8 on Mac OS 10.10, that nvm could detect the need for and use? If not, this seems like something that either the v8 team should resolve, or that would block upgrading it in anything that's not a semver-major line.

@gibfahn
Copy link
Member

gibfahn commented Aug 3, 2018

this seems like something that either the v8 team should resolve, or that would block upgrading it in anything that's not a semver-major line.

I'm inclined to agree by default. If someone feels strongly that landing V8 6.8 in 10.x is worth it, then it sounds like there are still avenues to explore regarding getting it working on OS X 10.10. If that is agreed to be infeasible then if someone wants to make a case for V8 6.8 then we could consider it.

cc/ @nodejs/v8-update in case there is a strong reason to upgrade.

@psmarshall
Copy link

Hi folks, sorry, kind of late to the party here. There is actually a simple fix from the V8 side: we can just remove 'thread_local' as this code runs single-threaded anyway (currently).

Here's a PR: nodejs/node#22105
We could float it on 10.x, allowing V8 6.8 to land. We would really like to land V8 6.8 so hopefully that's possible with this patch.

@ljharb
Copy link
Member

ljharb commented Aug 3, 2018

Something that allows upgrading v8 without breaking any single human who was previously working and supported sounds like a win to me.

@mhdawson
Copy link
Member

mhdawson commented Aug 7, 2018

I'm +1 to fixing in V8 for 10.x by floating the patch and building on 1011 for 11.x+ (which is now the case). This would seem to have the lowest impact. The only negative might be if there were later patches for V& that could not be backported because they also needed a newer compiler.

@refack
Copy link
Contributor Author

refack commented Aug 8, 2018

👍 for nodejs/node#22105

@rvagg
Copy link
Member

rvagg commented Aug 10, 2018

nodejs/node#21668 landed with @psmarshall's fix in it on v10.x-staging so I believe this can be closed now?

@refack refack closed this Aug 10, 2018
@refack refack deleted the refack-no-macos1010-for-v10 branch August 10, 2018 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants