LIHPC-Computational-Geometry / metis-rs

Idiomatic wrapper for METIS, the serial graph partitioner and fill-reducing matrix orderer
https://lihpc-computational-geometry.github.io/metis-rs/metis/
Apache License 2.0
10 stars 8 forks source link

Remove dependency on clang for `vendored` by not generating bindings on each build #21

Closed oisyn closed 9 months ago

oisyn commented 11 months ago

Since the METIS library in the vendor directory is essentially static (it requires a manual update), the bindings are technically also, but they are still regenerated on each build. This requires the user to have clang installed. The common approach in other Rust libraries is to regenerate the bindings manually, and have the file committed to the repo.

This PR adds a feature, generate-bindings, which will generate a new bindings.rs file based on METIS in the vendor directory. The file itself is located in metis-sys/gen/bindings.rs and is committed as part of this PR. This feature also enables vendored.

When building solely with vendored, it will use the pregenerated file and not be dependent on clang to be able to build metis-rs. So with vendored, metis-rs is now purely dependent on Rust and nothing else.

Also, when specifying vendored without default features, it no longer depends on bindgen either, which makes a clean build very lean:

oisyn@shesp:~/metis-rs$ cargo clean && cargo r --example graph --no-default-features --features vendored
   Compiling libc v0.2.148
   Compiling jobserver v0.1.26
   Compiling cc v1.0.83
   Compiling metis-sys v0.2.1 (/home/oisyn/metis-rs/metis-sys)
   Compiling metis v0.1.2 (/home/oisyn/metis-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 2.02s
     Running `target/debug/examples/graph`
[1, 1, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 0, 0]
oisyn commented 11 months ago

Oh but with my change, bindgen still runs when not vendored! It just doesn't run with vendored without generate-bindings.

There is one small caveat. It'll break if you used metis-rs with default-features = false, because it then doesn't specify bindgen as a dependency. Unfortunately I don't think there's a way to have it enabled without any features and then disable it when using vendored. But that's easily fixed by not using default-features = false.

Jasper-Bekkers commented 11 months ago

Ok, obviously not building bindgen is an advantage. Indeed, we (and a lot of folks in the rust ecosystem) structure crates like this so that a build of their project doesn't require any additional dependencies other then the default rust setup. (On windows, the default rust setup doesn't come with clang, for example, which this crate depended on).

However i'm not sure about this. bindgen has to run when not vendored, since in this case the sizes of idx_t and read_t are only known at build time. But there is no way to enable it when vendored is not enabled.

As @oisyn mentioned, this PR doesn't change the behavior for when metis-rs uses the system installed metis. Though I would add that in some cases it could also be useful to just ship pre-generated bindings for the set of idx_t and read_t's that you're willing to support for example.

oisyn commented 9 months ago

Hi! I would appreciate it if this got merged. Is there anything you'd rather see done differently, or do you have concerns that haven't been addressed yet?

hhirtz commented 9 months ago

Hi, sorry this took longer than needed. I'm fine with the idea.

As you said this breaks when default-features is false. I think we should address this. Currently rustc complains about bindgen being missing, which can confuse users. Maybe we can silence that and instead print a compile_error that explains that either vendored or default features must be enabled?

oisyn commented 9 months ago

I've addressed the issue. You now get a compile error when building without any features enabled explaining that you either need to enable "default" or "vendored".

oisyn commented 9 months ago

The generated bindings on my side use unsigned ints to represent enums, instead of signed ints in this PR. I guess this can change across different versions of clang?

Yeah that's a common problem with generating bindings for Rust. I think it's a Windows vs Linux thing.