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

Remove the syntex dependency #925

Closed
fitzgen opened this issue Aug 22, 2017 · 8 comments
Closed

Remove the syntex dependency #925

fitzgen opened this issue Aug 22, 2017 · 8 comments

Comments

@fitzgen
Copy link
Member

fitzgen commented Aug 22, 2017

It's deprecated. It has a long build time. We should get off of it.

I think the new hotness is syn (or maybe synom?) and quote?

@fitzgen
Copy link
Member Author

fitzgen commented Aug 23, 2017

I'm working on this.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 23, 2017

Unfortunately, it looks like using the quote crate doesn't result in pretty printed Rust code.

@dtolnay
Copy link
Member

dtolnay commented Aug 23, 2017

You can pass the output through rustfmt::format_input to make it pretty.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 23, 2017

Unfortunately, rustfmt is nightly only now, and bindgen can't be nightly only.

I don't think we want to stick to an old rustfmt (one that still worked on stable) either. That's no better than sticking with syntex.

We recently gained the ability to run the $PATH's rustfmt on the generated bindings, and I think we can rely on that functionality here. It's "just" a matter of CI and testing integration.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 24, 2017

This is hItting a bug in rustfmt that is making the formatted rust input invalid: rust-lang/rustfmt#1914

@fitzgen
Copy link
Member Author

fitzgen commented Aug 24, 2017

Can work around it by using a custom rustfmt.toml and passing it to the rustfmt invocation we use.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 29, 2017

Update: I have a branch without the syntex dependency that can generate bindings for all but one of our test headers (it just hangs in that one test header, and I haven't investigated it yet) and the emitted bindings have changed a little in inconsequential ways but are all passing their layout tests. There is also still some clean up to do and some constify module enums things.

Getting pretty close though.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 30, 2017

I have it passing all tests \o/

There is still some code clean up and minor rebasing to be done before this gets a PR.

fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Aug 31, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half!

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Aug 31, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Aug 31, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
@fitzgen fitzgen mentioned this issue Aug 31, 2017
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Sep 1, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Sep 1, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Sep 5, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
fitzgen added a commit to fitzgen/rust-bindgen that referenced this issue Sep 7, 2017
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes rust-lang#925
bors-servo pushed a commit that referenced this issue Sep 7, 2017
No more syntex

There are a few commits in this PR, but the big one is the commit that goes `syntex` -> `quote` for code generation. I've included a copy of its commit message below.

I tried to verify that it works with the stylo build still, but there were some issues, and then I checked with master, and that wasn't working either. So now I'm C-Reducing the failures on master and figured that this is at least ready for feedback in the meantime.

r? @emilio

----------------------------

The `syntex` crate is unmaintained. It is slow to build, and additionally it requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen` crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained, and builds faster. It doesn't have a typed API or builders, however; it only deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside `bindgen::ir::BindgenContext::gen` that was used for the old `syntex` context. Now `BindgenContext` doesn't have a lifetime parameter either. This should make it easier to revisit doing our analyses in parallel with `rayon`, since that context was one of the things that made it hard for `BindgenContext` to implement `Sync`.

Fixes #925
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants