Split off bindgen library into a sub-crate #204

jdub · 2016-11-04T09:57:17Z

Unfortunately there's a dependency on log via syntex_errors, so we
don't get rid of it all
I can't find a more sensible way to set dependencies based on whether
you're building the lib or bin
So --no-default-features means you need to know what you're doing,
as only the lib will build without the logging crates for now
The replacement log macros are pretty gross too, but they show a proof
of concept ;-)

jdub · 2016-11-04T09:57:28Z

As mentioned in #78.

emilio · 2016-11-04T13:11:43Z

FWIW, you can also probably get rid of syntex if you add an unstable feature to use aster with rustc's libsyntax. Codegen should work fine on nightly (maybe needs an aster update), and that should allow the #[no_std] use case you talked about.

metajack · 2016-11-04T16:26:23Z

cc @alexcrichton

I believe this is another case where we need rust-lang/cargo#1982

alexcrichton · 2016-11-04T19:57:24Z

Unfortunately that issue is unlikely to be fixed soon, so I'd recommend for now at least to do the refactoring to separate the binary/library into two cargo projects

jdub · 2016-11-04T21:34:34Z

Hmm, @emilio: How do you feel about @alexcrichton's suggestion?

It'd mean breaking one meaning of bindgen in Cargo.toml, and if all of these changes make it upstream, we'd be breaking one meaning of bindgen on crates.io:

Cargo.toml: How many users would depend on bindgen and use the binary in, e.g. build.rs? I understand from other conversations that servo doesn't use bindgen on-the-fly (instead generating code with the bindgen binary and checking it in), so this wouldn't affect servo. On first consideration, I'd expect most users referencing bindgen in their Cargo.toml to be using it as a library (or plugin).
crates.io: Either we'd break cargo install bindgen or every reference to bindgen in Cargo.toml.

Sounds like a lot of pain without a lot of gain.

My use case is exclusively nightly, so I'll give @emilio's suggestion a go before getting too excited about splitting crates.

metajack · 2016-11-04T21:36:55Z

Servo wants to move to running bindgen as part of the build, as does Stylo and probably Quantum Render. What we do now is pretty awful and requires access to every platform configuration to generate bindings to check in.

metajack · 2016-11-04T21:37:45Z

To add to that, we're probably willing to endure a lot of renaming pain, because the current situation is already extremely painful.

jdub · 2016-11-04T21:56:14Z

That gives us a lot of latitude!

In that case, I'd suggest breaking cargo install https://github.com/servo/rust-bindgen (and anyone who build-depends on it and uses the binary). Let's make this the best durn bindgen library we can, and have a sub-crate for the binary use case.

(My interest is in the build.rs library use case, so this dovetails nicely.)

@Yamakaky, could you weigh in with your thoughts on what we should do when it comes to bindgen on crates.io? I'm leaning towards favouring the cargo install bindgen binary use case (and breaking library / plugin users), but you know your users better than I. 😄

emilio

Please rebase instead of merging, you have a lot of unrelated commits in this branch.

In general the idea looks nice, but some setup scripts rely on the following to work:

$ git clone https://github.com/servo/rust-bindgen
$ cd rust-bindgen && cargo build
$ ./target/debug/bindgen foo

Updating those is probably not an issue, but... cc @bholley and @Manishearth.

In any case, this is not ready to land yet, needs a rebase and get CI working again.

I think instead of moving the binary, moving the library may be less-breaking in this case (this crate is not yet pusblished on crates.io, and I believe there aren't a lot of people using it as a library with a git dependency).

After this move, we could publish the library on crates.io so it's straight-forward to reference it.

emilio · 2016-11-05T11:25:23Z

bindgen_bin/src/main.rs

        Some(version) if version != expected_version => {
-            error!("Using clang {:?}, expected {:?}", version, expected_version);
+            println!("Using clang {:?}, expected {:?}",
+                   version,


nit: Alignment here looks odd.

jdub · 2016-11-05T12:13:05Z

Yep, this is very much work-in-progress, not ready to merge. I've been working on the test suite this afternoon.

I'll switch things around to have the library as sub-crate -- thanks.

Manishearth · 2016-11-05T15:29:06Z

Updating those is probably not an issue,

Fine with breaking that and doing a major bump.

jdub · 2016-11-05T15:46:05Z

Status:

bindgen root crate is the binary
libbindgen is the library
No version bumps yet
Rustified the test suite and updated the Travis / Makefile bits
Test suite runs on Windows now, but fails pretty hard (MSVC differences)
There are legit test suite failures, probably related to recent changes on master
17 failures on Linux, 37 on Windows
Test suite doesn't do batching yet
Despite how this PR started, I've focused on crate juggling, and haven't brought in the logless library changes! (yet?)

jdub · 2016-11-05T21:28:24Z

Corrections:

rust-bindgen.py was always passing --no-unstable-rust, so I've added that to the Rustified tests.
Turns out I was building with x86_64-pc-windows-gnu, not -msvc. (Haven't convinced MSVC to find Clang yet.)

1 failure on Linux, 20 on Windows.

bors-servo · 2016-11-06T09:25:27Z

☔ The latest upstream changes (presumably #215) made this pull request unmergeable. Please resolve the merge conflicts.

jdub · 2016-11-06T10:20:12Z

1 failure on MacOS, btw. Same as Linux: One of the module names in namespace.hpp doesn't appear to get the same (or cross-platform consistent) ID. From the new test suite output:

diff expected generated
--- expected: "/Users/jdub/src/rust-bindgen/libbindgen/tests/expectations/namespace.rs"
+++ generated from: "/Users/jdub/src/rust-bindgen/libbindgen/tests/headers/namespace.hpp"
 pub mod root {

... 8< ...

-    pub mod _bindgen_mod_id_13 {
+    pub mod _bindgen_mod_id_977 {

emilio · 2016-11-06T10:51:29Z

Hmm, which failures are we talking about on Windows? Id mismatches? Or binding generation/type layout failures?

jdub · 2016-11-06T11:10:03Z

Here's the libbindgen test log for a Windows build.

Integer repr differences
Mangling vs. #[link_name]
Lots of differences in jsval_layout_opaque.hpp
Occasional ID differences

Pretty sure it hasn't been tested on Windows though (or at least for ages), because the old tests attempted to run bindgen instead of bindgen.exe. 😄

bors-servo · 2016-11-06T11:18:55Z

☔ The latest upstream changes (presumably #216) made this pull request unmergeable. Please resolve the merge conflicts.

jdub · 2016-11-15T18:45:17Z

Never been so happy to see the number 13.

emilio · 2016-11-15T18:48:12Z

@bors-servo r+

I think with the new harness expectations aren't updated automatically right? Seems like an easy fix once this macro-patch lands.

bors-servo · 2016-11-15T18:48:13Z

📌 Commit 073b12f has been approved by emilio

bors-servo · 2016-11-15T18:48:15Z

⌛ Testing commit 073b12f with merge 6b285e6...

Split off bindgen library into a sub-crate - Unfortunately there's a dependency on log via syntex_errors, so we don't get rid of it all - I can't find a more sensible way to set dependencies based on whether you're building the lib or bin - So `--no-default-features` means you need to know what you're doing, as only the lib will build without the logging crates for now - The replacement log macros are pretty gross too, but they show a proof of concept ;-)

bors-servo · 2016-11-15T19:17:45Z

☀️ Test successful - status-travis

fitzgen · 2016-11-15T19:19:00Z

\o/

Thanks again @jdub!

@fitzgen

Generate a separate test function for every header - Header tests can now be run selectively, e.g. `cargo test header` will test all headers `cargo test union` will test headers with 'union' in the file name - The list of test functions is generated by `build.rs`, so never needs to be updated - Clever approach suggested by @fitzgen This PR depends on #204, so will include those changes until they're merged into master and this can be rebased. But I'll keep the commits separate.

emilio · 2016-11-15T19:23:04Z

Thanks a bunch for landing this @jdub! :)

jdub · 2016-11-15T19:24:45Z

Thanks to you both for being so helpful!

@emilio

Add an option to emit our ir for debugging Similar to our ability to emit the clang AST, this adds an option to emit our IR for debugging purposes. This can wait to land until after #204 is merged. r? @emilio

@emilio

Add an option to emit our ir for debugging Similar to our ability to emit the clang AST, this adds an option to emit our IR for debugging purposes. This can wait to land until after #204 is merged. r? @emilio

libbindgen: Make logging optional Note that the log crate isn't completely banished, as other is required by other dependencies. Believe it or not, this change was the original aim of #204.

highfive added the S-awaiting-review label Nov 4, 2016

jdub mentioned this pull request Nov 4, 2016

Re-introduce docopt #202

Closed

jdub changed the title ~~Rather disgusting hack to build without log dependencies~~ Split off bindgen binary into a sub-crate Nov 4, 2016

emilio reviewed Nov 5, 2016

View reviewed changes

jdub changed the title ~~Split off bindgen binary into a sub-crate~~ Split off bindgen library into a sub-crate Nov 5, 2016

jdub force-pushed the logless-library branch from c25768d to a5e0faf Compare November 5, 2016 15:37

highfive added the S-needs-rebase label Nov 6, 2016

jdub force-pushed the logless-library branch 2 times, most recently from 4d9fdc8 to eec7d27 Compare November 6, 2016 10:14

highfive added the S-needs-rebase label Nov 6, 2016

jdub mentioned this pull request Nov 6, 2016

Speed up integration tests (move building expectations) #216

Merged

jdub force-pushed the logless-library branch 2 times, most recently from 1c64974 to f4158e5 Compare November 6, 2016 13:08

jdub added 2 commits November 16, 2016 05:31

Document library usage

c555fcf

Remove plugin and its documentation

073b12f

jdub force-pushed the logless-library branch from 6823663 to 073b12f Compare November 15, 2016 18:31

highfive added S-awaiting-review and removed S-tests-failed labels Nov 15, 2016

highfive removed the S-awaiting-review label Nov 15, 2016

highfive added S-awaiting-merge and removed S-needs-rebase labels Nov 15, 2016

bors-servo merged commit 073b12f into rust-lang:master Nov 15, 2016

highfive removed the S-awaiting-merge label Nov 15, 2016

This was referenced Nov 15, 2016

Replace clang.rs iterator with generic boxed Map<Range<>> #213

Closed

clang::Cursor::args should return an Option<Vec<Cursor>> #207

Merged

jdub deleted the logless-library branch November 15, 2016 19:52

jdub mentioned this pull request Nov 15, 2016

Avoid superfluous repeated builds on Travis #265

Merged

jdub mentioned this pull request Nov 15, 2016

libbindgen: Make logging optional #269

Merged

emilio mentioned this pull request Nov 16, 2016

const field inside class is not handled correctly on Windows #271

Closed

jdub mentioned this pull request Nov 16, 2016

Travis build time jumped from 20 min to 1h30 min or more #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split off bindgen library into a sub-crate #204

Split off bindgen library into a sub-crate #204

jdub commented Nov 4, 2016

jdub commented Nov 4, 2016

emilio commented Nov 4, 2016 •

edited

Loading

metajack commented Nov 4, 2016

alexcrichton commented Nov 4, 2016

jdub commented Nov 4, 2016

metajack commented Nov 4, 2016

metajack commented Nov 4, 2016

jdub commented Nov 4, 2016

emilio left a comment

emilio Nov 5, 2016

jdub commented Nov 5, 2016

Manishearth commented Nov 5, 2016

jdub commented Nov 5, 2016

jdub commented Nov 5, 2016

bors-servo commented Nov 6, 2016

jdub commented Nov 6, 2016

emilio commented Nov 6, 2016

jdub commented Nov 6, 2016 •

edited

Loading

bors-servo commented Nov 6, 2016

jdub commented Nov 15, 2016

emilio commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

fitzgen commented Nov 15, 2016

emilio commented Nov 15, 2016

jdub commented Nov 15, 2016

Split off bindgen library into a sub-crate #204

Split off bindgen library into a sub-crate #204

Conversation

jdub commented Nov 4, 2016

jdub commented Nov 4, 2016

emilio commented Nov 4, 2016 • edited Loading

metajack commented Nov 4, 2016

alexcrichton commented Nov 4, 2016

jdub commented Nov 4, 2016

metajack commented Nov 4, 2016

metajack commented Nov 4, 2016

jdub commented Nov 4, 2016

emilio left a comment

Choose a reason for hiding this comment

emilio Nov 5, 2016

Choose a reason for hiding this comment

jdub commented Nov 5, 2016

Manishearth commented Nov 5, 2016

jdub commented Nov 5, 2016

jdub commented Nov 5, 2016

bors-servo commented Nov 6, 2016

jdub commented Nov 6, 2016

emilio commented Nov 6, 2016

jdub commented Nov 6, 2016 • edited Loading

bors-servo commented Nov 6, 2016

jdub commented Nov 15, 2016

emilio commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

bors-servo commented Nov 15, 2016

fitzgen commented Nov 15, 2016

emilio commented Nov 15, 2016

jdub commented Nov 15, 2016

emilio commented Nov 4, 2016 •

edited

Loading

jdub commented Nov 6, 2016 •

edited

Loading