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

Various changes to the build script #28

Merged
merged 11 commits into from
Aug 27, 2019

Conversation

glandium
Copy link
Contributor

The biggest change is how bindgen is setup both for native and cross compilation. This should finish to address #23.

They cause the following warnings:
warning: attribute must be of the form `#[link(name = "...", /*opt*/ kind = "dylib|static|...",
                                               /*opt*/ cfg = "...")]`
 --> /tmp/coreaudio-sys/target/x86_64-apple-darwin/debug/build/coreaudio-sys-0d8fd082e359c2ec/out/coreaudio.rs:3:1
  |
3 | #[link = "/tmp/MacOSX10.14.sdk/System/Library/Frameworks/AudioToolbox"]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(ill_formed_attribute_input)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #57571 <rust-lang/rust#57571>

And they are actually redundant with the cargo:rustc-link-lib lines the
build script prints already.
@glandium
Copy link
Contributor Author

The last failure here is because this is not my day: https://rust-lang.github.io/rustup-components-history/x86_64-apple-darwin.html says rustfmt is missing just today...

@glandium glandium force-pushed the build-script branch 3 times, most recently from 7588de1 to add52f5 Compare July 26, 2019 14:36
The current setup requires 2 environment variables for
cross-compilation, and can fail to build under some circumstances on a
freshly installed OSX + Xcode machine.

This change simplifies the setup by:
- On native builds, using `xcrun --show-sdk-path` instead of
  `xcode-select -p` to find the SDK.
- On cross builds, checking COREAUDIO_SDK_PATH.
- Pass --target to clang, which makes things work for cross-compilation
  with plain clang (as opposed to osxcross).
- Pass -isysroot to clang instead of -F, which makes it find all
  possible headers in the SDK, especially TargetConditionals.h, which
  lives in $COREAUDIO_SDK_PATH/usr/include.
- Instruct cargo of the environment variables that affect the build
  script.

COREAUDIO_CFLAGS is left as a convenience, for now.
As of 0.49, bindgen supports a BINDGEN_EXTRA_CLANG_ARGS environment variable
that can be used to pass extra arguments to clang. This can be used
instead of COREAUDIO_CFLAGS.
Because recent versions of bindgen generate their bindings on one line,
because of bindgen's use of u128 and rust's warnings of its unsafety
FFI-wise, and because of rust-lang/rust#62999,
combined, building the crate generates massive logs... which exceed
travis's maximum log length.

It was disabled in RustAudio#13 because back then rustfmt had some problems with
the bindgen output. Now that rustfmt is part of rust, these kind of
problems presumably should happen less if at all.
Simply printing errors to stderr does nothing visible by default. So
when building without an SDK available, there is no apparent error
showing up other than in some file "hidden" in target/, and an error
that including coreaudio.rs failed.

Similarly, when attempting to build for an unsupported target, the
script reaches the `unreachable!()`.
Instead of passing the full paths to the headers to bindgen, create a
meta header that #include's all the needed ones. This leaves it to clang
to find them in the right /System/Library/Frameworks directory.
Now that the build script itself doesn't need the SDK path, allow not to
specify one explicitly. This allows things to work out when the
environment works out of the box one way or another (via wrapper
scripts, setting BINDGEN_EXTRA_CLANG_ARGS, etc.)
@glandium
Copy link
Contributor Author

glandium commented Aug 6, 2019

Just rebased to incorporate the README fixup in the right commit, and to have a green build, now that rustfmt is there for nightly.

@seivan
Copy link

seivan commented Aug 12, 2019

If you want to support iOS you gotta add headers.push("CoreAudio/CoreAudioTypes.h"); - but that's not enough since coreaudio-rs has code that touches on keys that only macOS exports.

@mitchmindtree
Copy link
Member

@seivan would you be up for opening an issue about this?

Thanks a lot @glandium, this generally looks good to me!

I don't have the ability to test this locally just at the moment - @zicklag would you be interested in reviewing this? It looks like this touches a lot of the cross-compilation support you added recently and looks to simplify things quite a bit with a new bindgen version (most changes are detailed in the commit msgs).

@seivan seivan mentioned this pull request Aug 13, 2019
@seivan
Copy link

seivan commented Aug 13, 2019

@mitchmindtree Done: #29

@zicklag
Copy link
Contributor

zicklag commented Aug 13, 2019

@mitchmindtree I can't try it out just yet, but I'll look at it when I next get the chance.

It looks like this makes some great improvements!

Copy link
Contributor

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

I'm testing the new changes now. :)


```bash
export COREAUDIO_FRAMEWORKS_PATH=/build/osxcross/target/SDK/MacOSX10.11.sdk/System/Library/Frameworks
export COREAUDIO_SDK_PATH=/build/osxcross/target/SDK/MacOSX10.11.sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to mention using BINDGEN_EXTRA_CLANG_ARGS here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It did work for me without having to set any extra clang args, so it might be unnecessary, but in the even that somebody needs it, it could save them some time searching for it.

@zicklag
Copy link
Contributor

zicklag commented Aug 25, 2019

It worked for my build of Amethyst. 👍

@mitchmindtree
Copy link
Member

Good enough for me!

Thanks a lot for the review @zicklag and thanks again @glandium!

@mitchmindtree mitchmindtree merged commit 824d3f5 into RustAudio:master Aug 27, 2019
@zicklag
Copy link
Contributor

zicklag commented Aug 28, 2019

Awesome! 🙂

@mitchmindtree Could we make a release for this. I'm using these cross-compiling changes for my project, but I have to use a Cargo.tom replace directive to use it.

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.

4 participants