-
Notifications
You must be signed in to change notification settings - Fork 280
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
Investigate use of bindgen for producing bindings #775
Comments
Oh, good point that if someone is linking non-vendored library there's no guarantee that headers will match. Perhaps we should require bindgen at build time there. I meant this: https://github.com/matklad/cargo-xtask |
openssl-sys makes it optional somehow. We should check what they do. |
Ah, I see. So contributors of unrelated things will be "aware of" these xtasks, in the sense that their deps will pollute their lockfile and maybe they'll show up on command-line autocomplete. But they can still ignore them if they want to, assuming we've committed their output. In some ways it's even less annoying than a dev-dependency. |
Well, that's the question. Should we? Given there's lot of unrelated stuff going on I think we should even though it disagrees with my "don't commit computer-generated stuff" stance. dev-dependency would be wrong in any case. |
I think we should -- I feel much less strongly about "don't commit computer-generated stuff". (In fact I think the opposite, as long as we have lints that check that the computer-generated stuff is consistent.) But -- this is a case where the computer-generated stuff is potentially system-specific, which is a situation where you don't want it committed. So I'm also unsure. But I'm leaning toward "yes, for now" since we currently have everything committed, and if we move toward bindgen we should try to do it in a minimally disruptive way. |
There's not really that much system-specific for the vendored library. I still hope to do the max align thing at some point but that's orthogonal. |
Ok. Let's commit the output. We can revisit after we've gotten it working. |
Earlier discussion: #716 (comment)
The recommended way of using bindgen is to add it as a dependency and to use it from build.rs when building the crate. This has the advantaged of producing the correct bindings for the system that is building rust-secp, which may differ from system to system because C is a poorly specified language.
This has the disadvantage of adding
bindgen
and its dep tree to our dep tree, and it's not clear how valuable the advantage is given that nobody has complained about our bindings being wrong yet. (And in fact, we have hardcoded some C->Rust mappings in our secp256k1-sys/src/types.rs, because Rust stdlib didn't provide them until very recently, which you'd expect to exacerbate this sort of thing. But AFACT it hasn't.) And people are using Rust on weird systems -- we get issues about broken tools and missing headers etc.So I would like to have a script to use bindgen, commit the output, and check it in CI. Kix alternately suggests the "xtask pattern" which I believe refers to this and which he also referenced in August. I read its README then and read it again now and still have no idea what it is supposed to be. However, Kix suggests that when using this pattern it would require contributors to run bindgen/xtask "even for unrelated changes". Presumably we could fix that by committing more stuff.
The text was updated successfully, but these errors were encountered: