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.

Sign up for GitHub

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

Jump to bottom

Add tests for 32-bit systems. #722

Closed
thomcc opened this issue Apr 19, 2020 · 10 comments · May be fixed by #1214
Closed

Add tests for 32-bit systems. #722

thomcc opened this issue Apr 19, 2020 · 10 comments · May be fixed by #1214
Assignees
@thomcc
Labels
testing / ci

Comments

@thomcc
Copy link
Member

thomcc commented Apr 19, 2020

One impediment here is that the bundled bindings include tests that only work on 64-bit systems (because they include hard-coded constants).

Probably worth removing those tests when generating the bundled bindings and including them for non-bundled cases (e.g. buildtime_bindgen).
@thomcc thomcc self-assigned this Apr 19, 2020
@thomcc
Copy link
Member Author

thomcc commented Apr 23, 2020

Would have caught #724 probably

@dkg
Copy link
Contributor

dkg commented Jul 25, 2022

This is actually causing build failures on Debian's 32-bit architectures. For example, here's a failing build log for i386.

I'd love to have the latest version of the libsqlite3-sys in debian, but to do that i'm probably going to need to patch out these tests if the platform is 32-bit, which is a bit disappointing.

@thomcc
Copy link
Member Author

thomcc commented Jul 25, 2022

Yeah, the bindgen tests in libsqlite3-sys are platform-specific, unfortunately. I wrote a PR a long time ago to make bindgen generate them on the side, with the idea that we could have several, but nothing happened with it: rust-lang/rust-bindgen#1761 (It's bitrotted quite a bit, and I probably don't have the time to fix it up either).

Either way, I don't think the tests bindgen emits help much. They basically just test that #[repr(C)] is implemented correctly, so I'd accept a patch removing them.

dkg added a commit to dkg/rusqlite that referenced this issue Aug 16, 2022
@dkg
Drop bindgen layout tests
32b1ecf 
The auto-generated bindgen layout tests are architecture dependent and
cause breakage on 32-bit platforms at least.

Note that this does not yet remove the layout tests from
bindgen-bindings/bindgen_*.rs -- i'm not sure how those are generated.

Addresses: rusqlite#722
@dkg dkg mentioned this issue Aug 16, 2022
Merged
@dkg
Copy link
Contributor

dkg commented Aug 16, 2022

@thomcc wrote:

Either way, I don't think the tests bindgen emits help much. They basically just test that #[repr(C)] is implemented correctly, so I'd accept a patch removing them.

#1213 is a patch to remove them from the bundled bindings. let me know if you'd rather it looks different somehow. thanks for maintaining rusqlite!

thomcc pushed a commit that referenced this issue Aug 16, 2022
@dkg @thomcc
Drop bindgen layout tests
53b1b59 
The auto-generated bindgen layout tests are architecture dependent and
cause breakage on 32-bit platforms at least.

Note that this does not yet remove the layout tests from
bindgen-bindings/bindgen_*.rs -- i'm not sure how those are generated.

Addresses: #722
@thomcc
Copy link
Member Author

thomcc commented Aug 16, 2022

Thanks.

@thomcc thomcc closed this as completed Aug 16, 2022
dkg added a commit to dkg/rusqlite that referenced this issue Aug 17, 2022
@dkg
Drop test_layout bindgen tests from stored versions
8e7d71f 
Note that this was done manually by cutting out all #[test] functions
named *_test_layout_*, rather than the automated re-generation step
that produced 53b1b59.

Closes: rusqlite#722
@dkg dkg mentioned this issue Aug 17, 2022
Open
@dkg
Copy link
Contributor

dkg commented Aug 17, 2022

#1214 includes removal of the tests from the different stored versions as well.

I notice that those versions don't do things like strip out __GNUC_VA_LIST, but i left those choices alone :)

@thomcc
Copy link
Member Author

thomcc commented Aug 17, 2022

Those versions should be removed honestly. There are a lot of things I need to do to the build.rs when I find time, but haven't found it sadly.

@dkg
Copy link
Contributor

dkg commented Aug 17, 2022

i know the feeling! I'm just reporting what i see and trying to be helpful, not trying to nag you :)

FWIW, at least one of these bundled versions is still in use during standard compilation when not using bundled libsqlite3.

i'm also looking at the code change that resulted from my regeneration of the bundled bindings in #1213, and noticing that it is significantly more transformative than the manual cut jobs in #1214. I hope i didn't screw that up, but i'd certainly appreciate any sort of more detailed review -- if you've got feedback you want me to consider, i'd be happy to consider it.

@thomcc
Copy link
Member Author

thomcc commented Aug 17, 2022

Give me a week or so and I can find time for a more detailed review. Looking again, that seems to have cut out more than it should have.

@dkg
Copy link
Contributor

dkg commented Aug 17, 2022

i've opened #1215 to try to repair the excessive cutting. sorry about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thomcc thomcc

Labels
testing / ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Drop test_layout bindgen tests from stored versions dkg/rusqlite
2 participants
@dkg @thomcc