Produce portable output (avoid generating system-dependent tests + configuration to improve type replacements)

[dimbleby]

I prefer to generate my FFI code once and then distribute that, rather than generate it at build time. I see that you recommend the opposite, but even so: I expect that plenty of my potential users - especially on Windows - won't have the required dependencies installed. Better for me to figure out once how to get bindgen working, and be done with it.

At the moment, though, I have to do a little massaging of the code produced by bindgen to make it portable. My life would be that little bit easier if bindgen could be persuaded to produce portable output in the first place.

Ways in which I currently hand-modify output, and suggestions as to how this might be avoided:

• The exact definition of various types defined in system headers varies. So I blacklist them, and then modify the autogenerated code to import a portable version of those types eg use libc::timeval

  • WIBNI I could tell bindgen where to get a type from

• One consequence of this is that Debug can be wrongly derived. Eg it is derived on a structure containing an in_addr, which I obtain as libc::in_addr, which doesn't implement Debug.

  • (Actually libc::in_addr probably should be Debug - derive Debug on structures? libc#302).
  • But it isn't, so I have to manually un-derive Debug for structures that contain it
  • WIBNI bindgen didn't assume that things that it didn't know about (or which were blacklisted) were Debug

• autogenerated testcases all assume that pointer size is per the machine on which they were generated. So I delete them.

  • WIBNI I could tell bindgen to not generate such testcases

None of this is a big deal for me - bindgen is great and the work I just described really isn't very much. Feel free to close this out if it's not of interest to you!

[emilio]

So the autogenerated tests are useful for plenty of reasons (most notably sanity-check when you're using C++), but it's a fair concern and should be pretty easy to add an option to avoid generating them.

Re. deriving Debug, that's a bit more tricky, but it's a fair use case, so we could try to support that. Probably we could add configuration to avoid deriving debug for blacklisted types or something like that.

Do you have any thoughts @fitzgen?

[fitzgen]

An option to avoid layout tests is fine by me. Options surrounding control of deriving Debug seems fine by me.

The larger concept of intimately understanding portability and types in bindgen and the bindings we emit is a -1 for me. That sounds like too much complexity, and I think C++ and templates has already eaten our whole complexity budget.

[dimbleby]

Ha, yes, the title of this issue is maybe a bit grander than it ought to be!

What I'm actually interested in of course is just the three specific suggestions that I made. I'm very happy for you to consider them on their own merits - without anyone having to be sucked into some more general and complicated project.

[fitzgen]

Ok, then yeah those things all seem fine to me.

[emilio]

An option for disabling deriving debug landed today, disabling test generation should be straight forward.

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[fitzgen]

A good first bug even!

[jdm]

@andrew-x You might be interested in fixing this (it's a tool that is used to support Servo, rather than directly in Servo proper).

[andrew-x]

Hi @jdm I'd like to take a shot at this. I'll try to set up a development environment first then I'll likely come back with a bunch of questions.

[emilio]

@andrew-x that's awesome! Let us know when you need anything :)

[jdm]

@andrew-x Have you made any progress here? Any questions so far?

[andrew-x]

Hey sorry for radio silence. I was able to setup a dev environment (on windows 10) and I am able to successfully build with "cargo build", but when I try "cargo test" I get a bunch of fails.

I am also confused on the module and the issue so had a couple of clarification questions.

• what does bindgen do exactly? what output should I expect? I tried messing about with the files in the /bindgen-integration folder but I am still pretty confused. I also read through the README and CONTRIBUTING files.
• what is the current output and what is the output this issue expects?
• what would be a good jumping off point for this issue?

Sorry for these rather basic sounding questions, any help is appreciated!

[KiChjang]

@andrew-x Are you using the nightly version of rustc?

[jdm]

@andrew-x bindgen takes C/C++ header files as input, and spits out Rust FFI bindings that can be used to interact with the original C/C++ libraries.

[jdm]

Unassigning due to lack of activity.

[sadmansk]

Is this issue resolved? If not, I could work on it

[jdm]

There is still no way to disable test generation, so that would be great!

[sadmansk]

Can you tell me briefly what's needed to be done since #632 got merged?

[jdm]

Oh, my mistake. @fitzgen Is there anything left to do here?

[dimbleby]

Bullet 1 of my original report is still entirely open, and bullet 2 partially so.

For a little more context: here are the patches I currently apply to the bindgen-generated code:

• import some cross-platform types, per bullet 1 "WIBNI I could tell bindgen where to get a type from"
• (I'm not expecting bindgen to be able to cope with the platform-specific definition of ares_socket_t- though of course it'd be great if it could)
• (likewise I haven't reported the non-implementation of Default for ares_options - perhaps I should?)
• remove the faulty derivation of Debug for ares_addr_ttl: faulty per bullet 2

As you can see, this is a pretty small set of modifications to make to the autogen code so no big deal for me if you decided that it's more trouble than it's worth to worry about them.

Edit: oh, and I missed the adding of #![allow(non_camel_case_types, non_snake_case)]. This seems to me like something that bindgen could usefully do automatically?

[emilio]

So, a few points:

import some cross-platform types, per bullet 1 "WIBNI I could tell bindgen where to get a type from"

The idea is to call bindgen with --raw-line/raw_line to add those, you shouldn't need a patch to generate these. Ditto for ares_socket_t (if blacklisting isn't working for it for some reason, please say so, that's a bug).

(likewise I haven't reported the non-implementation of Default for ares_options - perhaps I should?)

We have an (opt-in) way to generate/derive Default. It's opt-in because it's not clear than zeroing the struct is an appropriate default for all C structs. If using that option doesn't derive it, then it's a bug. Otherwise, you can implement it manually from your crate outside of the generated file instead of patching the generated one.

remove the faulty derivation of Debug for ares_addr_ttl: faulty per bullet 2

We should add more control over how we treat blacklisted items, but that's not easy right now. We can definitely add an option to assume nothing about them, but even though it'd fit your use-case, I'm not sure it'd be generic enough.

Edit: oh, and I missed the adding of #![allow(non_camel_case_types, non_snake_case)]. This seems to me like something that bindgen could usefully do automatically?

That'd break workflows that include!() the bindgen file in something that isn't a standalone module. To fix that properly we'd need to merge something like #580 (and it's not clear to me the extra noise in the bindgen is worth it vs. requiring an extra raw_line). I'm interested into different opinions though, what do you think about the approach in #580?

[dimbleby]

Thanks! Summary of the below is: yes, you're quite right, let's not do anything more under the banner of #424.

In turn:

• I hadn't noticed --raw-line before

  • I think it'll be cleaner anyway for me to continue to apply a patch, than to add the lines one at a time from the bindgen command line
  • Maybe an option to prefix the contents of some file to the bindgen output would be more convenient to use?
  • But if you're happy with raw-line, that's fine by me

• Yeah, I don't know why I didn't think to blacklist ares_socket_t before.

  • possibly I was confused about the likely interaction between that and whitelisting ares_*
  • but it all works out fine
  • at which point this is just a special case of the previous bullet

• I'd also missed with-derive-default previously

  • on reflection I agree: I don't want that generally - it defaults too many things that might not make sense - and that I ought to implement the specific default implementation that I do want elsewhere
  • so no action here

• The faulty Debug is indeed awkward: and I'm inclined to agree that what I have might be a relatively unusual case

  • so again, 'no action' here is fine by me

• My instinct on full support non rust naming conventions of generated code #580 is that the change in generated code is just too ugly!

  • if this were my project, I'd lean towards 'thanks-but-no-thanks' on this as it currently stands
  • I feel as though there ought to be a better solution out there somewhere; but I don't know what it is...

[fitzgen]

I'm going to close this issue because it seems like we've reached consensus that there isn't anything left to do here.

If there is still stuff to do, I'd appreciate it if we opened new, more narrowly focused and well-defined follow up issues, because the scope here is a bit unclear to me.

Thanks!