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

v8: fix build on solaris platforms #1079

Closed
wants to merge 1 commit into from

Conversation

jbergstroem
Copy link
Member

Not sure what our policy is about patching v8 but my general standpoint is to avoid it at almost all costs. What that out the way, carrying this "one-liner" (patch sent upstream and currently in progress, but I'm unsure when or in what branch this will land) adds support for building io.js on smartos/illumos which also passes the test suite.

`v8/3c7e4403` introduced a different cast which broke building on
Illumos. Revert to previous behavior for V8_OS_SOLARIS.

Found on SmartOS while building with gcc 4.9.0.
@mscdex
Copy link
Contributor

mscdex commented Mar 6, 2015

Is there a fix for this upstream?

@jbergstroem
Copy link
Member Author

/cc @rvagg @geek

@bnoordhuis
Copy link
Member

@jbergstroem Patches uploaded to the V8 bug tracker are normally ignored. You have to install depot_tools, run fetch v8, apply your patch and upload it with git-cl. Don't forget to assign reviewers afterwards or it still won't be seen. :-) If you have questions, you know where to find me.

@jbergstroem
Copy link
Member Author

@bnoordhuis Ah. I just assumed magic would happen because it was set as assigned. I'll give it a go.

@geek
Copy link
Member

geek commented Mar 6, 2015

Seems like a reasonable change for a big impact, thanks @jbergstroem

@jbergstroem
Copy link
Member Author

Review going on here now: https://codereview.chromium.org/990063002

@Fishrock123
Copy link
Member

Looks like the patch landed upstream.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2015

In what version will it become available to us? @bnoordhuis are we good to float this until then?

@Fishrock123
Copy link
Member

Looks like it landed in master, if I'm not mistaken. That would be v8 4.3, or about 11? weeks out. Quite a ways.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2015

But how many v8 upgrades are going to happen in that 11 weeks? Seems like a long time to wait for a two line change.

@Fishrock123
Copy link
Member

According to our plans, one for 4.2, and one for 4.3, both when they ship in stable chrome (at the end of each dev cycle).

Seems like it's reasonable to float it, but I'm sure @bnoordhuis can weigh in much better.

@bnoordhuis
Copy link
Member

It's okay to back-port (edit: meant float) it, it'll probably apply cleanly. Maybe @jbergstroem can ping the V8 people and ask them to back-port it to the 4.1 and 4.2 branches.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2015

@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2015

Looks like the usual failures, and one random timeout on OS X. I'm going to land this.

cjihrig pushed a commit that referenced this pull request Mar 12, 2015
`v8/3c7e4403` introduced a different cast which broke building on
Illumos. Revert to previous behavior for V8_OS_SOLARIS. Found on
SmartOS while building with gcc 4.9.0.

V8-Issue: https://code.google.com/p/v8/issues/detail?id=3935
V8-Patch: https://codereview.chromium.org/990063002
PR-URL: #1079
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2015

Thanks! Landed in 8c4f0df

@cjihrig cjihrig closed this Mar 12, 2015
@jbergstroem
Copy link
Member Author

I have asked v8 people for a backport, but haven't gotten any response. Will follow up.

@rvagg rvagg mentioned this pull request Mar 14, 2015
@jbergstroem
Copy link
Member Author

Looks like its getting merged into 4.1 (and 4.2): https://chromium.googlesource.com/v8/v8.git/+/f2d617d6b962b8a6709b2564fe1d576572801237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants