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

Makefile: Run OpenTitan userspace tests #2012

Closed
wants to merge 1 commit into from

Conversation

alistair23
Copy link
Contributor

Pull Request Overview

This PR runs the libtock-rs binaries as part of the build test.

Recently apps on OpenTitan was completely broken (fixed with: #1970) due to a lack of testing. One of my PMP changes broke userspace apps, but wasn't caught on the board as PMP was disabled on the FPGA bitsrteam. Having a CI that runs even a basic app in userspace would have caught this issue, hence this PR.

The idea of this PR is to move the CI towards what a user will run. That should hopefully increase the coverage of the CI while also catching bugs before users do. This should hopefully help improve sync issues between libtock-* and tock as well.

Testing Strategy

Running the CI.

TODO or Help Wanted

Unfortunately the GitHub artifact action is completely broken and requires a user to have a GitHub account to download artifacts.

Due to this we need the CI to have a user and a token to run the test. For example I can run the test like this (not a real token):

USER_PAT="alistair23:cjqka6fkhhodne7945fioynd3hs6m6yig88oft7x" make ci-job-qemu

For the CI we could probably save a token securely in the CI (I know Travis can do this at least) but that won't help for people running it locally.

Otherwise we have to wait for GitHub to fix their artifact builder, have libtock-rs use something other then GitHub actions (a third party service) or possibly check in the binaries or build libtock-rs binaries ourselves.

Thoughts?

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@ppannuto
Copy link
Member

Could we use the artifacts attached to a libtock-rs (and for that matter libtock-c) release? Those are publicly accessible.

@hudson-ayers
Copy link
Contributor

Could we use the artifacts attached to a libtock-rs (and for that matter libtock-c) release? Those are publicly accessible.

This has the added benefit of not blocking Tock PRs because of breakages that are actually the fault of changes in libtock-rs.

@bradjc
Copy link
Contributor

bradjc commented Jul 13, 2020

The kernel doesn't guarantee it won't break most userspace apps. If we pin to a specific userspace release, the apps have to only use the stabilized syscall drivers.

@alistair23
Copy link
Contributor Author

The problem with releases is then we can't break the ABI between Tock kernel releases. For example if there is a PR that changes the ABI and we are still using libtock-c/libtock-rs from the 1.5 release the tests will fail until the next libtock-c/libtock-rs release.

Also this doesn't mean we will track the master branch of each repo, it is still a specific commit we are building from.

It's probably worth making the userspace tests not block merging. So if a PR changes the userspace ABI it can state that the tests are expected to break and they will be fixed in the future as it's difficult to update both Tock and libtock-* at the same time.

@bradjc
Copy link
Contributor

bradjc commented Dec 3, 2020

With the wisdom of a few months since this PR, what do you think @hudson-ayers @ppannuto (the de-facto other members of the CI task force)? I put this in your hands.

@hudson-ayers
Copy link
Contributor

I think that pinning a commit and having the tests not block merging would make this acceptable to include. I think that would require the userspace tests be in a separate action from the default QEMU tests. I also think that it would be nice to use a name that clearly indicates that test is allowed to fail, so new contributors don't waste time trying to figure out why CI is not passing.

This would basically just be a tool so reviewers can click and see if they notice any obvious regression in which userspace tests are passing.

@alevy alevy added the triage-1 label Jan 11, 2021
@alevy
Copy link
Member

alevy commented Jan 11, 2021

Closing this for now in favor of figuring out what we want in #2023

@alevy alevy closed this Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants