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

v4: fix building v8 on libc++ 3.8.0 #9763

Closed

Conversation

jbergstroem
Copy link
Member

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

deps/v8

Description of change

Make sure the map allocators are of the same type. This fixes building Node.js 4.x on libc++ 3.8.0 (for instance FreeBSD 11).

Upstream bug/patch: https://bugs.freebsd.org/208467

/cc @thealphanerd

@jbergstroem jbergstroem added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Nov 23, 2016
@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Nov 23, 2016
@jbergstroem
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/6154/

(I've disabled the nodejs 4.x/freebsd 11 skip)

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but you should bump the patch level in v8-version.h.

@MylesBorins MylesBorins changed the base branch from v4.x to v4.x-staging November 23, 2016 23:50
Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Member

V8: CI

will land if green

/cc @nodejs/v8 to ensure this is ok to float... as the version of V8 on v4.x is unmaintained I can't think of a better way to do this

@jbajwa
Copy link
Contributor

jbajwa commented Nov 28, 2016

I think rebase is required as the base branch is switched to nodejs:v4.x-staging from nodejs:v4.x. nodejs:v4.x doesn't have the updated make-v8.sh hence we are seeing the CI failures

@ofrobots
Copy link
Contributor

ofrobots commented Nov 29, 2016

For posterity, upstream has this fix in v8/v8@96cb90983 (partially). js-type-feedback.h no longer exists upstream, so I don't see a corresponding upstream commit for that.

@jbergstroem
Copy link
Member Author

@ofrobots: cool, thanks for bringing it upstream!

@ofrobots
Copy link
Contributor

@jbergstroem to be clear, I didn't bring it upstream. Relevant upstream branches (other than the one corresponding to v4.x) already had the fix.

Make sure the map allocators are of the same type. This fixes
building Node.js 4.x on libc++ 3.8.0 (for instance FreeBSD 11).

Upstream bug/patch: https://bugs.freebsd.org/208467
@MylesBorins
Copy link
Member

I've gone ahead and rebased... one more go at CI

https://ci.nodejs.org/job/node-test-commit-v8-linux/444/

MylesBorins pushed a commit that referenced this pull request Dec 1, 2016
Make sure the map allocators are of the same type. This fixes
building Node.js 4.x on libc++ 3.8.0 (for instance FreeBSD 11).

Upstream bug/patch: https://bugs.freebsd.org/208467

PR-URL: #9763
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
Copy link
Member

the failures on the v8 CI are appearing on v4.6.2, unrelated to this change.

Landed in c1effb1

@MylesBorins MylesBorins closed this Dec 1, 2016
@MylesBorins MylesBorins mentioned this pull request Dec 6, 2016
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Dec 7, 2016
Make sure the map allocators are of the same type. This fixes
building Node.js 4.x on libc++ 3.8.0 (for instance FreeBSD 11).

Upstream bug/patch: https://bugs.freebsd.org/208467

PR-URL: nodejs/node#9763
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants