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

Properly pass bindgen cflags to bindgen. #148

Merged
merged 2 commits into from Sep 21, 2018
Merged

Properly pass bindgen cflags to bindgen. #148

merged 2 commits into from Sep 21, 2018

Conversation

@emilio
Copy link
Member

emilio commented Sep 4, 2018

This gets me a cross build without anything else.

It's probable that we can simplify a bunch of the stuff in the build.rs file
with this patch, but I haven't tried to do that yet.

Whitespace handling is a bit broken (I hope nobody tries to build this crate in
a directory with spaces) but all the other uses of split_whitespace are equally
broken in build.rs.


This change is Reviewable

This gets me a cross build without anything else.

It's probable that we can simplify a bunch of the stuff in the build.rs file
with this patch, but I haven't tried to do that yet.

Whitespace handling is a bit broken (I hope nobody tries to build this crate in
a directory with spaces) but all the other uses of split_whitespace are equally
broken in build.rs.
@jdm
Copy link
Member

jdm commented Sep 4, 2018

How did you even find this?

@emilio
Copy link
Member Author

emilio commented Sep 4, 2018

How did you even find this?

I've poked too much at the moz.build files for cbindgen recently :)

@emilio
Copy link
Member Author

emilio commented Sep 4, 2018

This should also allow us to cross-compile OS X builds from Linux, should we want that :)

@emilio
Copy link
Member Author

emilio commented Sep 4, 2018

If we want to take this fix, this should probably get cleaned up so the replace is not needed and such, and so this is unified with Gecko's usage in:

https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/layout/style/bindgen.toml.in#3

And then upstreamed. But this probably works as an immediate solution to unblock stuff.

@paulrouget
Copy link

paulrouget commented Sep 4, 2018

Added mozjs_sys = { git = "https://github.com/servo/mozjs", branch = "mac-cross" } to Cargo.toml.

I get:


error[E0063]: missing field `__bindgen_padding_0` in initializer of `mozjs_sys::root::JS::ForOfIterator`
   --> /Users/paul/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs-0.9.0/src/conversions.rs:604:28
    |
604 |         let mut iterator = ForOfIterator {
    |                            ^^^^^^^^^^^^^ missing `__bindgen_padding_0`

error: aborting due to previous error

@asajeffrey
Copy link
Member

asajeffrey commented Sep 4, 2018

My goodness, that file was well-hidden. Should we add a println!("cargo:rerun-if-changed=js/rust/bindgen-flags");?

@emilio
Copy link
Member Author

emilio commented Sep 4, 2018

@paulrouget was it a clobber build? This touches configure stuff and I have to clobber a few times while writing the patch, the makefile.cargo condition is pretty broken.

@paulrouget
Copy link

paulrouget commented Sep 4, 2018

was it a clobber build?

Yes.

Gonna retry just in case.

@emilio
Copy link
Member Author

emilio commented Sep 5, 2018

If you still see it, then it'd be really unfortunate, because that means I cannot reproduce it even on the OS X loaner that I took...

@emilio
Copy link
Member Author

emilio commented Sep 5, 2018

Please also ensure that the override is getting used, if you're using an outdated build that doesn't match this mozjs version it silently won't get used.

@jdm
Copy link
Member

jdm commented Sep 5, 2018

I also have the same issue that Paul sees. I have verified that the mozjs_sys override with this branch is being used.

@paulrouget
Copy link

paulrouget commented Sep 5, 2018

adding mozjs = { git = "https://github.com/servo/rust-mozjs", branch = "android" } to Cargo.toml too solves the issue

@paulrouget
Copy link

paulrouget commented Sep 11, 2018

Is this blocked on anything?

@emilio
Copy link
Member Author

emilio commented Sep 18, 2018

Not that I know of (well, needs review). It does fix my build which needed a bunch of extra headers on OSX, but probably doesn't fix the bindgen issue.

@paulrouget
Copy link

paulrouget commented Sep 21, 2018

@asajeffrey or @jdm, can you please review this?

@jdm
Copy link
Member

jdm commented Sep 21, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2018

📌 Commit ae5dcde has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2018

Testing commit ae5dcde with merge ffdd32d...

bors-servo added a commit that referenced this pull request Sep 21, 2018
Properly pass bindgen cflags to bindgen.

This gets me a cross build without anything else.

It's probable that we can simplify a bunch of the stuff in the build.rs file
with this patch, but I haven't tried to do that yet.

Whitespace handling is a bit broken (I hope nobody tries to build this crate in
a directory with spaces) but all the other uses of split_whitespace are equally
broken in build.rs.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/148)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2018

💔 Test failed - status-travis

@jdm
Copy link
Member

jdm commented Sep 21, 2018

We hit #152.

@jdm
Copy link
Member

jdm commented Sep 21, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2018

Testing commit ae5dcde with merge e18734b...

bors-servo added a commit that referenced this pull request Sep 21, 2018
Properly pass bindgen cflags to bindgen.

This gets me a cross build without anything else.

It's probable that we can simplify a bunch of the stuff in the build.rs file
with this patch, but I haven't tried to do that yet.

Whitespace handling is a bit broken (I hope nobody tries to build this crate in
a directory with spaces) but all the other uses of split_whitespace are equally
broken in build.rs.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/148)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2018

💔 Test failed - status-travis

@jdm
Copy link
Member

jdm commented Sep 21, 2018

I think this PR may need to be rebased to accommodate the changes in #153, so I'm just going to press the button instead since the actual travis job passed.

@jdm jdm merged commit c7ee3cc into android Sep 21, 2018
3 of 5 checks passed
3 of 5 checks passed
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
homu Test failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@emilio emilio deleted the mac-cross branch Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.