Closed Bug 1366564 Opened 7 years ago Closed 7 years ago

rust-bindgen can't find Mac headers that aren't in /usr/include: fatal error: 'stdlib.h' file not found

Categories

(Firefox Build System :: General, defect, P2)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: cpeterson, Assigned: gps)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Stylo])

Attachments

(1 file, 1 obsolete file)

Bug 1366564 - Validate Xcode installation state in configure;
59 bytes, text/x-review-board-request
rillian
: review+
Details 
I tried to build Stylo last night on Mac, but rust-bindgen failed because the clang std headers installed by mach bootstrap can't #include_next the real std headers. I hit build errors like the following when trying to compile the generated bindings code:

/Users/chris/.mozbuild/clang/include/c++/v1/stdlib.h:94:15: fatal error: 'stdlib.h' file not found

jryans suggested that I run `xcode-select --install`, which fixed the problem.

Bobby says xcode-select installs the stdlib headers in /usr/include, which makes them findable by bindgen without complicated autoconf stuff.

@ Nick: can rust-bindgen somehow find Mac's headers if they are not in /usr/include ? Or should rust-bindgen or Firefox's configure just detect the missing /usr/include headers and log an error message that tells people to run xcode-select --install?
Flags: needinfo?(nfitzgerald)
Blocks: stylo-tooling
Bindgen doesn't do any fancy include directory finding itself, it completely relies on libclang for that. Users supply the include directories via one of

* When using a builder:

    builder.clang_arg("-I/usr/include")

* When using the CLI tool:

    bindgen <bindgen options> -- <clang options> -I/usr/include

Since bindgen doesn't deal with detecting include directories at all right now, I don't think it makes a ton of sense for it to take on this new responsibility. I think it makes most sense for Firefox's build system to do this kind of detection.
Flags: needinfo?(nfitzgerald)
That make sense. Seems like mach bootstrap should just run `xcode-select --install` automatically.
> $ xcode-select --install
> xcode-select: error: command line tools are already installed, use "Software Update" to install updates

Looks like we'd need to parse that error message, which seems brittle. :/
(In reply to Ralph Giles (:rillian) | needinfo me from comment #3)
> > $ xcode-select --install
> > xcode-select: error: command line tools are already installed, use "Software Update" to install updates
> 
> Looks like we'd need to parse that error message, which seems brittle. :/

We could potentially check if an example file exists (like /usr/include/stdlib.h from this bug's title), and only run `xcode-select --install` if it's missing?  Not sure if that's accurate enough.
Wouldn't `xcode-select --switch` be more appropriate? (although it requires knowning where the xcode install is)

Note `xcode-select --print-path` could be helpful too, and so would `xcrun --show-sdk-path` and other flags of xcrun.
(In reply to Mike Hommey [:glandium] from comment #5)
> Note `xcode-select --print-path` could be helpful too, and so would `xcrun
> --show-sdk-path` and other flags of xcrun.

Could we just have bindgen invoke `xcrun --show-sdk-path`, and then concatenate whatever that returns with "/usr/include", so we don't have to rely on the user running xcode-select or similar?  Would that be acceptable?
Flags: needinfo?(nfitzgerald)
We could. But, it still feels more "better" to me to have bindgen invocations do this -- what would be the advantage of bindgen doing this instead of making stylo's bindgen invocation do it?

Emilio, opinions?
Flags: needinfo?(nfitzgerald) → needinfo?(emilio+bugs)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #8)
> We could. But, it still feels more "better" to me to have bindgen
> invocations do this -- what would be the advantage of bindgen doing this
> instead of making stylo's bindgen invocation do it?
> 
> Emilio, opinions?

I meant in stylo's bindgen invocations, but fixing this once-for-all in bindgen itself would work, too!
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #8)
> We could. But, it still feels more "better" to me to have bindgen
> invocations do this -- what would be the advantage of bindgen doing this
> instead of making stylo's bindgen invocation do it?
> 
> Emilio, opinions?

We run already the clang binary, so doing this sounds fine.

That should probably live in https://github.com/KyleMayes/clang-sys, though.
Flags: needinfo?(emilio+bugs)
Blocks: 1375774
No longer blocks: stylo-nightly-build
Is this still blocking 1375774?  Now that we're cross-compiling, this likely isn't an issue on build boxes.  If we really want to fix this, see https://stackoverflow.com/a/15371967/8388 for ideas on how to check if `xcode-select --install` is required on different OSX versions.
Mac cross-compiled builds are still blocked by bug 1368083, which is another header file path issue similar to this bug. Nathan thinks Ralph's fix for bug 1368083 will fix (or can be used to fix) this bug.
Assigning to Ralph for now because his fix for bug 1368083 may also fix this build issue.
Assignee: nobody → giles
I don't believe the fix for bug 1368083 addresses this. It provides a work around in that mach will forward `-isysroot` settings to bindgen, but as I understand it the problem is that clang from the default xcode install will look in the SDK sysroot by default, while bindgen looks in /usr/include by default.

Most developers will run the equivalent of `xcode-select --install` at some point, but we should verify that if bindgen requires it.
This issue doesn't need to block building Mac in automation or local builds (bug 1375774) since there is a workaround.
No longer blocks: 1375774
Priority: P3 → --
Priority: -- → P4
In case someone else finds this and finds that their build doesn't work, even with comment 5. I guess I had an old clang build from brew and it was still installed in /usr/local/Cellar/llvm/HEAD. Stylo seems to expect something in /usr/local/Cellar/llvm/version since that's where it is with xcode. I had to manually uninstall llvm (brew uninstall llvm), remove all locally built versions of clang / llvm, and reinstall llvm. Restarted my terminal just to clear all env flags and it worked.
See Also: → 1394697
FWIW, I experienced the problem with 10.13.

running xcode-select --install didn't help at first.

However, shortly after a new upgrade appeared in the App Store for a new version of the command line tools (actually too, one for xcode on 10.11), and after installing those it worked.
Raising priority to reflect that people are hitting this in the wild and that the workaround is non-obvious.

As I wrote in bug 1403001, configure should ideally detect that we're in a bad state and recommend concrete steps to fix.
Blocks: highsierra
Severity: normal → critical
Priority: P4 → P2
No longer blocks: highsierra
We're hashing things out in #build. I'll start on a patch.
Assignee: giles → gps
Status: NEW → ASSIGNED
Comment on attachment 8912289 [details]
Bug 1366564 - Validate Xcode installation state in configure;

https://reviewboard.mozilla.org/r/183642/#review188818

LGTM with comments addressed.

::: build/moz.configure/toolchain.configure:104
(Diff revision 1)
> +option('--disable-xcode-checks',
> +       help='Do not check that Xcode is installed and properly configured')
> +
> +@depends(host)
> +def host_is_mac(host):
> +    return host.kernel == 'Darwin'

You don't use this, and only check the host in on place, so might as well remove it.

::: build/moz.configure/toolchain.configure:113
(Diff revision 1)
> +    if host.kernel != 'Darwin' or not xcode_checks:
> +        return
> +
> +    def bad_xcode_select():
> +        die('Could not find installed Xcode; run Xcode.app to ensure '
> +            'initial Xcode configuration is performed')

I would be more verbose here. We know having the latest Xcode can be important, and I don't know how many people will map 'run Xcode.app' to "Look for Xcode in Applications and install it from the app store if it isn't there, then launch it."

::: build/moz.configure/toolchain.configure:134
(Diff revision 1)
> +                           'com.apple.pkg.CLTools_Executables',
> +                            onerror=no_cltools)
> +
> +    return res
> +
> +set_config('XCODE_CLTOOLS_PATH', xcode_command_line_tools)

`pkgutil` returns several lines on success, none of which are a path (unless `location: /` counts). Maybe just return the output of `xcode-select -p` instead?

It would be nice if there were a way to get this into the dependency graph without cluttering up the build config with information we're not using.
Attachment #8912289 - Flags: review?(giles) → review+
Comment on attachment 8912289 [details]
Bug 1366564 - Validate Xcode installation state in configure;

https://reviewboard.mozilla.org/r/183642/#review188818

> I would be more verbose here. We know having the latest Xcode can be important, and I don't know how many people will map 'run Xcode.app' to "Look for Xcode in Applications and install it from the app store if it isn't there, then launch it."

I believe `xcode-select --install` will prompt to install xcode itself if it isn't present so maybe something like "Please install Xcode from the App Store and launch it to complete initial configuration, or run 'xcode-select --install' from the command line."
Summary: stylo: rust-bindgen can't find Mac headers that aren't in /usr/include: fatal error: 'stdlib.h' file not found → rust-bindgen can't find Mac headers that aren't in /usr/include: fatal error: 'stdlib.h' file not found
Whiteboard: [Stylo]
Comment on attachment 8912289 [details]
Bug 1366564 - Validate Xcode installation state in configure;

https://reviewboard.mozilla.org/r/183642/#review188924
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2ecf684f49e
Validate Xcode installation state in configure; r=rillian
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #1)
> Bindgen doesn't do any fancy include directory finding itself, it completely
> relies on libclang for that. Users supply the include directories via one of
> 
> * When using a builder:
> 
>     builder.clang_arg("-I/usr/include")
> 
> * When using the CLI tool:
> 
>     bindgen <bindgen options> -- <clang options> -I/usr/include
> 
> Since bindgen doesn't deal with detecting include directories at all right
> now, I don't think it makes a ton of sense for it to take on this new
> responsibility. I think it makes most sense for Firefox's build system to do
> this kind of detection.

... except nobody passes /usr/include to clang on its command line. And if the build reaches rust code, it means at least some C/C++ code has been built already, so clang is also happy that /usr/include doesn't exist. So whatever clang is doing to find the system headers, it seems like bindgen should do it too.
(In reply to Mike Hommey [:glandium] from comment #31)
> ... except nobody passes /usr/include to clang on its command line. And if
> the build reaches rust code, it means at least some C/C++ code has been
> built already, so clang is also happy that /usr/include doesn't exist. So
> whatever clang is doing to find the system headers, it seems like bindgen
> should do it too.

So we explicitly ask bindgen not to add search path from clang on macOS.

Specifically, bindgen skips that when --target is specified [1], and we specify --target for macOS build target [2] because of conflict with -stdlib=libc++ (which was added in servo/servo#15414).

I forgot the full rationale why we need to pass -stdlib=libc++ and how that would conflict with search path provided by clang. IIRC, there are some other environment variables / parameters involve for the target macOS version, and clang produces different search paths for different target versions. That was probably a workaround for that.

If we can pass in the correct target macOS version, probably we can allow bindgen to add clang search path, so that this would no longer be a problem.


[1] https://github.com/rust-lang-nursery/rust-bindgen/blob/e78394959f40193a4bf9e5718dd4f779cb57437a/src/lib.rs#L1508-L1528
[2] https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/layout/style/ServoBindings.toml#15-18
Attachment #8912272 - Flags: review?(gps)
Attachment #8912272 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e2ecf684f49e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Mike Hommey [:glandium] from comment #31)

> ... except nobody passes /usr/include to clang on its command line. And if
> the build reaches rust code, it means at least some C/C++ code has been
> built already, so clang is also happy that /usr/include doesn't exist. So
> whatever clang is doing to find the system headers, it seems like bindgen
> should do it too.

When I looked at it, the issue was that Apple clang, the system compiler, is configured to use the headers in the default Xcode SDK by default, so it works fine if there's no /usr/include. That search path configuration is a feature of the clang/clang++ front end, and not libclang. So we would have to manually invoke clang to get the default search path, then add that to the configuration bindgen passes to libclang.

However, because of minimum-supported-version issues, bindgen doesn't in general use the system libclang. Only the build system knows what C/C++ compiler is actually being used, so to me it seems less confusing to have the build system parse out the include paths and add them to the non-system include directives passed to bindgen rather than passing an extra compiler bindgen should call to do the same thing.
Blocks: 1403982
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: