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

libtcgtpm: use bindgen in build.rs instead of bindgen-cli in Makefile #606

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stefano-garzarella
Copy link
Member

We had several issues in our CI because we forgot to install bindgen-cli used in the libtcgtpm/Makefile to generate the TPM bindings.

Instead of using bindgen-cli (and then having as a requirement to have it installed), let's use bindgen in build.rs, this way the dependency is handled directly by cargo.

Let's also clean up the references to libtcgtpm/src/bindings.rs since bindings were already generated in the OUT_DIR.

Remove the reference to install bindgen-cli in the CI workflows, Documentation, and container image.

We had several issues in our CI because we forgot to install
`bindgen-cli` used in the libtcgtpm/Makefile to generate the TPM
bindings.

Instead of using `bindgen-cli` (and then having as a requirement to have
it installed), let's use `bindgen` in build.rs, this way the dependency
is handled directly by cargo.

Let's also clean up the references to `libtcgtpm/src/bindings.rs` since
bindings were already generated in the OUT_DIR.

Remove the reference to install `bindgen-cli` in the CI workflows,
Documentation, and container image.

Signed-off-by: Stefano Garzarella <[email protected]>
@stefano-garzarella
Copy link
Member Author

Rust / Check unsafe blocks is failing because the base branch still requires bindgen-cli.
To make it pass, I can undo the change where I removed its installation for that workflow, but not sure if it makes sense or not, since after merging this PR, installing bindgen-cli is useless. @joergroedel WDYT?

Copy link
Member

@cclaudio cclaudio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cclaudio
Copy link
Member

I was not able to reproduce the Rust / Check unsafe blocks issue. I fetched the PR and run

cargo clippy --workspace --all-features --exclude packit --exclude stage1 --exclude svsm-fuzz --exclude igvmbuilder --exclude igvmmeasure --quiet -- -W clippy::undocumented_unsafe_blocks

Although that prints many unsafe warnings, the command exited with success (exit 0).

@stefano-garzarella
Copy link
Member Author

I was not able to reproduce the Rust / Check unsafe blocks issue. I fetched the PR and run

Yeah, it's expected. We introduced a new CI workflow to avoid new unsafe blocks not documented.
That workflow checks out the base branch of the PR (e.g. main) and run that command to counts the number of unsafe blocks not documented, in order to compare them with the PR applied.

In this case I removed the bindgen-cli installation in the workflow, so that step fails.
I can put it back in that workflow, and maybe we can remove later, but when this PR will be merged, that should not be a problem anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants