libnitrokey 3.6 has been released, and I will soon update
nitrokey-sys-rs. But there are two issues that I would like to discuss
before this release. The first one is the bindgen strategy:
bindgen supports two operation modes. The recommended mode is adding
the bindgen crate as a build dependency, invoking it from the build.rs
script and generating the Rust source using the system settings at build
time. This makes sure that all types are correct for the build
platform, but it increases the compile time significantly and adds
several new dependencies. It’s also harder to track the differences
between releases because there is no version control for the generated
code.
The other option is calling bindgen as a command-line tool and adding
the generated source code to the published crate. This only works if
the generated code is platform-independent.
When I started nitrokey-sys-rs, I used the recommended approach, added
bindgen as a build dependency and called it from build.rs [0]. Later
on, I decided to go with the second approach and include the generated
code in the crate source code [1] because I wanted to reduce the compile
time. I found no platform-specific code in the generated Rust code, so
I assumed it would be fine.
Apparently, I was only looking for types like size_t and missed that
pointers are also platform-specific.
[0] https://git.ireas.org/nitrokey-sys-rs/tree/build.rs?id=85ce1e4d5090caec42293d4b1746bc815095c8a2
[1] https://git.ireas.org/nitrokey-sys-rs/commit/?id=f424891511b79e2ec1fb8e1dcb407228da693316
As I packaged nitrokey-sys-rs for Debian, I noticed this problem as the
build for the powerpc platform failed [2]. We solved this issue by
adding a patch [3] that invokes bindgen in build.rs. There was no
libnitrokey release since then, so I haven’t had the opportunity to
address it upstream.
[2] https://buildd.debian.org/status/fetch.php?pkg=rust-nitrokey-sys&arch=powerpc&ver=3.5.0-1&stamp=1579046648&raw=0
[3] https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/nitrokey-sys/debian/patches/use-bindgen.diff
To fix this oversight, I plan to switch back to the recommended bindgen
usage in build.rs. But as this pulls in some additional dependencies
and increases the build time, I wanted to bring this up here first. Are
you okay with the change?
Best,
Robin
Thanks for bringing this up, Robin. Let me prefix my response by saying
that I am pretty much not familiar with bindgen. I've never created a
project that uses it (I just read through the whirlwind tutorial in the
hope that I won't be b/sing too much).
In general I am more in favor of the current approach where the
generated source code is checked in, mostly for reasons you already
outlined:
1) faster compilation
2) the code is checked in and easier to inspect
3) fewer dependencies; though I guess it would be a build time
dependency and I don't care much about those; at least not if they are
Rust crates; in this case, though, would we require clang then at build
time? that would indeed be significant, as I don't believe that it is a
requirement for Rust itself
But having said that, I am not actually sure I understand the issue with
this approach to begin with. When you say that pointers are platform
specific, what exactly do you mean? I suppose they do have different
sizes, but (why) is that something of relevance? Shouldn't the code just
work? Bindgen should create Rust type definitions that match C types.
Rust in turn lays them out in memory as a C compiler would (repr(C)).
Platform/OS/toolchain dependent types should use their corresponding
counterparts in Rust land and everything should match up.
And so looking at the Power PC failure [2] and the test code failing, I
am not sure I follow why the tests are the way they are. My
understanding is that they are created by bindgen and not our making,
but I don't really get why they test what they test: the generated code
is *generic* Rust while the checks assume execution on a certain
platform for the results to match expectation, right? So really to me it
seems as if the tests are overly prescriptive. The non-test code
*should* work on a different platform (assuming no #ifdefs for a
specific toolchain/OS/architecture and other funkiness in the C code, of
course; but you mentioned you already verified that).
So the way I see it, instead of
```
#[test]
fn bindgen_test_layout_NK_device_info() {
assert_eq!(
::std::mem::size_of::<NK_device_info>(),
32usize,
concat!("Size of: ", stringify!(NK_device_info))
);
...
```
what should be emitted is (roughly):
```
[target.'cfg(target_arch = "amd64")']
#[test]
fn bindgen_test_layout_NK_device_info() {
assert_eq!(
::std::mem::size_of::<NK_device_info>(),
32usize,
concat!("Size of: ", stringify!(NK_device_info))
);
...
[target.'cfg(target_arch = "powerpc")']
#[test]
fn bindgen_test_layout_NK_device_info() {
assert_eq!(
::std::mem::size_of::<NK_device_info>(),
16usize,
concat!("Size of: ", stringify!(NK_device_info))
);
...
```
Now I am sure that I am not the first one to have that idea, and I
suspect there may be reasons for why that is not being done (a likely
candidate that it's just not the preferred use case). But perhaps a good
next step would be (assuming my reasoning above is correct), to check
whether the reasons do apply to us. If not, then, perhaps, we could just
adjust the code this way (the code already seems to be manually
modified)? (of course, I don't have Power PC system, so that could get
fiddly...)
Alternatively, and I understand that may be a controversial opinion, we
could also remove the tests: in my opinion they test invariants of
bindgen and the Rust compiler, which in turn should have tests on their
own (no argument that they are nice to have and probably fast running
and everything, but right now they are also in the way). In the same way
that we are not (directly) testing that a C compiler emits certain code
for a function we wrote, we should be able to trust the toolchains we
use to do the right thing.
And it's not that we solely trust blindly: we do have higher level tests
that would test these paths. Granted, they are in a different crate
right now and that's not great.
But, lastly, let me also point out the following: even in the same arch
& OS you could have differently sized types (see
https://en.wikipedia.org/w/index.php?title=64-bit_computing&oldid=975604853#64-bit_data_models).
So, I believe that bindgen is already guessing (I'd guess that can be
controlled somewhere, but we don't seem to do that) and making implicit
assumptions about the library (namely with which compiler it got
compiled). These assumption may or may not hold and violations of them
would *not* be caught by these tests. That, by the way, seems to be
acknowledged by the tutorial at least:
> And we can run cargo test to verify that the layout, size, and
alignment of our generated Rust FFI structs match **what bindgen thinks
they should be**
([4]; emphasis mine)
Let me know whether you agree with some of my thinking.
Daniel
[4] https://rust-lang.github.io/rust-bindgen/tutorial-4.html
On 9/19/20 11:24 AM, Robin Krahl wrote:
> libnitrokey 3.6 has been released, and I will soon update> nitrokey-sys-rs. But there are two issues that I would like to discuss> before this release. The first one is the bindgen strategy:>> bindgen supports two operation modes. The recommended mode is adding> the bindgen crate as a build dependency, invoking it from the build.rs> script and generating the Rust source using the system settings at build> time. This makes sure that all types are correct for the build> platform, but it increases the compile time significantly and adds> several new dependencies. It’s also harder to track the differences> between releases because there is no version control for the generated> code.>> The other option is calling bindgen as a command-line tool and adding> the generated source code to the published crate. This only works if> the generated code is platform-independent.>> When I started nitrokey-sys-rs, I used the recommended approach, added> bindgen as a build dependency and called it from build.rs [0]. Later> on, I decided to go with the second approach and include the generated> code in the crate source code [1] because I wanted to reduce the compile> time. I found no platform-specific code in the generated Rust code, so> I assumed it would be fine.>> Apparently, I was only looking for types like size_t and missed that> pointers are also platform-specific.>> [0] https://git.ireas.org/nitrokey-sys-rs/tree/build.rs?id=85ce1e4d5090caec42293d4b1746bc815095c8a2> [1] https://git.ireas.org/nitrokey-sys-rs/commit/?id=f424891511b79e2ec1fb8e1dcb407228da693316>> As I packaged nitrokey-sys-rs for Debian, I noticed this problem as the> build for the powerpc platform failed [2]. We solved this issue by> adding a patch [3] that invokes bindgen in build.rs. There was no> libnitrokey release since then, so I haven’t had the opportunity to> address it upstream.>> [2] https://buildd.debian.org/status/fetch.php?pkg=rust-nitrokey-sys&arch=powerpc&ver=3.5.0-1&stamp=1579046648&raw=0> [3] https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/nitrokey-sys/debian/patches/use-bindgen.diff>> To fix this oversight, I plan to switch back to the recommended bindgen> usage in build.rs. But as this pulls in some additional dependencies> and increases the build time, I wanted to bring this up here first. Are> you okay with the change?
Hi Daniel,
thanks for the detailed response!
On 2020-09-19 15:27:06, Daniel Müller wrote:
> 3) fewer dependencies; though I guess it would be a build time> dependency and I don't care much about those; at least not if they are> Rust crates; in this case, though, would we require clang then at build> time? that would indeed be significant, as I don't believe that it is a> requirement for Rust itself
Yes, it would be build dependencies, and yes, clang and libclang.so are
required. The additional Rust dependencies (first level) would be
cexpr, clang-sys, lazycell, peeking_take_while, rustc_hash and shlex.
> But having said that, I am not actually sure I understand the issue with> this approach to begin with. When you say that pointers are platform> specific, what exactly do you mean?
Sorry for not making this clear. I had a more detailed explanation in a
previous version of my mail, but removed it as it became too long. Yes,
the problem that I noticed and that triggered the build failure is
caused by the layout tests generated by bindgen that assume the size of
the pointer.
> the generated code is *generic* Rust while the checks assume execution> on a certain platform for the results to match expectation, right?
It depends on your point of view. If a struct has different sizes on
different platforms, it has platform-specific features. But I think the
main point of the tests is to validate that rustc does what bindgen
expects it to do, and this can only be checked per platform.
> So really to me it> seems as if the tests are overly prescriptive. The non-test code> *should* work on a different platform (assuming no #ifdefs for a> specific toolchain/OS/architecture and other funkiness in the C code, of> course; but you mentioned you already verified that).> > So the way I see it, instead of> ```> #[test]> fn bindgen_test_layout_NK_device_info() {> assert_eq!(> ::std::mem::size_of::<NK_device_info>(),> 32usize,> concat!("Size of: ", stringify!(NK_device_info))> );> ...> ```> > what should be emitted is (roughly):> ```> [target.'cfg(target_arch = "amd64")']> #[test]> fn bindgen_test_layout_NK_device_info() {> assert_eq!(> ::std::mem::size_of::<NK_device_info>(),> 32usize,> concat!("Size of: ", stringify!(NK_device_info))> );> ...> > [target.'cfg(target_arch = "powerpc")']> #[test]> fn bindgen_test_layout_NK_device_info() {> assert_eq!(> ::std::mem::size_of::<NK_device_info>(),> 16usize,> concat!("Size of: ", stringify!(NK_device_info))> );> ...> ```> > Now I am sure that I am not the first one to have that idea, and I> suspect there may be reasons for why that is not being done (a likely> candidate that it's just not the preferred use case).
See also:
https://github.com/rust-lang/rust-bindgen/issues/1213> But perhaps a good next step would be (assuming my reasoning above is> correct), to check whether the reasons do apply to us. If not, then,> perhaps, we could just adjust the code this way (the code already> seems to be manually modified)? (of course, I don't have Power PC> system, so that could get fiddly...)
Currently, the generated code is modified by removing unnecessary
items (per default, bindgen generates everything it sees) and by
adding deprecation flags for deprecated functions. I don’t remember why
I chose to manually remove the unnecessary items: I could just have
whitelisted NK_.*. I changed nitrokey-sys-rs to automatically generate
the bindings and apply patches using a Makefile [0] and quilt [1].
[0] https://git.sr.ht/~ireas/nitrokey-sys-rs/commit/7394a6b188c2a6c0d5ae72f8bba5fddc9720613b
[1] https://git.sr.ht/~ireas/nitrokey-sys-rs/commit/2b367a30df6e155621c116c41cc262dd96ad1a33
I’m open to continue to perform small manual changes, but duplicating
all tests for all architectures is not really maintainable.
> Alternatively, and I understand that may be a controversial opinion, we> could also remove the tests: in my opinion they test invariants of> bindgen and the Rust compiler, which in turn should have tests on their> own (no argument that they are nice to have and probably fast running> and everything, but right now they are also in the way). In the same way> that we are not (directly) testing that a C compiler emits certain code> for a function we wrote, we should be able to trust the toolchains we> use to do the right thing.
bindgen is much less mature and stable than the Rust compiler, and the
Rust compiler is much less mature and stable than gcc, clang etc. Also,
nitrokey-sys-rs’ only task is code generation, so I think it is fair to
test that the code is generated properly.
> And it's not that we solely trust blindly: we do have higher level tests> that would test these paths. Granted, they are in a different crate> right now and that's not great.
Would they? What kind of error would a layout mismatch cause? If it’s
a compile time error, we would probably notice it. But if it just
causes corrupted values at runtime, it might be more tricky.
> But, lastly, let me also point out the following: even in the same arch> & OS you could have differently sized types (see> https://en.wikipedia.org/w/index.php?title=64-bit_computing&oldid=975604853#64-bit_data_models).> So, I believe that bindgen is already guessing (I'd guess that can be> controlled somewhere, but we don't seem to do that) and making implicit> assumptions about the library (namely with which compiler it got> compiled). These assumption may or may not hold and violations of them> would *not* be caught by these tests. That, by the way, seems to be> acknowledged by the tutorial at least:> > > And we can run cargo test to verify that the layout, size, and> alignment of our generated Rust FFI structs match **what bindgen thinks> they should be**> > ([4]; emphasis mine)
True, but as far as I know, rustc/clang/LLVM is the only Rust compiler.
And from my understanding, the point of the tests is not to validate
that the compiled library and the Rust types have the same layout, but
that the Rust compiler layed out the types as bindgen expected it.
Anyway, what do you think about this solution: We continue to generate
ffi.rs off-line, but we add
#[cfg(target_pointer_width = "64")]
to the test cases and we advise users to re-generate and validate the
bindings and the tests for platforms with a different pointer width.
/Robin
On 9/20/20 9:30 AM, Robin Krahl wrote:
> On 2020-09-19 15:27:06, Daniel Müller wrote:>> So really to me it>> seems as if the tests are overly prescriptive. The non-test code>> *should* work on a different platform (assuming no #ifdefs for a>> specific toolchain/OS/architecture and other funkiness in the C code, of>> course; but you mentioned you already verified that).> > See also:> https://github.com/rust-lang/rust-bindgen/issues/1213
Thanks for digging that out! That basically confirms my suspicion.
>> And it's not that we solely trust blindly: we do have higher level tests>> that would test these paths. Granted, they are in a different crate>> right now and that's not great.> > Would they? What kind of error would a layout mismatch cause? If it’s> a compile time error, we would probably notice it. But if it just> causes corrupted values at runtime, it might be more tricky.
A runtime error. I don't think you can detect layout mismatches
properly, short of parsing the compiled library, so corruption is all
you get. And as I said, I don't think the current set of tests should
give you even that much confidence. They just test a different thing
altogether (in my opinion less valuable [though of course still having a
place for the reasons you outlined]).
>> But, lastly, let me also point out the following: even in the same arch>> & OS you could have differently sized types (see>> https://en.wikipedia.org/w/index.php?title=64-bit_computing&oldid=975604853#64-bit_data_models).>> So, I believe that bindgen is already guessing (I'd guess that can be>> controlled somewhere, but we don't seem to do that) and making implicit>> assumptions about the library (namely with which compiler it got>> compiled). These assumption may or may not hold and violations of them>> would *not* be caught by these tests. That, by the way, seems to be>> acknowledged by the tutorial at least:>>>>> And we can run cargo test to verify that the layout, size, and>> alignment of our generated Rust FFI structs match **what bindgen thinks>> they should be**>>>> ([4]; emphasis mine)> > True, but as far as I know, rustc/clang/LLVM is the only Rust compiler.> And from my understanding, the point of the tests is not to validate> that the compiled library and the Rust types have the same layout, but> that the Rust compiler layed out the types as bindgen expected it.
It's not about the Rust compiler but the C compiler that has been used
to compile the library being wrapped. And on Windows, most notably, you
can (hypothetically) have libnitrokey compiled with MS Visual C++ or
Cygwin and get differently sized integers. So if libnitrokey uses a long
somewhere in a public function and it got compiled with MS VC++, it
would be a 32 bit value. Interface with that from LLVM & Rust (which I
assume uses LP64; compile the lib with Cygwin if it's the other way
around) and you have your mismatch.
> Anyway, what do you think about this solution: We continue to generate> ffi.rs off-line, but we add> #[cfg(target_pointer_width = "64")]> to the test cases and we advise users to re-generate and validate the> bindings and the tests for platforms with a different pointer width.
I think that would be great! But for my understanding: why not just have
a #[cfg(target_pointer_width = "32")] path as well and make the code
fully reusable?
That would then more easily give us the ability to verify your claim
that pointers are the only variable between platforms (and not, say,
size_t, int, long, etc).
Is the main worry the maintenance effort when creating a new release?
Daniel
On 2020-09-20 10:06:22, Daniel Müller wrote:
> > True, but as far as I know, rustc/clang/LLVM is the only Rust compiler.> > And from my understanding, the point of the tests is not to validate> > that the compiled library and the Rust types have the same layout, but> > that the Rust compiler layed out the types as bindgen expected it.> > It's not about the Rust compiler but the C compiler that has been used> to compile the library being wrapped. And on Windows, most notably, you> can (hypothetically) have libnitrokey compiled with MS Visual C++ or> Cygwin and get differently sized integers. So if libnitrokey uses a long> somewhere in a public function and it got compiled with MS VC++, it> would be a 32 bit value. Interface with that from LLVM & Rust (which I> assume uses LP64; compile the lib with Cygwin if it's the other way> around) and you have your mismatch.
I think we’re talking about different things. From my understanding,
there are two different error sources:
1. bindgen might have wrong assumptions about the layout of the types in
the compiled native code. We can’t catch this with tests because
bindgen only looks at the headers. This is the problem that you
describe.
2. bindgen might have wrong assumptions about how Rust lays out its data
structures. This is what the unit tests check, and this is
independent of the compiler used for the native library (but still
platform-specific).
> > Anyway, what do you think about this solution: We continue to generate> > ffi.rs off-line, but we add> > #[cfg(target_pointer_width = "64")]> > to the test cases and we advise users to re-generate and validate the> > bindings and the tests for platforms with a different pointer width.> > I think that would be great! But for my understanding: why not just have> a #[cfg(target_pointer_width = "32")] path as well and make the code> fully reusable?
Sure, that would be nice. But I only want to add and maintain tests
that I can execute and verify, at least once to make sure that their
assumptions are correct. As far as I see, this would involve setting up
cross compilation and something like qemu to execute the tests, right?
And there might be some exotic platforms with other pointer sizes than
32-bit or 64-bit.
If you have the option to execute the 32-bit tests, I can add them to
the patch series and include them in future releases.
> That would then more easily give us the ability to verify your claim> that pointers are the only variable between platforms (and not, say,> size_t, int, long, etc).
To be clear: Of course, size_t, int, and long vary. But unless I
missed something, libnitrokey only uses sized int types in structs.
/Robin
On 9/20/20 11:27 AM, Robin Krahl wrote:
> On 2020-09-20 10:06:22, Daniel Müller wrote:>> It's not about the Rust compiler but the C compiler that has been used>> to compile the library being wrapped. And on Windows, most notably, you>> can (hypothetically) have libnitrokey compiled with MS Visual C++ or>> Cygwin and get differently sized integers. So if libnitrokey uses a long>> somewhere in a public function and it got compiled with MS VC++, it>> would be a 32 bit value. Interface with that from LLVM & Rust (which I>> assume uses LP64; compile the lib with Cygwin if it's the other way>> around) and you have your mismatch.> > I think we’re talking about different things. From my understanding,> there are two different error sources:> 1. bindgen might have wrong assumptions about the layout of the types in> the compiled native code. We can’t catch this with tests because> bindgen only looks at the headers. This is the problem that you> describe.> 2. bindgen might have wrong assumptions about how Rust lays out its data> structures. This is what the unit tests check, and this is> independent of the compiler used for the native library (but still> platform-specific).
Yep, exactly.
>>> Anyway, what do you think about this solution: We continue to generate>>> ffi.rs off-line, but we add>>> #[cfg(target_pointer_width = "64")]>>> to the test cases and we advise users to re-generate and validate the>>> bindings and the tests for platforms with a different pointer width.>>>> I think that would be great! But for my understanding: why not just have>> a #[cfg(target_pointer_width = "32")] path as well and make the code>> fully reusable?> > Sure, that would be nice. But I only want to add and maintain tests> that I can execute and verify, at least once to make sure that their> assumptions are correct. As far as I see, this would involve setting up> cross compilation and something like qemu to execute the tests, right?
i686-unknown-linux-gnu is a fully supported platform [5]. I think all
you'd need is rustup --install --target i686-unknown-linux-gnu (or
whatever the exact options) and then use that for compilation (cargo
build --target XXX).
Given that it's both Linux and unless you explicitly disable 32 bit
support in the kernel (not done by any distributions that I am aware of
by default), the binary *should* just run then. So I doubt you'll need
an emulator or VM, but if you are not a fan of rustup you may want to
run/install that in isolation somewhere.
> And there might be some exotic platforms with other pointer sizes than> 32-bit or 64-bit.
I'd think that if you package for all the platforms that Debian supports
(at least to the degree you apparently did) and it only blew up once,
that should be fine. It's a possibility that a platform has a different
pointer size, but seems rather unlikely these days (16-bit MSP430
microcontrollers perhaps, but they may not even handle USB :P), and you
can always keep the fallback you suggested.
> If you have the option to execute the 32-bit tests, I can add them to> the patch series and include them in future releases.
Not out of the box. But if you can't/won't do the above, I think I
should be able to set up a GitHub actions/GitLab CI project that tests
on i686-unknown-linux-gnu.
Daniel
[5] https://doc.rust-lang.org/nightly/rustc/platform-support.html
On 2020-09-20 11:51:47, Daniel Müller wrote:
> i686-unknown-linux-gnu is a fully supported platform [5]. I think all> you'd need is rustup --install --target i686-unknown-linux-gnu (or> whatever the exact options) and then use that for compilation (cargo> build --target XXX).> Given that it's both Linux and unless you explicitly disable 32 bit> support in the kernel (not done by any distributions that I am aware of> by default), the binary *should* just run then. So I doubt you'll need> an emulator or VM, but if you are not a fan of rustup you may want to> run/install that in isolation somewhere.
Nice! And yes, I’m already using a virtual machine for rustup. ;-)
For my own reference, here are the steps to run the tests for i686 on
Debian:
# dpkg --add-architecture i386
# apt update
# apt install libnitrokey-dev:i386 gcc-multilib g++-multilib
$ rustup target add i686-unknown-linux-gnu
$ cargo test --target i686-unknown-linux-gnu
> I'd think that if you package for all the platforms that Debian supports> (at least to the degree you apparently did) and it only blew up once,> that should be fine. It's a possibility that a platform has a different> pointer size, but seems rather unlikely these days (16-bit MSP430> microcontrollers perhaps, but they may not even handle USB :P), and you> can always keep the fallback you suggested.
Rust is not available for all platforms supported by Debian. Here is a
list of all builds for nitrokey-sys-rs:
https://buildd.debian.org/status/package.php?p=rust-nitrokey-sys
BD-Uninstallable means that a build dependency, must likely Rust, is not
available.
/Robin
Apparently, there are more variables than the pointer width that
affect the layout of the generated structs: For powerpc, armel and
armhf (all 32-bit), only the layout test for the NK_device_info struct
fails. For i368, the test for ReadSlot_t fails too. (NK_device_info
contains a pointer, ReadSlot_t only contains sized integers and arrays
of sized integers.)
We could add test cases for the x86 and x86_64 target architectures, but
this would still just guess that the layout is consistent within an
architecture. Unfortunately, there seems to be no way to use the Rust
target as a compilation condition.
/Robin
On 9/21/20 11:22 AM, Robin Krahl wrote:
> Apparently, there are more variables than the pointer width that> affect the layout of the generated structs: For powerpc, armel and> armhf (all 32-bit), only the layout test for the NK_device_info struct> fails. For i368, the test for ReadSlot_t fails too. (NK_device_info> contains a pointer, ReadSlot_t only contains sized integers and arrays> of sized integers.)
Oh. Probably different alignment/padding then (?).
> We could add test cases for the x86 and x86_64 target architectures, but> this would still just guess that the layout is consistent within an> architecture. Unfortunately, there seems to be no way to use the Rust> target as a compilation condition.
Can't you combine target_arch = XXX and target_os = YYY to get pretty
close? Or how is that insufficient?
Out of curiosity, when you did all those experiments, have you compared
the produced Rust code? Is it always the same (excluding the tests, of
course)?
So other than --no-layout-tests (which I still think is the saner trade
off compared to dynamic generation ;-) [assuming the code is the same])
I have one last suggestion:
Could we have a switch (feature, environment variable, whatever) that
toggles between the two approach (my preference: defaulting to
statically generated stuff)? The checked in code would not contain
layout tests and the dynamically generated would have the full shebang.
This way, at least we wouldn't make everybody pay for these tests while
opt in is always possible. So for distributions nothing would have to
change conceptually (they'd just have to opt in).
Realistically, these tests would only by run by developers of the crate
and by package maintainers, right? If you just build a crate from the
git repository or from crates.io I don't think that there is even an
option to run them. So I don't think that such a hypothetical switch
would need to be explicitly plumbed through and exposed by consuming
crates.
If that's not feasible/desired then I have no other ideas right now and
we should probably go with unconditional generation.
Daniel
On 2020-09-21 18:51:44, Daniel Müller wrote:
> Can't you combine target_arch = XXX and target_os = YYY to get pretty> close? Or how is that insufficient?
Probably, but again, it’s just an assumption. For example, we have two
targets for 64-bit windows that might behave differently:
x86_64-pc-windows-gnu
x86_64-pc-windows-msvc
> Out of curiosity, when you did all those experiments, have you compared> the produced Rust code? Is it always the same (excluding the tests, of> course)?
I didn’t manually check the platforms, I just had a closer look at the
logs of the Debian build server [0]. Unfortunately, I can’t use the
build servers for debugging.
[0] https://buildd.debian.org/status/logs.php?pkg=rust-nitrokey-sys&ver=3.5.0-1
And I can’t run i686-bindgen on my own machine to compare the generated
bindings because I don’t know how to get the i686 clang libraries. (I
can’t install it using the package manager, apparently because some
binaries would collide.)
> Could we have a switch (feature, environment variable, whatever) that> toggles between the two approach (my preference: defaulting to> statically generated stuff)? The checked in code would not contain> layout tests and the dynamically generated would have the full shebang.> This way, at least we wouldn't make everybody pay for these tests while> opt in is always possible. So for distributions nothing would have to> change conceptually (they'd just have to opt in).
I think the only way to avoid pulling in bindgen is to add a feature. I
think that should work. A drawback of this approach is that if the
tests are run with the bindgen feature and nitrocli is compiled without
modifications, we wouldn’t actually test the code that is used by
nitrocli. So distribution packagers have to make sure that the bindgen
feature is also enabled when compiling nitrocli. (Still better than the
current situation.)
Ideally, we would always use the statically generated bindings and just
generate the platform-specific layout tests if the bindgen feature is
enabled. Then we would notice if there is platform-specific code in the
bindings. Unfortunately, this is currently not supported by bindgen [1].
[1] https://github.com/rust-lang/rust-bindgen/pull/1761> Realistically, these tests would only by run by developers of the crate> and by package maintainers, right? If you just build a crate from the> git repository or from crates.io I don't think that there is even an> option to run them. So I don't think that such a hypothetical switch> would need to be explicitly plumbed through and exposed by consuming> crates.
Yes, I think it’s impossible to execute the nitrokey-sys tests when
running cargo test/install for nitrocli. Still, I think it would make
sense for users with Rust targets other than i686-unknown-linux-gnu and
x86_64-unknown-linux-gnu to execute the tests for nitrokey-sys. Again,
they would have to make sure to also use the dynamically generated
bindings when compiling nitrocli. AFAIK they could do that by adding
nitrokey-sys = { version = "*", features = ["bindgen"] }
to the Cargo.toml for nitrocli.
/Robin
On 9/22/20 4:49 AM, Robin Krahl wrote:
>> Could we have a switch (feature, environment variable, whatever) that>> toggles between the two approach (my preference: defaulting to>> statically generated stuff)? The checked in code would not contain>> layout tests and the dynamically generated would have the full shebang.>> This way, at least we wouldn't make everybody pay for these tests while>> opt in is always possible. So for distributions nothing would have to>> change conceptually (they'd just have to opt in).> > I think the only way to avoid pulling in bindgen is to add a feature. I> think that should work. A drawback of this approach is that if the> tests are run with the bindgen feature and nitrocli is compiled without> modifications, we wouldn’t actually test the code that is used by> nitrocli. So distribution packagers have to make sure that the bindgen> feature is also enabled when compiling nitrocli. (Still better than the> current situation.)> > Ideally, we would always use the statically generated bindings and just> generate the platform-specific layout tests if the bindgen feature is> enabled. Then we would notice if there is platform-specific code in the> bindings. Unfortunately, this is currently not supported by bindgen [1].> > [1] https://github.com/rust-lang/rust-bindgen/pull/1761> >> Realistically, these tests would only by run by developers of the crate>> and by package maintainers, right? If you just build a crate from the>> git repository or from crates.io I don't think that there is even an>> option to run them. So I don't think that such a hypothetical switch>> would need to be explicitly plumbed through and exposed by consuming>> crates.> > Yes, I think it’s impossible to execute the nitrokey-sys tests when> running cargo test/install for nitrocli. Still, I think it would make> sense for users with Rust targets other than i686-unknown-linux-gnu and> x86_64-unknown-linux-gnu to execute the tests for nitrokey-sys. Again,> they would have to make sure to also use the dynamically generated> bindings when compiling nitrocli. AFAIK they could do that by adding> > nitrokey-sys = { version = "*", features = ["bindgen"] }> > to the Cargo.toml for nitrocli.
Yeah, so what I was thinking is:
- we could generate bindings dynamically twice
- 1) without tests
- we could then verify that the created header file is the same as
the statically generated one (literally a diff(1) or cmp(1) on the two
headers; or, if you do it in build.rs, with Rust file operations)
- if they are not equal, that would mean that our assumptions are
wrong; that should be treated as the same class of failure as a test
failure is at the moment
- 2) with tests
- that's when we can run the layout tests
I think that could alleviate both problems in a semi-nice fashion. And I
don't believe that we'd need any special bindgen support to pull that
off. The first step could happen in the Makefile or in build.rs, for
example.
> Please have a look at my feature-bindgen branch:> https://git.sr.ht/~ireas/nitrokey-sys-rs/log/feature-bindgen> If the bindgen feature is activated, the bindings and layout tests are> generated during the build.
Looks great to me!
Daniel
On 2020-09-22 19:31:56, Daniel Müller wrote:
> I think that could alleviate both problems in a semi-nice fashion. And I> don't believe that we'd need any special bindgen support to pull that> off. The first step could happen in the Makefile or in build.rs, for> example.
I wanted to generate the bindings in build.rs and then perform the
comparison in a Rust test case to make it easier to execute the test,
but unfortunately this doesn’t work because we have to also apply the
patches to the generated bindings.
Therefore I added a new target to the Makefile that re-generates the
bindings and makes sure that they match the version in the Git
repository. This is not as flexible and elegant as the first approach,
but at least we don’t have to manually apply the patches to the
generated bindings.
I think we’re ready to release v3.6.0, do you agree?
/Robin
On 9/23/20 4:21 AM, Robin Krahl wrote:
> On 2020-09-22 19:31:56, Daniel Müller wrote:>> I think that could alleviate both problems in a semi-nice fashion. And I>> don't believe that we'd need any special bindgen support to pull that>> off. The first step could happen in the Makefile or in build.rs, for>> example.> I wanted to generate the bindings in build.rs and then perform the> comparison in a Rust test case to make it easier to execute the test,> but unfortunately this doesn’t work because we have to also apply the> patches to the generated bindings.>> Therefore I added a new target to the Makefile that re-generates the> bindings and makes sure that they match the version in the Git> repository. This is not as flexible and elegant as the first approach,> but at least we don’t have to manually apply the patches to the> generated bindings.>> I think we’re ready to release v3.6.0, do you agree?
I think so!
Daniel