rustup: wrap bindgen to find header files in a nix-shell #45330

symphorien · 2018-08-19T13:21:19Z

Motivation for this change

this is a second attempt at #40746
it should work in any nix-shell where the header file is correctly parsed by clang, as long as -x c++ is
explicitly specified for c++ headers.

This is done in two commits: the first one is the one which wraps bindgen. The second one is an attempt at making tests works. Around 190/300 pass, but the test suite requires nightly rustfmt so most if not all failures are just a question of formatting :(
This second commit is therefore unnecessary, but when we update rustc or when rustfmt becomes stable it may prove useful. I was not sure whether to leave it, so if you think I should remove it from the PR just tell me.

About closure size: to have NIX_CXXSTDLIB_COMPILE populated, the only non absurd way I found was to add clang as propagatedBuildInputs. This is wasteful, but I could not get libstdcxxHook to work.

Things done

[] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- [] NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

symphorien · 2018-08-19T13:23:22Z

cc @Ralith as maintainer

Ralith · 2018-08-19T19:26:40Z

Ideally, the injected arguments to clang should be added immediately after the -- if one was present in the user's arguments, so as not to inadvertently silently clobber anything. I'm also not sure it's correct to be parsing for -x c++ both before and after the --. Stable bindgen seems to be broken right now (libclang.so no longer exists, apparently?) so I haven't double-checked, but I assume there's only one place where that's supposed to appear.

symphorien · 2018-08-20T19:09:58Z

Ideally, the injected arguments to clang should be added immediately after the --

I agree, but this is beyond my bash skills. Anyway, we only add -isystem arguments when the user should only use -I so they commute most of the time (as far as the "headers only" usage of bindgen is concerned)

I'm also not sure it's correct to be parsing for -x c++ both before and after the --

-x is not a valid bindgen option so this is fine. This helps keep the wrapper short and simple.

(libclang.so no longer exists, apparently?)

its was moved from clang-unwrapped.lib to libclang.lib.

I can solve the parsing of -x before -- this week if you wish so, but please tell me if you want to keep the second commit before. This would make the git manipulations easier :)

Ralith · 2018-08-25T17:26:30Z

Sorry for the delay getting back to you; this fell off my stack for a while.

I agree, but this is beyond my bash skills.

Can you add leave a comment explaining that this behavior is unintended instead, then?

Anyway, we only add -isystem arguments when the user should only use -I so they commute most of the time (as far as the "headers only" usage of bindgen is concerned)

It could still be an issue with e.g. -D/-U, though admittedly that's likely rare. The PR as-is will certainly be a drastic improvement regardless.

-x is not a valid bindgen option so this is fine. This helps keep the wrapper short and simple.

The concern is that might change in the future without any of us noticing.

please tell me if you want to keep the second commit

Seems like a good thing to have, unless there's prospects of upstream making the tests more portable themselves.

symphorien · 2018-08-27T15:30:30Z

Can you add leave a comment explaining that this behavior is unintended instead, then?

Done

The concern is that might change in the future without any of us noticing.

Fixed

I squashed everything together.

Ralith

Thanks! Big step in the right direction.

Mic92 · 2018-08-28T13:05:33Z

I get an error message, maybe you can clarify what happens:

$ nix-review pr 45330
nix-shell> bindgen
error: header '-isystem' does not exist. # <- is this error supposed to happen?
thread 'main' panicked at 'Unable to generate bindings: ()', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

symphorien · 2018-08-28T15:20:23Z

huh it seems it is bindgen's fault.

$ LIBCLANG_PATH=/nix/store/psgsm82jh18jg3k7m6lyff2ai300aqxz-clang-5.0.2-lib/lib /nix/store/b1m3bmckzk9axjbh0230hmhg3c37vwqk-rust-bindgen-0.37.0/bin/.bindgen-wrapped -- -isystem blah
error: header '-isystem' does not exist.
thread 'main' panicked at 'Unable to generate bindings: ()', libcore/result.rs:945:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic_fmt
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::result::unwrap_failed
   9: std::panicking::try::do_call
  10: __rust_maybe_catch_panic
  11: bindgen::main
  12: std::rt::lang_start::{{closure}}
  13: std::panicking::try::do_call
  14: __rust_maybe_catch_panic
  15: std::rt::lang_start_internal
  16: main
  17: __libc_start_main
  18: _start

symphorien · 2018-08-28T15:41:45Z

Fixed by a small patch which I sent upstream: rust-lang/rust-bindgen#1376

$ bindgen 
error: The following required arguments were not provided:
    <header>

USAGE:
    bindgen [FLAGS] [OPTIONS] <header> -- <clang-args>...

For more information try --help

symphorien · 2018-08-28T15:46:17Z

Hum for some reason the UI of github didn't take my push force into account: the PR is now symphorien@33969e2

(edit: fail paste)

Mic92 · 2018-08-28T16:12:33Z

pkgs/development/tools/rust/bindgen/default.nix

+  patches = [
+    # https://github.com/rust-lang-nursery/rust-bindgen/pull/1376
+    (fetchpatch {
+      url = https://github.com/rust-lang-nursery/rust-bindgen/pull/1376/commits/c8b5406f08af82a92bf8faf852c21ba941d9c176.patch;


If they ask you to update the pull request, would this patch url still be valid?

right it would disappear. I just tested with the precedent hash of this PR

apparently https://github.com/nixos/nixpkgs/commit/db096988b8c.patch is still available

The easy part is to add NIX_CFLAGS_COMPILE for "regular" libraries. A bit more tricky is to add the required flags for libclang to find libstdcxx. For this we parse arguments to bindgen to look for -x c++ or -xc++ and if found add NIX_CXXSTDLIB_COMPILE to the arguments. This variable is populated by a complex dance of setupHooks. We trigger this by adding clang to propagatedBuildInputs. A more subtle way may exist.

options: mark clang-args last before, `bindgen -- -I blah` would try to open `-I` as a header file, leading to a very confusing error message. The "real" use case behind this is described here: NixOS/nixpkgs#45330 I have checked that these ways of passing args still work/error as expected: ``` bindgen a.h bindgen a.h -- bindgen a.h -- -Dblah=1 bindgen -- -Dblah=1 bindgen -- bindgen ```

symphorien · 2018-08-30T11:29:06Z

The PR was merged so the commit should not disappear anymore.

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Aug 19, 2018

symphorien force-pushed the bindgen2 branch 2 times, most recently from 8dbb33d to db09698 Compare August 27, 2018 15:27

Ralith approved these changes Aug 27, 2018

View reviewed changes

Mic92 changed the title ~~Wrap bindgen to find header files in a nix-shell~~ rustup: wrap bindgen to find header files in a nix-shell Aug 28, 2018

symphorien mentioned this pull request Aug 28, 2018

options: mark clang-args last rust-lang/rust-bindgen#1376

Merged

symphorien force-pushed the bindgen2 branch from db09698 to 33969e2 Compare August 28, 2018 15:45

Mic92 reviewed Aug 28, 2018

View reviewed changes

symphorien force-pushed the bindgen2 branch from 33969e2 to 4c4fc22 Compare August 28, 2018 16:31

Mic92 merged commit da11a26 into NixOS:master Aug 30, 2018

symphorien deleted the bindgen2 branch March 21, 2020 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustup: wrap bindgen to find header files in a nix-shell #45330

rustup: wrap bindgen to find header files in a nix-shell #45330

symphorien commented Aug 19, 2018

symphorien commented Aug 19, 2018

Ralith commented Aug 19, 2018

symphorien commented Aug 20, 2018

Ralith commented Aug 25, 2018

symphorien commented Aug 27, 2018

Ralith left a comment

Mic92 commented Aug 28, 2018 •

edited

symphorien commented Aug 28, 2018

symphorien commented Aug 28, 2018

symphorien commented Aug 28, 2018 •

edited

Mic92 Aug 28, 2018 •

edited

symphorien Aug 28, 2018

symphorien Aug 28, 2018

symphorien commented Aug 30, 2018

rustup: wrap bindgen to find header files in a nix-shell #45330

rustup: wrap bindgen to find header files in a nix-shell #45330

Conversation

symphorien commented Aug 19, 2018

Motivation for this change

Things done

symphorien commented Aug 19, 2018

Ralith commented Aug 19, 2018

symphorien commented Aug 20, 2018

Ralith commented Aug 25, 2018

symphorien commented Aug 27, 2018

Ralith left a comment

Choose a reason for hiding this comment

Mic92 commented Aug 28, 2018 • edited

symphorien commented Aug 28, 2018

symphorien commented Aug 28, 2018

symphorien commented Aug 28, 2018 • edited

Mic92 Aug 28, 2018 • edited

Choose a reason for hiding this comment

symphorien Aug 28, 2018

Choose a reason for hiding this comment

symphorien Aug 28, 2018

Choose a reason for hiding this comment

symphorien commented Aug 30, 2018

Mic92 commented Aug 28, 2018 •

edited

symphorien commented Aug 28, 2018 •

edited

Mic92 Aug 28, 2018 •

edited