Use `cbindgen` via Rust API #14013

Cryoris · 2025-03-13T12:29:53Z

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

qiskit-bot · 2025-03-13T12:30:01Z

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

@Qiskit/terra-core

coveralls · 2025-03-13T15:47:58Z

Pull Request Test Coverage Report for Build 14171774193

Details

26 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 88.099%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	6	91.98%

Totals
Change from base Build 14171354739:	0.02%
Covered Lines:	72805
Relevant Lines:	82640

💛 - Coveralls

mtreinish

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?

crates/cext/build_header.rs

Cryoris · 2025-03-14T10:28:08Z

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 🙂

and use Makefile to correctly move or regenerate the file lazily

jakelishman · 2025-03-19T09:44:45Z

I thought a point of this PR was that the build-script API is more featureful than the stand-alone command-line build. If so, then surely we must be removing the stand-alone build as a supported option, or we're setting ourselves up for pain when the two build methods start producing different results.

I don't see a huge problem with "to build the header file you must build the library", especially at this early stage, but as a minimal workaround at least: cargo check necessarily executes build scripts, but doesn't fully compile the library.

crates/cext/build_header.rs

crates/cext/cbindgen.toml

Makefile

jakelishman · 2025-03-19T10:45:06Z

Makefile

 $(C_LIB_CARGO_PATH):
 	cargo rustc --release --crate-type cdylib -p qiskit-cext

-$(C_QISKIT_H): $(C_LIB_CARGO_PATH)
-	cbindgen --crate qiskit-cext --output $(C_DIR_INCLUDE)/qiskit.h --lang C
+# Compile the C library and move the header into the right location, if it was generated
+$(C_LIB_CARGO_PATH): $(C_DIR_INCLUDE)
+	cargo build --release -p qiskit-cext
+	@if [ -f crates/cext/qiskit.h ]; then mv crates/cext/qiskit.h $(C_DIR_INCLUDE)/qiskit.h; fi

 $(C_DIR_LIB):
 	mkdir -p $(C_DIR_LIB)

-$(C_LIBQISKIT): $(C_DIR_LIB) $(C_LIB_CARGO_PATH)
+$(C_DIR_INCLUDE):
+	mkdir -p $(C_DIR_INCLUDE)
+
+$(C_LIBQISKIT): $(C_DIR_LIB)  $(C_LIB_CARGO_PATH)
 	cp $(C_LIB_CARGO_PATH) $(C_DIR_LIB)/$(subst _cext,,$(C_LIB_CARGO_FILENAME))

-.PHONY: cheader clib c
+$(C_QISKIT_H): $(C_LIB_CARGO_PATH)
+	@if [ ! -f "$(C_QISKIT_H)" ]; then cbindgen --crate qiskit-cext --output $(C_DIR_INCLUDE)/qiskit.h; fi


There's quite a lot of duplication going on here, and recipes building more than they're specifically asked. Recipes with specific filenames should build that filename only. Copying the file from one place to another is a separate recipe.

I don't think we should have cbindgen as a command-line tool anywhere in the Makefile anymore, since we're now doing this by build script?

The header handling is not analogous to how you did the lib handling before: there's separate targets for creating the include dir and copying the header into the include dir. Also cbindgen is now no longer needed as command line tool since I added target/qiskit.h as tracked file in the build script (to ensure it is rebuilt if it was removed/touched). I tested locally that deleting the headers manually retriggers the build process.

write header into target/qiskit.h and copy into dist/c/include with Makefile. The cargo build script tracks changes in target/qiskit.h and rebuilds if changed/deleted Not sure if this is the best way but it seems to work :)

…o cbindgen-script

this is probably why CI doesn't work, we need to ensure the cdylib is built

mtreinish

This is looking good to me. Thanks for the updates on this, I just have one small inline question/comment about the build_header.rs but otherwise I think this ready to merge.

mtreinish · 2025-03-26T16:23:01Z

crates/cext/build_header.rs

+    let config = cbindgen::Config::from_file("cbindgen.toml").unwrap();
+
+    // Ensure target path exists and then set the full filename of qiskit.h.
+    let mut path = ::std::path::PathBuf::from_str("../../target/").unwrap();


I always worry when I see a relative path unqualified like this, since it's assuming the cwd is crates/cext/build_header but I checked and there is not official way to get the target dir from cargo. The file!() macro might be useful here, but I'm not sure what that returns, if it's the absolute path to build_header.rs on the local filesystem that might be a better option, but if it just returns build_header.rs that's obviously not useful.

An absolute path would indeed be nicer, but I didn't really find a way to get it. file!() returned just crates/cext/build_header.rs as path, not the absolute path to reach the target dir... I tested building both from Qiskit root and cargo/cext and it seems to work, if that is consolidation enough 😬

Well then lets go with it, I couldn't come up with a better solution either and there is a tracking issue in cargo already: rust-lang/cargo#9661

So I'm fine with doing this and if/when we come up with an alternative we can adjust it. But, do you know if this works on windows? Maybe we wait until #14030 merges just to make sure.

Done, let's see if CI passes

* move cbindgen to Rust build * add include guard * allow cmdline build and compile-time build * write `qiskit.h` to `crates/cext` and use Makefile to correctly move or regenerate the file lazily * try cleaner build setup write header into target/qiskit.h and copy into dist/c/include with Makefile. The cargo build script tracks changes in target/qiskit.h and rebuilds if changed/deleted Not sure if this is the best way but it seems to work :) * try fix build deps, and fix clippy * fix merge artifact this is probably why CI doesn't work, we need to ensure the cdylib is built * include dir creation as dependency (cherry picked from commit 241818b)

* move cbindgen to Rust build * add include guard * allow cmdline build and compile-time build * write `qiskit.h` to `crates/cext` and use Makefile to correctly move or regenerate the file lazily * try cleaner build setup write header into target/qiskit.h and copy into dist/c/include with Makefile. The cargo build script tracks changes in target/qiskit.h and rebuilds if changed/deleted Not sure if this is the best way but it seems to work :) * try fix build deps, and fix clippy * fix merge artifact this is probably why CI doesn't work, we need to ensure the cdylib is built * include dir creation as dependency (cherry picked from commit 241818b) Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>

Cryoris added 2 commits March 13, 2025 12:06

move cbindgen to Rust build

a5b2d8b

add include guard

09b4b8c

Cryoris added the C API Related to the C API label Mar 13, 2025

Cryoris requested a review from a team as a code owner March 13, 2025 12:29

mtreinish reviewed Mar 13, 2025

View reviewed changes

crates/cext/build_header.rs Outdated Show resolved Hide resolved

allow cmdline build and compile-time build

8c31da2

Cryoris requested a review from mtreinish March 18, 2025 10:12

write qiskit.h to crates/cext

8a81b06

and use Makefile to correctly move or regenerate the file lazily

Merge branch 'main' into cbindgen-script

7fa0d26

jakelishman reviewed Mar 19, 2025

View reviewed changes

Cryoris added 5 commits March 21, 2025 14:13

try cleaner build setup

606e2f5

write header into target/qiskit.h and copy into dist/c/include with Makefile. The cargo build script tracks changes in target/qiskit.h and rebuilds if changed/deleted Not sure if this is the best way but it seems to work :)

Merge branch 'cbindgen-script' of github.com:Cryoris/qiskit-terra int…

f9b2056

…o cbindgen-script

try fix build deps, and fix clippy

ddfc187

fix merge artifact

2580d17

this is probably why CI doesn't work, we need to ensure the cdylib is built

include dir creation as dependency

46878f9

Cryoris requested a review from jakelishman March 25, 2025 18:03

mtreinish reviewed Mar 26, 2025

View reviewed changes

mtreinish added the Changelog: None Do not include in changelog label Mar 26, 2025

Cryoris added this to the 2.0.0 milestone Mar 28, 2025

Cryoris mentioned this pull request Mar 31, 2025

Create folder before building #14134

Closed

Merge branch 'main' into cbindgen-script

9aeb23e

mtreinish approved these changes Mar 31, 2025

View reviewed changes

mtreinish enabled auto-merge March 31, 2025 13:40

mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Mar 31, 2025

mtreinish added this pull request to the merge queue Mar 31, 2025

Merged via the queue into Qiskit:main with commit 241818b Mar 31, 2025
25 checks passed

mergify bot mentioned this pull request Mar 31, 2025

Use cbindgen via Rust API (backport #14013) #14139

Merged

Cryoris deleted the cbindgen-script branch March 31, 2025 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use `cbindgen` via Rust API #14013

Use `cbindgen` via Rust API #14013

Uh oh!

Cryoris commented Mar 13, 2025 •

edited

Loading

Uh oh!

qiskit-bot commented Mar 13, 2025

Uh oh!

coveralls commented Mar 13, 2025 •

edited

Loading

Uh oh!

mtreinish left a comment

Uh oh!

Uh oh!

Cryoris commented Mar 14, 2025

Uh oh!

jakelishman commented Mar 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakelishman Mar 19, 2025

Uh oh!

Cryoris Mar 24, 2025

Uh oh!

mtreinish left a comment

Uh oh!

mtreinish Mar 26, 2025

Uh oh!

Cryoris Mar 26, 2025

Uh oh!

mtreinish Mar 26, 2025

Uh oh!

Cryoris Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

Use cbindgen via Rust API #14013

Use cbindgen via Rust API #14013

Uh oh!

Conversation

Cryoris commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Uh oh!

qiskit-bot commented Mar 13, 2025

Uh oh!

coveralls commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 14171774193

Details

💛 - Coveralls

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cryoris commented Mar 14, 2025

Uh oh!

jakelishman commented Mar 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakelishman Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Cryoris Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

mtreinish Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cryoris Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

mtreinish Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Cryoris Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Use `cbindgen` via Rust API #14013

Use `cbindgen` via Rust API #14013

Cryoris commented Mar 13, 2025 •

edited

Loading

coveralls commented Mar 13, 2025 •

edited

Loading