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.

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

deps: remove thread_local to fix V8 compilation #22105

Closed
wants to merge 1 commit into from
Closed

deps: remove thread_local to fix V8 compilation #22105

wants to merge 1 commit into from

Conversation

psmarshall
Copy link
Contributor

@psmarshall psmarshall commented Aug 3, 2018 •
edited by refack
Loading

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

This should fix compilation on macOS 10.10, addressing these issues:
nodejs/build#1415
Fixes: nodejs/build#1426

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 3, 2018
@psmarshall psmarshall mentioned this pull request Aug 3, 2018
Closed
@targos
Copy link
Member

targos commented Aug 3, 2018

Thanks. Maybe we should do that only in the backport PR?
That will be easier to test as well. Tests run on macOS 10.11 for master

@psmarshall
Copy link
Contributor Author

Yes, fine by me, I just didn't know which branch to target it to. Could you target the PR to the right place? I'm not able to change it 👍

@psmarshall psmarshall changed the base branch from master to v10.x-staging August 3, 2018 11:30
@psmarshall psmarshall changed the base branch from v10.x-staging to master August 3, 2018 11:42
@psmarshall
Copy link
Contributor Author

Oh wait I figured out how to change the target branch. I'm not sure how to set this up. We want this to land with the V8 6.8 backport onto 10.x-staging - but I can't target this to 10.x-staging until v8 6.8 is in there (otherwise the patch makes no sense..).

@targos
Copy link
Member

targos commented Aug 3, 2018

My suggestion was to add this change to the 6.8 backport PR

@targos targos mentioned this pull request Aug 4, 2018
Closed
2 tasks
mhdawson
mhdawson approved these changes Aug 7, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Assuming the V8 team are confident this does not have a negative impact. LTGM as it allows V8 6.8 to compile with OSX 1010 and provides an easier path for V8 6.8 landing in the 10.x line.

@psmarshall
Copy link
Contributor Author

This was picked into #21668, closing.

@psmarshall psmarshall closed this Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@psmarshall @targos @mhdawson @nodejs-github-bot