Closed Bug 1335302 Opened 4 years ago Closed 4 years ago

stylo: build-time bindgen doesn't work on mac

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bholley, Assigned: xidorn)

References

Details

Attachments

(2 files, 1 obsolete file)

Manish hit this too. We've both disabled build-time bindgen, but seems like someone should look into this. Ted or Nathan?
Attached file log
failure log
I work-around the compile errors by adding "--stdlib=libc++" and "--target=x86_64-apple-darwin" to clang_arg. This fixes the compile error, but the build script still fails for invalid memory reference...
Filed rust-bindgen issue for this crash: https://github.com/servo/rust-bindgen/issues/462
The crash I mentioned in comment 3 is because of "--stdlib=libc++". libclang has a bug on parsing its own header, it seems.

Without that, it wouldn't crash. The issue here is that, bindgen gives wrong search paths, and there is no C++11 headers in that path. Supposely clang-sys invokes the system builtin clang rather than the clang installed by homebrew or else.

This can be workaround by "export CLANG_PATH=/usr/local/opt/llvm/bin/clang" before calling "./mach build".

IIRC, mach is supposed to provide CLANG_PATH environment variable to cargo build, so the question is why it doesn't work as expected. It is probably a question for Nathan, then.
I was wrong in comment 5. It doesn't work with CLANG_PATH specified either. The clang path is correct, which indicates that CLANG_PATH from mach script does its work as expected.

It seems the path of clang is correct, the version of clang is correct, but the path it returns is wrong... I have no idea why...
After some investigation, it eventually turns out that it is because of MACOSX_DEPLOYMENT_TARGET environment variable passed from mach. When MACOSX_DEPLOYMENT_TARGET <= 10.8, clang would return
> /usr/include/c++/4.2.1
> /usr/include/c++/4.2.1/backward
as the search path for C++, unless -stdlib=libc++ is explicitly specified. There is no C++11 headers in these directories.

If that environment variable does not present or >= 10.9, clang would return the path of its own headers, which includes everything we need.

Given that we've removed support for OS X 10.6~10.8, we can probably lift this variable to 10.9 directly, so that we would not need any special handling for bindgen.
Nathan, WDYT?
Flags: needinfo?(nfroyd)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> Nathan, WDYT?

Upgrading MACOSX_DEPLOYMENT_TARGET would be a fine idea, as our default is 10.7.  But that gets into issues of updating the Mac automation builders (bug 1270217), which AIUI is non-trivial.

Is there some way of injecting -stdlib=libc++ into bindgen/clang-sys instead?  That would at least match what we force in our configury.

Ted, do you remember if there were/are issues getting >=10.9 SDKs on the cross builders?
Flags: needinfo?(nfroyd) → needinfo?(ted)
That's bug 1270217, it's blocked by the fact that our OS X build machines are still running 10.7 currently. That won't get fixed until we migrate all of our Mac builds to taskcluster (bug 1267425 etc).
bug 1324892 covers updating the SDK.
Flags: needinfo?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> That's bug 1270217, it's blocked by the fact that our OS X build machines
> are still running 10.7 currently. That won't get fixed until we migrate all
> of our Mac builds to taskcluster (bug 1267425 etc).

What's the timeframe for that? If it's likely to happen before the end of Q2 (which is our target for turning on --enable-stylo on nightly), could we just make --enable-stylo imply MACOSX_DEPLOYMENT_TARGET=10.9, which would only impact local developer builds?
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> > That's bug 1270217, it's blocked by the fact that our OS X build machines
> > are still running 10.7 currently. That won't get fixed until we migrate all
> > of our Mac builds to taskcluster (bug 1267425 etc).
> 
> What's the timeframe for that? If it's likely to happen before the end of Q2
> (which is our target for turning on --enable-stylo on nightly), could we
> just make --enable-stylo imply MACOSX_DEPLOYMENT_TARGET=10.9, which would
> only impact local developer builds?

Note that you can add --enable-macos-target=10.9 (or what have you) to the mozconfig if you're on mac.  This is not quite as convenient as having configure do it for you, but it is a short-term workaround.
Also what about just setting the env var during binding generation? Does that work?
(In reply to Nathan Froyd [:froydnj] from comment #13)
> Note that you can add --enable-macos-target=10.9 (or what have you) to the
> mozconfig if you're on mac.  This is not quite as convenient as having
> configure do it for you, but it is a short-term workaround.

This doesn't seem to fix it.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15)
> (In reply to Nathan Froyd [:froydnj] from comment #13)
> > Note that you can add --enable-macos-target=10.9 (or what have you) to the
> > mozconfig if you're on mac.  This is not quite as convenient as having
> > configure do it for you, but it is a short-term workaround.
> 
> This doesn't seem to fix it.

Ah, right.  We'd probably need to set it during binding generation, as Emilio suggests in comment 14.
If somebody writes a patch I can test it.
This really shouldn't be necessary because of:

https://dxr.mozilla.org/mozilla-central/source/config/config.mk#378

but who knows?
Attachment #8832238 - Flags: feedback?(bobbyholley)
I don't think the patch would be useful... because this happens because of the presenting of this environment variable, so setting it to the same thing shouldn't have any effect.

If upgrading it to 10.9 is nontrivial, I think the next easiest thing might be setting it to empty for cargo.
I think I have a minimum reproducible example for the bindgen crash, and I probably know where the problem is... but I don't have clear idea how to fix it. Anyway, the problem is clear now.
I've verified that, with servo/rust-bindgen#466 applied, build-time bindgen now works on macOS with the additional line in .mozconfig:
> ac_add_options --enable-macos-target=10.9

That said, if bug 1270217 is expected to get fixed soonish, we don't need any change other than upgrading the vendored bindgen to a version with that fixed.
Depends on: 1336228
Comment on attachment 8832238 [details] [diff] [review]
export MACOSX_DEPLOYMENT_TARGET during cargo build

Thanks for sorting that out xidorn!

Nathan, see comment 12.
Flags: needinfo?(nfroyd)
Attachment #8832238 - Flags: feedback?(bobbyholley)
(I filed bug 1336228 for the bindgen update).
This patch addresses problems arising from several decisions that make
sense in isolation:

* We compile Gecko with a MACOSX_DEPLOYMENT_TARGET of 10.7, largely
  because upgrading our Mac builders with a newer Xcode etc. is rather
  difficult, and our cross-compilation infrastructure isn't quite ready
  for prime time;

* Despite this, we compile Gecko with -stdlib=libc++ to enable using a
  C++11-compliant standard library;

* Given a deployment target of 10.7, clang, invoked via bindgen, opts to
  use libstdc++ as the C++ standard library;

* bindgen then has issues finding some of the C++11-era standard library
  headers that we use.  Even if it didn't, if we tried exporting
  standard library data structures across the C++/Rust boundary, we
  would be in for a world of pain later on when the inevitable
  mismatches occur.

The easiest fix for now is to force a MACOSX_DEPLOYMENT_TARGET of 10.9
when compiling with stylo (and thus using bindgen).  This change forces
bindgen-invoked clang to use the correct C++ standard library.  While
this change wouldn't be suitable for automated infrastructure
builds (see first bullet, above), we're not currently building Stylo on
or for OS X machines in automation, and we expect to have the
cross-compilation issues sorted out by the time we would care about
building Stylo for OS X in automation.  All we care about here is
unblocking local Stylo developers on OS X.

Asking for feedback from two Mac developers that I know of working on Stylo; if
anybody else wants to test it out and report back, that would be great.  The
patch doesn't seem to blow up on my local Linux box, at least.
Attachment #8833998 - Flags: feedback?(manishearth)
Attachment #8833998 - Flags: feedback?(bobbyholley)
I think the patch addresses the ni?
Flags: needinfo?(nfroyd)
Isn't the bindgen crash fixed already, shouldn't -stdlib=libc++ be enough now?
Attachment #8832238 - Attachment is obsolete: true
(In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> Isn't the bindgen crash fixed already, shouldn't -stdlib=libc++ be enough
> now?

I guess that would be sufficient, if we can do that solely in bindgen.
Comment on attachment 8833998 [details] [diff] [review]
enable proper OS X deployment target for stylo builds; r=chmanchester,f=bholley

(In reply to Nathan Froyd [:froydnj] from comment #27)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > Isn't the bindgen crash fixed already, shouldn't -stdlib=libc++ be enough
> > now?
> 
> I guess that would be sufficient, if we can do that solely in bindgen.

I thought there were multiple issues here though? It's not clear to me from xidorn's investigation whether -stdlib=libc++ is sufficient.

Passing the f? over to him for now. I'm happy to retest once incubator picks up the bindgen fix.
Attachment #8833998 - Flags: feedback?(xidorn+moz)
Attachment #8833998 - Flags: feedback?(manishearth)
Attachment #8833998 - Flags: feedback?(bobbyholley)
If we add -stdlib=libc++, it is probably better to add --target=x86_64-apple-darwin in addition, to disable the search path fixup bindgen applies (which I want to kill, because I don't see good reason to keep that hacky thing...) otherwise there would be two sets of stdlib headers in the search path, which may lead to unpredictable issues.

Given that Gecko uses -stdlib=libc++, and this argument affects the search path, I'd slightly prefer we add this argument (with disabling the fixup in bindgen via adding --target or something) to keep thing sync with Gecko's config, even though using higher version of macosx target also fixes this issue.
Comment on attachment 8833998 [details] [diff] [review]
enable proper OS X deployment target for stylo builds; r=chmanchester,f=bholley

Review of attachment 8833998 [details] [diff] [review]:
-----------------------------------------------------------------

As I mentioned in comment 29, I think it is probably better to add clang args to build_gecko.rs to keep it more sync with Gecko's config, so probably no Gecko side change is necessary.
Attachment #8833998 - Flags: feedback?(xidorn+moz)
Submitted https://github.com/servo/servo/pull/15414 for change in Servo side.
Xidorn, is any more work needed to fix build-time bindgen on Mac in mozilla-central?
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Priority: -- → P3
I don't think there is anything else need for build-time bindgen to work on Mac. We can close this bug as FIXED.

Feel free to reopen if anyone still fail to build with build-time bindgen on Mac.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(xidorn+moz)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.