Skip to content

Commit

Permalink
Auto merge of harfbuzz#139 - spl:dep-harfbuzz-include, r=jdm
Browse files Browse the repository at this point in the history
Print proper cargo:include values

In order to build a Rust crate with C/C++ that uses the HarfBuzz library referenced by the `harfbuzz` crate, I need to know where the header files are. I believe the standard way of doing that is the `links` key/value environment variables. In particular, I'm expecting `DEP_HARFBUZZ_INCLUDE` to tell me the include path for `hb.h` and other header files. Unfortunately, this doesn't seem to be working correctly now. This PR is an attempt to resolve that.

Summary of changes:
* Prints proper values for `cargo:include` in the build script for both the `pkg-config` and vendored cases.
* Changes a use of `to_str().unwrap()` to `PathBuf::display()`. I believe this is better, but I'm happy to be corrected.

As far as adding a useful feature goes, I think this PR is complete. However, there are a couple of potential improvements that would be nice, but I'm not sure how to go about them.

**`not(feature = "build-native-harfbuzz")`**

I'm not exactly sure what disabling the feature `build-native-harfbuzz` does. It doesn't use `pkg-config` and doesn't build the vendored library? Consequently I'm not sure what to print for `cargo:include` here.

**Testing**

It would be nice to test for the presence and correctness of `DEP_HARFBUZZ_INCLUDE`. I used the following files (in the repository's top-level directory for convenience) both with `HARFBUZZ_SYS_NO_PKG_CONFIG=1` and without:

`Cargo.toml`:

```toml
[project]
name = "foo"
version = "0.5.0"
authors = []
build = "build.rs"

[dependencies]
harfbuzz-sys = { path = "harfbuzz-sys" }

[build-dependencies]
cc = "1.0"
```

`build.rs`:

```rust
use std::env;
fn main() {
    let mut cfg = cc::Build::new();
    for path in env::split_paths(&env::var_os("DEP_HARFBUZZ_INCLUDE").unwrap()) {
        cfg.include(path);
    }
    cfg.file("src/test.c").warnings(false).compile("test");
}
```

`src/test.c`:

```c
#include <hb.h>
int test() {
    hb_font_t *hb_font;
    return 0;
}
```

`src/lib.rs`: empty

I'm not sure how to add such a test without simply writing a script and running it in CI. I did a quick search on GitHub and couldn't find any other projects that actually tested `cargo:include` in this way.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/139)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Mar 29, 2019
2 parents 27bd7e9 + 692048e commit 62a0050
Showing 1 changed file with 17 additions and 5 deletions.
22 changes: 17 additions & 5 deletions harfbuzz-sys/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,24 @@ fn main() {

println!("cargo:rerun-if-env-changed=HARFBUZZ_SYS_NO_PKG_CONFIG");
if env::var_os("HARFBUZZ_SYS_NO_PKG_CONFIG").is_none() {
if pkg_config::find_library("harfbuzz").is_ok() {
if let Ok(lib) = pkg_config::probe_library("harfbuzz") {
// Avoid printing an empty value
if !lib.include_paths.is_empty() {
// DEP_HARFBUZZ_INCLUDE has the paths of harfbuzz and dependencies.
println!(
"cargo:include={}",
env::join_paths(lib.include_paths)
.unwrap()
.to_str()
.unwrap()
);
}
return;
}
}

let out_dir = PathBuf::from(env::var_os("OUT_DIR").unwrap());

// On Windows, HarfBuzz configures atomics directly; otherwise,
// it needs assistance from configure to do so. Just use the makefile
// build for now elsewhere.
Expand All @@ -37,18 +50,17 @@ fn main() {
.success()
);

let out_dir = PathBuf::from(env::var_os("OUT_DIR").unwrap());
println!(
"cargo:rustc-link-search=native={}",
out_dir.join("lib").to_str().unwrap()
out_dir.join("lib").display()
);
println!("cargo:rustc-link-lib=static=harfbuzz");
}

// Dependent crates that need to find hb.h can use DEP_HARFBUZZ_INCLUDE from their build.rs.
// DEP_HARFBUZZ_INCLUDE has the path of the vendored harfbuzz.
println!(
"cargo:include={}",
env::current_dir().unwrap().join("harfbuzz/src").display()
out_dir.join("include").join("harfbuzz").display()
);
}

Expand Down

0 comments on commit 62a0050

Please sign in to comment.