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

Use cbindgen via Rust API #14013

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Mar 13, 2025

Summary

We currently generate the header using cbindgen as command line tool. This works, but

  • doesn't expose all features cbindgen has (ok almost all)
  • requires users to additionally install the command line tool (and run it)
  • emits a lot of warnings we (currently?) don't know how to silence

This PR moves to using the Rust API of cbindgen and builds the header as part of the qiskit-cext compilation process. This fixes all the above points. The one down side is that the cbindgen.toml was a bit nicer to read IMO, but that's a small price to pay.

It would be nice to backport this to stable/2.0 for a smoother build process for users.

Details

  • The Makefile is slightly simpler now since we don't need to separately call the command line tool to generate the header file.
  • The resulting header file is exactly the same as before, up to an additional include guard I added in this PR

@Cryoris Cryoris added the C API Related to the C API label Mar 13, 2025
@Cryoris Cryoris requested a review from a team as a code owner March 13, 2025 12:29
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Mar 13, 2025

Pull Request Test Coverage Report for Build 13854764156

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • 123 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.02%) to 88.142%

Files with Coverage Reduction New Missed Lines %
crates/circuit/src/dag_circuit.rs 1 86.25%
crates/qasm2/src/lex.rs 2 92.98%
crates/accelerate/src/euler_one_qubit_decomposer.rs 4 91.39%
crates/accelerate/src/remove_identity_equiv.rs 5 94.44%
crates/circuit/src/circuit_data.rs 55 91.86%
crates/accelerate/src/two_qubit_decompose.rs 56 91.84%
Totals Coverage Status
Change from base Build 13831282993: 0.02%
Covered Lines: 72742
Relevant Lines: 82528

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I like the concept of this integrating the header generation into the build process, it makes a lot of sense, especially when building the standalone c dynamic library.

My concern is for the case where people are trying to get the header file without building the library. For example, if a user of the precompiled wheels is trying to generate the header and build a custom python extension the normal workflow is to run cbindgen manually or run make cheader. So I'm wondering what the workflow is for that use case here because you removed the cbindgen.toml so the standalone header generation doesn't work anymore does it?

config.parse = parse;

// Ensure the include directory exists, and then set the full header path.
let mut path = "../../dist/c/include".to_string();
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this is always what we want. Like I get that's what we had in the makefile, but it seems overly prescriptive on the use case. Like when I'm working on the C API I tend to have a cwd of crates/cext and just call cargo build and cbindgen directly.

I'm thinking that we might want the build file to only save it to the cwd and rely on the makefile to move it to the final destination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds more stable 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'll revise: this path seems to be relative to the cext crate -- whether running

cd $QISKIT_ROOT
cargo build -p qiskit-cext

or

cd crates/cext
cargo build

The header file is correctly placed in $QISKIT_ROOT/dist/c/include 🙂 In this case this is more elegant than having an additional Makefile move the file, since we just need to cargo, right?

Copy link
Member

Choose a reason for hiding this comment

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

I was actually arguing that this was not desireable behavior. If you do:

cd crates/cext
cargo build

(or now cargo rustc --crate-type cdylib -p qiskit-cext)

I would expect the header to be in the CWD rather than ../../dist/c/include.

@Cryoris
Copy link
Contributor Author

Cryoris commented Mar 14, 2025

My concern is for the case where people are trying to get the header file without building the library.

Yes that's a good point. It seems we can actually have both (until we would need features only exposed in the Rust API): we can have the settings in a cbindgen.toml and allow using the command line tool, but also pull that config file in the crate build process. I'll push a commit with this in a bit 🙂

@Cryoris Cryoris requested a review from mtreinish March 18, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C API Related to the C API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants