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.

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

Idea for regression test for accidental removal of definitions #2104

Closed
Thomasdezeeuw opened this issue Mar 6, 2021 · 2 comments · Fixed by #2109
Closed

Idea for regression test for accidental removal of definitions #2104

Thomasdezeeuw opened this issue Mar 6, 2021 · 2 comments · Fixed by #2109
Labels
A-CI Area: CI-related items C-testsuite

Comments

@Thomasdezeeuw
Copy link
Contributor

I've been thinking about how to add regression tests for issues like #2101, wherein between versions 0.2.86 and 0.2.87 a system call was accidentally removed for a specific target. The structure for libc is quite complex with many #[cfg]s throughout the code base (for good reasons), this makes it quite easy to mistakenly remove an item for a target and not catch it in review.

Currently there is libc-test, which tests if a given definition in Rust is the same as in C. But this doesn't catch if a definition is removed between libc version.

So, now my idea. Keep around a list of definitions (functions, constants etc.) that are exported by libc in a text file (separate from the code base) for each OS/architecture combination. For example such a file for Android:

# linux-android.txt
accept
accept4
SOCK_NONBLOCK
SOCK_CLOEXEC
# Etc...

To make architecture specific changes we can use a diff-like format: + for additional definitions, - for unsupported definitions.

# arm-linux-androideabi.txt
-accept4 # Android on Arm doesn't support `accept4(2)` (Not true, just an example).
+accept6 # A system call only supported on Android on Arm (again just an example).

A build script could read these files for a given target, i.e. for the arm-linux-androideabi it would include both linux-android.txt and arm-linux-androideabi.txt, and convert it to a Rust test file. Such a test file would look something like the following:

#![allow(unused_imports)]

// Used files:
// linux-android.txt
// arm-linux-androideabi.txt

use libc::{
    accept, // From linux-android.txt.
    SOCK_NONBLOCK, // From linux-android.txt.
    SOCK_CLOEXEC, // From linux-android.txt.
    accept6, // From arm-linux-androideabi.txt.
    // Etc...
};

If a definition is removed between versions the test file wouldn't build, and thus fail the test/CI.

This is just an idea I had, it might be too much work to maintain such lists of definitions. I can put in the work for the setup/build script, but I realise that that is not were most of the work is in.

@Thomasdezeeuw Thomasdezeeuw added the C-API-request Category: API request label Mar 6, 2021
@JohnTitor JohnTitor added A-CI Area: CI-related items C-testsuite and removed C-API-request Category: API request labels Mar 6, 2021
@JohnTitor
Copy link
Member

This sounds like a great idea! We use rusr-semverver for this purpose but it doesn't make much sense currently. We, for example, sometimes move a declaration to the parent module but semverver complains about it (it's not a breaking change for us), that's why we ignore its check on bors. But this approach doesn't have such a false positive so it'd be better.

@joshtriplett
Copy link
Member

I like this plan! We might also be able to compare our lists to lists from other sources (e.g. Debian symbols files) to help us determine what we currently don't have supported. (Those lists don't include compile-time constants, but they include all library symbols.)

bors added a commit that referenced this issue Mar 27, 2021
…hnTitor

Add regression test infrastructure

Please the commit messages for details.

I still need to add lists for the following targets, but I got the major ones I think.

TODO:
* aarch64-unknown-hermit
* x86_64-unknown-hermit
* x86_64-pc-solaris
* x86_64-sun-solaris
* sparcv9-sun-solaris
* x86_64-fortanix-unknown-sgx
* x86_64-unknown-illumos
* asmjs-unknown-emscripten
* wasm32-unknown-emscripten
* wasm32-unknown-unknown
* wasm32-wasi
* Check symbols added after commit ed45c26.

TODO: add a bit to the contributing guide about adding to these lists.

Closes #2104.
bors added a commit that referenced this issue Mar 27, 2021
…hnTitor

Add regression test infrastructure

Please the commit messages for details.

I still need to add lists for the following targets, but I got the major ones I think.

TODO:
* aarch64-unknown-hermit
* x86_64-unknown-hermit
* x86_64-pc-solaris
* x86_64-sun-solaris
* sparcv9-sun-solaris
* x86_64-fortanix-unknown-sgx
* x86_64-unknown-illumos
* asmjs-unknown-emscripten
* wasm32-unknown-emscripten
* wasm32-unknown-unknown
* wasm32-wasi
* Check symbols added after commit ed45c26.

TODO: add a bit to the contributing guide about adding to these lists.

Closes #2104.
bors added a commit that referenced this issue Apr 1, 2021
Add regression test infrastructure

Please the commit messages for details.

I still need to add lists for the following targets, but I got the major ones I think.

TODO:
* aarch64-unknown-hermit
* x86_64-unknown-hermit
* x86_64-pc-solaris
* x86_64-sun-solaris
* sparcv9-sun-solaris
* x86_64-fortanix-unknown-sgx
* x86_64-unknown-illumos
* asmjs-unknown-emscripten
* wasm32-unknown-emscripten
* wasm32-unknown-unknown
* wasm32-wasi
* Check symbols added after commit ed45c26.

TODO: add a bit to the contributing guide about adding to these lists.

Closes #2104.
bors added a commit that referenced this issue Apr 2, 2021
Add regression test infrastructure

Please the commit messages for details.

I still need to add lists for the following targets, but I got the major ones I think.

TODO:
* aarch64-unknown-hermit
* x86_64-unknown-hermit
* x86_64-pc-solaris
* x86_64-sun-solaris
* sparcv9-sun-solaris
* x86_64-fortanix-unknown-sgx
* x86_64-unknown-illumos
* asmjs-unknown-emscripten
* wasm32-unknown-emscripten
* wasm32-unknown-unknown
* wasm32-wasi
* Check symbols added after commit ed45c26.

TODO: add a bit to the contributing guide about adding to these lists.

Closes #2104.
bors added a commit that referenced this issue Apr 2, 2021
Add regression test infrastructure

Please the commit messages for details.

I still need to add lists for the following targets, but I got the major ones I think.

TODO:
* aarch64-unknown-hermit
* x86_64-unknown-hermit
* x86_64-pc-solaris
* x86_64-sun-solaris
* sparcv9-sun-solaris
* x86_64-fortanix-unknown-sgx
* x86_64-unknown-illumos
* asmjs-unknown-emscripten
* wasm32-unknown-emscripten
* wasm32-unknown-unknown
* wasm32-wasi
* Check symbols added after commit ed45c26.

TODO: add a bit to the contributing guide about adding to these lists.

Closes #2104.
@bors bors closed this as completed in 0a93b70 Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: CI-related items C-testsuite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants