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

Associated constants don't take the prefix into account properly #238

Closed
kvark opened this issue Oct 31, 2018 · 9 comments · Fixed by #240
Closed

Associated constants don't take the prefix into account properly #238

kvark opened this issue Oct 31, 2018 · 9 comments · Fixed by #240

Comments

@kvark
Copy link
Contributor

kvark commented Oct 31, 2018

#234 is missing a test for preffixed bindings. This is what we get:

typedef uint32_t WGPUTextureUsageFlags;
#define WGPUTextureUsageFlags_TRANSFER_DST (TextureUsageFlags){ .bits = 2 }

This doesn't compile, clearly, since there is no TextureUsageFlags.

cc @IGI-111

@IGI-111
Copy link
Contributor

IGI-111 commented Oct 31, 2018

I'm taking a look at this, will have to possibly add an alternative to LiteralExpr so that the prefixing can take place.

IGI-111 added a commit to IGI-111/cbindgen that referenced this issue Oct 31, 2018
Fix mozilla#238

This ensures that constants of a struct type have their type and the
type of their underlying value expressions renamed in the case of a
prefix.
eqrion pushed a commit that referenced this issue Nov 5, 2018
Fix #238

This ensures that constants of a struct type have their type and the
type of their underlying value expressions renamed in the case of a
prefix.
@kvark
Copy link
Contributor Author

kvark commented Nov 5, 2018

Thank you @IGI-111 !
I tried to test this today and faced other issues.
First of all, the #[repr(transparent)] isn't taken into account here, since the macro tries to assign the .bits field, and there are no fields on uint32_t.

Secondly, something strange is going on when I run wgpu-bindings:

kvark@ant /mnt/base/moz/wgpu $ cargo +nightly run --manifest-path wgpu-bindings/Cargo.toml
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s                                                                                                               
     Running `target/debug/wgpu-bindings`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: CargoExpand("wgpu-native", Compile("    Finished dev [unoptimized + debuginfo] target(s) in 0.07s\n"))', libcore/result.rs:1009:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

It looks like the compilation is successful, but something returns an error?

@IGI-111
Copy link
Contributor

IGI-111 commented Nov 5, 2018

Supporting #repr(transparent) should be pretty straightforward since the we're only interested in the first field. Open a separate issue and i'll look into it, no promises though since I didn't even know that was a thing. The more you know I guess.

As for that error message, which unwrap is failing here? Might be something completely unrelated. Can you post the backtrace?

@kvark
Copy link
Contributor Author

kvark commented Nov 5, 2018

It's the unwrap from this code on our side:

    cbindgen::Builder::new()
        .with_crate(source_dir)
        .with_config(config)
        .with_parse_expand(&["wgpu-native"])
        .generate()
        .unwrap()
        .write_to_file(crate_dir.join("wgpu.h"));

@IGI-111
Copy link
Contributor

IGI-111 commented Nov 5, 2018

That is quite a weird Err value indeed. I bet something just fails silently inside and the output is just misleading because it's missing a proper error message.

@kvark
Copy link
Contributor Author

kvark commented Nov 5, 2018

@IGI-111 how can I investigate this / provide more info?

@IGI-111
Copy link
Contributor

IGI-111 commented Nov 5, 2018

I'd start by figuring out where that Err is generated, and then finding the source of the failure, it's probably wise to pull out gdb here.

Maybe run cbindgen on only part of the offending codebase, or on all files individually? I mean with a relatively minimal example we could start to speculate on the cause.

Best you can do is open an issue with the smallest offending code you can find.

@grovesNL
Copy link
Contributor

grovesNL commented Nov 11, 2018

@kvark I don't know the reason, but running cargo clean and retrying seems to resolve it for me.

It seems like the command cbindgen runs for with_parse_expand causes the issue occasionally:

"/Users/josh/.rustup/toolchains/nightly-x86_64-apple-darwin/bin/cargo" "rustc" "--lib" "--manifest-path" "/Users/josh/Projects/wgpu-native/wgpu-native/Cargo.toml" "-p" "wgpu-native:0.1.0" "--" "-Z""unstable-options" "--pretty=expanded"

Sometimes this seems to produce empty output, and instead results in an exit status of 0 with that message in stderr and empty stdout. I'm not sure how to reproduce it consistently though, and cargo clean seems to have fixed it for me (at least at the moment).

Does anyone know why --pretty=expanded would not result in output for a successful compilation?

@grovesNL
Copy link
Contributor

grovesNL commented Nov 11, 2018

I'm able to reproduce this consistently now.

λ "/Users/josh/.rustup/toolchains/nightly-x86_64-apple-darwin/bin/cargo" "rustc" "--lib" "--manifest-path" "/Users/josh/Projects/wgpu-native/wgpu-native/Cargo.toml" "-p" "wgpu-native:0.1.0" "--" "-Z""unstable-options" "--pretty=expanded" > out1
   Compiling wgpu-native v0.1.0 (file:///Users/josh/Projects/wgpu-native/wgpu-native)
    Finished dev [unoptimized + debuginfo] target(s) in 0.94s
λ "/Users/josh/.rustup/toolchains/nightly-x86_64-apple-darwin/bin/cargo" "rustc" "--lib" "--manifest-path" "/Users/josh/Projects/wgpu-native/wgpu-native/Cargo.toml" "-p" "wgpu-native:0.1.0" "--" "-Z""unstable-options" "--pretty=expanded" > out2
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s

out1 contains the expanded source as expected.
out2 is empty.

This reminded me of behavior I noticed with rust-clippy, so I went looking for a similar issue. It seems we're blocked by the same issue as rust-lang/rust-clippy#2604, so macro expansion will only work when recompilation is required.

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 a pull request may close this issue.

3 participants