Add --depfile option #820

jschwe · 2023-03-09T16:00:13Z

Add an option to output a depfile for outside build-systems to learn the file dependencies of the bindings.

cbindgen already keeps track of all the source-files it visits, the information just had to be collected and passed up the callchain.

The output depfile is the common Makefile .d file, which cargo also already produes. example:

/path/to/generated_header.h: /path/to/src1 /path/to/src2

Whitespace in filepaths is escaped to support filepaths containing whitespace. This requires assuming an encoding, so we assume the filepath is utf-8 compliant (and otherwise lossy convert to utf-8).

Since the left hand side requires a file, it only makes sense to generate a depfile if --out is also specified, so the current implementation just ignores --depfile if --out is missing.

I looked at the sorrounding code, and cbindgen is generally very liberal with unwrap, so I also didn't add any error handling for this PR. If the filepaths contain non-unicode characters, I chose to use the lossy variant for conversion,

I tested this with CMake and Ninja on Linux, and the integration seems to be working as expected (also with paths containing spaces).
I'm not sure yet how to best add tests to cbindgen itself though - the tests seem to be generated from the build-script and I'm not quite sure how to add my one for this feature. Any pointers regarding would be appreciated.

Closes #815
Closes #192

emilio

Thanks, looks reasonable, but I think we should have a defined order to avoid the ordering depending on cache keys / hashmap iteration order.

It'd also be great to have a test for this.

emilio · 2023-03-10T12:44:08Z

src/bindgen/bindings.rs

+
+        writeln!(&mut depfile).unwrap();
+
+        depfile.flush().unwrap();


Doesn't closing the file flush as well? I doubt this is needed right?

When the file is dropped, errors when closing are ignored. Manually flushing and unwrapping results in a panic for errors. I don't have a strong opinion here, since errors when closing the file should be extremely rare. I can remove this line if you prefer.

emilio · 2023-03-10T12:48:16Z

src/bindgen/parser.rs

@@ -181,6 +181,8 @@ impl<'a> Parser<'a> {
            }
        }

+        self.out.source_files = self.cache_src.keys().map(|k| k.to_owned()).collect();


So this order is basically random, I'd rather have a well defined order by pushing the path in the Entry::Vacant case in parse_mod. Otherwise let's have some defined order in the depfile generation?

I pushed a commit which sorts the output before writing. This way all source files are sorted, and the config file (if any) is the last entry. ~~This should make writing tests much easier, since it allows us to just compare an output file with an expected file.~~ Edit: The last sentence was wrong, since the depfile contains absolute paths (and I'm fairly certain this is necessary)

jschwe · 2023-03-10T13:45:12Z

It'd also be great to have a test for this.

Do you have any recommendations for a test? Should I create a new integration test in tests, or integrate this into the tests test?

jschwe · 2023-03-11T11:22:24Z

Update: This PR currently doesn't support expanding (since we use rustc to expand the crate, and we don't know the source files ourselves). Probably this can be solved by letting rustc itself emit a .d file during the expansion and parsing that too. Edit: rustc doesn't emit anything when expanding, so we would need to invoke rustc a second time to actually get the dep-info from rustc :(.

Maybe it would be better to put that into a seperate PR though, to keep the diff of this one smaller?

jschwe · 2023-03-12T10:55:02Z

I added some tests which use CMake to verify that the generated depfile works as expected (i.e. the bindings aren't regenerated on a second build).
I previously also added --depfile to the tests in tests in commit test --depfile, but I'm leaning towards dropping that commit and only keeping the new CMake based test for the --depfile option. What do you think?

I also discovered that implicit configs (cbindgen.toml in the root dir) where previously not put into the depfile, so I added a commit which addresses that. Since the Config struct and methods are public, I added new internal methods to avoid any semver breaking changes.

jschwe · 2023-03-23T12:28:32Z

friendly ping

jschwe · 2023-04-13T06:06:33Z

@emilio Could I get another review round? Changes since your last review are summarized in this comment: #820 (comment)

jschwe · 2023-05-19T06:20:52Z

@emilio Could I get another review round? Changes since your last review are summarized in this comment: #820 (comment)

emilio

Sorry for the lag in re-reviewing this, personal life got in the way.

I think there are only a couple relatively minor issues.

I think making the path a (crate-private) field in the Config struct would simplify a bunch of the core failures.

Thanks for all the testing effort, much appreciated.

src/bindgen/parser.rs

emilio · 2023-05-29T12:52:50Z

src/bindgen/builder.rs

+    pub(crate) fn with_config_and_src(
+        mut self,
+        config: Config,
+        config_src: Option<path::PathBuf>,


Any reason we could not just store the src_path: Option<PathBuf> in the Config? That seems like it'd be cleaner?

The config_src should contain the path to the config file (if it exists).
The Config itself is parsed from the config file. It would be strange if the user could set a path in the config file, which should point to the config file itself.

Edit: Just checked and saw serde has a skip_deserializing attribute, so my previous concern is invalid.

Finished the change, this is indeed much cleaner.

jschwe · 2023-05-29T15:08:09Z

Probably #836 should be fixed first before merging, so that the CI can run again.

emilio

Just one minor tweak. Let me look at fixing the actions. Looks good with those, thanks!

src/bindgen/parser.rs

src/main.rs

tests/depfile.rs

tests/tests.rs

emilio · 2023-05-29T16:11:28Z

Can you rebase? We should have GH actions now.

Since our MSRV is now Rust 1.54, we can rely on cargo setting `CARGO_BIN_EXE_cbindgen` to the cbindgen path in integration tests. This is more reliable than guessing the path, since cargo knows where it placed the bin.

Add an option to output a depfile for outside build-systems to learn the source file dependencies of the bindings. This can be used by 3rd party build system integrations to only rerun bindgen when necessary. Testing is done via CMake integration tests, since CMake is a 3rd party buildsystem which supports depfiles.

jschwe · 2023-05-29T16:19:36Z

Rebased and squashed into two commits.

emilio

Awesome, ty!

Updates the requirements on [cbindgen](https://github.com/eqrion/cbindgen) to permit the latest version. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/mozilla/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.25.0</h2> <pre><code> * Re-release of yanked 0.24.6 as a major release * Update MSRV to 1.57 * Support variadic arguments (`...`) ([#805](mozilla/cbindgen#805)) * Add --depfile option ([#820](mozilla/cbindgen#820)) * Breaking changes: The `Config` struct now has a private member. </code></pre> <h2>0.24.6 (YANKED: depfile option was breaking, see <a href="https://redirect.github.com/eqrion/cbindgen/issues/841">#841</a>)</h2> <pre><code> * Update MSRV to 1.57 * Support variadic arguments (`...`) ([#805](mozilla/cbindgen#805)) * Add --depfile option ([#820](mozilla/cbindgen#820)) </code></pre> <h2>0.24.5</h2> <pre><code> * Don't enforce tempfile version. </code></pre> <h2>0.24.4</h2> <pre><code> * Move expand infinite recursion fix ([#799](mozilla/cbindgen#799)) * Add with_cpp_compat to the builder ([#796](mozilla/cbindgen#796)) * Handle never type in return position consistently ([#780](mozilla/cbindgen#780)) * Fix warnings ([#816](mozilla/cbindgen#816), [#819](mozilla/cbindgen#819)) * Updated documentation ([#788](mozilla/cbindgen#788), [#791](mozilla/cbindgen#791), [#792](mozilla/cbindgen#792), [#810](mozilla/cbindgen#810), [#823](mozilla/cbindgen#823)) </code></pre> <h2>0.24.3</h2> <pre><code> * Make struct expressions correctly generated through typedefs ([#768](mozilla/cbindgen#768)). </code></pre> <h2>0.24.2</h2> <pre><code> * Make bitfield operators use explicit constructors. </code></pre> <h2>0.24.1</h2> <pre><code> * Add support for unary negation ([#765](mozilla/cbindgen#765)). * Make more bitfield operators constexpr ([#765](mozilla/cbindgen#765)). </code></pre> <h2>0.24.0</h2> <pre><code> * Basic const generic support ([#759](mozilla/cbindgen#759), [#760](mozilla/cbindgen#760) [#762](mozilla/cbindgen#762)). * Suffixes on integer literals are now honored to avoid narrowing ([#764](mozilla/cbindgen#764)). </code></pre> <h2>0.23.0</h2> <pre><code> * Better support for constexpr. ([#756](mozilla/cbindgen#756)) * constexpr is now enabled by default in C++ mode. You can use const.allow_constexpr=false to revert to previous behavior. ([#756](mozilla/cbindgen#756)) * Minimum syn version no longer parses old rust code. ([#754](mozilla/cbindgen#754)) </code></pre>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/mozilla/cbindgen/commit/dd9a550152cd162a3aa01757a55dd22fc56d0d8a"><code>dd9a550</code></a> Fix minimal Rust version in CI</li> <li><a href="https://github.com/mozilla/cbindgen/commit/0529d215e7a1b2ad94ca166c1b26ad96f10e4a1c"><code>0529d21</code></a> Revert "Upgrade clap 3 to clap 4"</li> <li><a href="https://github.com/mozilla/cbindgen/commit/289a31ba45c28a6d8d5eb681f7e1c83a23cdb673"><code>289a31b</code></a> Fix clippy warning</li> <li><a href="https://github.com/mozilla/cbindgen/commit/67fea1a1a2e77464f96f184b298aed848ac38d53"><code>67fea1a</code></a> Fix CI</li> <li><a href="https://github.com/mozilla/cbindgen/commit/80526e72f9109d45da10625e790791fc3a5cc18a"><code>80526e7</code></a> Update changelog for v0.25.0</li> <li><a href="https://github.com/mozilla/cbindgen/commit/1e2ffd4414e1124f526a19ccb3b627c0e3694f53"><code>1e2ffd4</code></a> CI: Replace forbidden actions with cli code</li> <li><a href="https://github.com/mozilla/cbindgen/commit/f61946b9798982c4c27da9fd5917824db2093095"><code>f61946b</code></a> CI: Add semver checks to CI deploy job</li> <li><a href="https://github.com/mozilla/cbindgen/commit/b61aa2c330b3d4fe3bbe4fa71d9bc7104b7c94f7"><code>b61aa2c</code></a> msrv 1.64</li> <li><a href="https://github.com/mozilla/cbindgen/commit/b734008c71f31a125797e799f420e1ad32f6b2f9"><code>b734008</code></a> Upgrade clap 3 to clap 4</li> <li><a href="https://github.com/mozilla/cbindgen/commit/667de09279c3d5f0524216fefa949e64cbd3bc1a"><code>667de09</code></a> Add: Add rust-toolchain.toml</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.24.0...v0.25.0">compare view</a></li> </ul> </details> <br /> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Updates the requirements on [cbindgen](https://github.com/mozilla/cbindgen) to permit the latest version. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/mozilla/cbindgen/releases">cbindgen's releases</a>.</em></p> <blockquote> <h2>0.26.0</h2> <ul> <li>Fix swapping of <code>>>=</code> and <code><<=</code> in constants.</li> <li>Add support for #[deprecated] (<a href="https://redirect.github.com/mozilla/cbindgen/issues/860">#860</a>).</li> <li>Built-in support for bitflags 2.0.</li> <li>Support for "C-unwind" ABI.</li> <li>Generate bindings for non-public extern items if they are #[no_mangle].</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/mozilla/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h1>0.26.0</h1> <pre><code> * Fix swapping of `>>=` and `<<=` in constants. * Add support for #[deprecated] ([#860](mozilla/cbindgen#860)). * Built-in support for bitflags 2.0. * Support for "C-unwind" ABI. * Generate bindings for non-public extern items if they are #[no_mangle]. </code></pre> <h2>0.25.0</h2> <pre><code> * Re-release of yanked 0.24.6 as a major release * Update MSRV to 1.57 * Support variadic arguments (`...`) ([#805](mozilla/cbindgen#805)) * Add --depfile option ([#820](mozilla/cbindgen#820)) * Breaking changes: The `Config` struct now has a private member. </code></pre> <h2>0.24.6 (YANKED: depfile option was breaking, see <a href="https://redirect.github.com/mozilla/cbindgen/issues/841">#841</a>)</h2> <pre><code> * Update MSRV to 1.57 * Support variadic arguments (`...`) ([#805](mozilla/cbindgen#805)) * Add --depfile option ([#820](mozilla/cbindgen#820)) </code></pre> <h2>0.24.5</h2> <pre><code> * Don't enforce tempfile version. </code></pre> <h2>0.24.4</h2> <pre><code> * Move expand infinite recursion fix ([#799](mozilla/cbindgen#799)) * Add with_cpp_compat to the builder ([#796](mozilla/cbindgen#796)) * Handle never type in return position consistently ([#780](mozilla/cbindgen#780)) * Fix warnings ([#816](mozilla/cbindgen#816), [#819](mozilla/cbindgen#819)) * Updated documentation ([#788](mozilla/cbindgen#788), [#791](mozilla/cbindgen#791), [#792](mozilla/cbindgen#792), [#810](mozilla/cbindgen#810), [#823](mozilla/cbindgen#823)) </code></pre> <h2>0.24.3</h2> <pre><code> * Make struct expressions correctly generated through typedefs ([#768](mozilla/cbindgen#768)). </code></pre> <h2>0.24.2</h2> <pre><code> * Make bitfield operators use explicit constructors. </code></pre> <h2>0.24.1</h2> <pre><code> * Add support for unary negation ([#765](mozilla/cbindgen#765)). * Make more bitfield operators constexpr ([#765](mozilla/cbindgen#765)). </code></pre> <h2>0.24.0</h2> <pre><code> * Basic const generic support ([#759](mozilla/cbindgen#759), [#760](mozilla/cbindgen#760) [#762](mozilla/cbindgen#762)). </code></pre>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/mozilla/cbindgen/commit/703b53c06f9fe2dbc0193d67626558cfa84a0f62"><code>703b53c</code></a> v0.26.0</li> <li><a href="https://github.com/mozilla/cbindgen/commit/56f0febc9b5cc7ee957912d55b1fdee67f34d766"><code>56f0feb</code></a> Update MSRV in Readme</li> <li><a href="https://github.com/mozilla/cbindgen/commit/9b4a14958ea6ce2a61accb9cdbad84cfca9c3013"><code>9b4a149</code></a> Add support for out-of-line bitfields declarations</li> <li><a href="https://github.com/mozilla/cbindgen/commit/35f2e44ef2792d1c2e9e97f8ccf4458b79ae88f2"><code>35f2e44</code></a> Update URLs</li> <li><a href="https://github.com/mozilla/cbindgen/commit/85eb0f4436e90d479f5d041b98f160ce2e66eeea"><code>85eb0f4</code></a> Bump clippy msrv to 1.64</li> <li><a href="https://github.com/mozilla/cbindgen/commit/43af1ebe6e4d2bb9ec90167616a3d88bf34c7c0b"><code>43af1eb</code></a> Handle bitflags bits method calls</li> <li><a href="https://github.com/mozilla/cbindgen/commit/f72e44715697981b9317b7f0688216990e60346d"><code>f72e447</code></a> CHANGES: Note #[deprecated] support.</li> <li><a href="https://github.com/mozilla/cbindgen/commit/14730702306533c2805c9dd4a6846e762a4bbcca"><code>1473070</code></a> utilities: annotation: Clean-up deprecated parsing and getter.</li> <li><a href="https://github.com/mozilla/cbindgen/commit/0fb5d07ae4ede7d3fcf91c47bcfa29090ce0a8b9"><code>0fb5d07</code></a> Add support for #[deprecated].</li> <li><a href="https://github.com/mozilla/cbindgen/commit/d8355da4664f68d399631f2f215cbf991c121d2f"><code>d8355da</code></a> Support "C-unwind" ABI</li> <li>Additional commits viewable in <a href="https://github.com/mozilla/cbindgen/compare/v0.25.0...0.26.0">compare view</a></li> </ul> </details> <br /> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

The new v0.25.0 release of cbindgen adds a [`--depfile` option](mozilla/cbindgen#820), that CMake can use to determine when to re-run cbindgen. This feature requires the `DEPFILE` parameter of `add_custom_command` and is available on most generators by now. If CMake is too old, or the Generator does not support `DEPFILE`, then we fallback to always rerunning cbindgen as before.

jschwe mentioned this pull request Mar 9, 2023

Add support for outputting dependencies #192

Closed

jschwe force-pushed the depfile branch 2 times, most recently from 2ba92fa to 5a78d77 Compare March 9, 2023 16:18

emilio requested changes Mar 10, 2023

View reviewed changes

jschwe force-pushed the depfile branch from 01ab9e8 to 2ab3583 Compare March 11, 2023 08:30

jschwe force-pushed the depfile branch 4 times, most recently from 1291bb5 to 9f427e5 Compare March 12, 2023 10:12

jschwe requested a review from emilio March 12, 2023 10:48

emilio requested changes May 29, 2023

View reviewed changes

src/bindgen/parser.rs Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

tests/depfile.rs Outdated Show resolved Hide resolved

tests/tests.rs Show resolved Hide resolved

jschwe and others added 2 commits May 29, 2023 18:15

Improve cbindgen detection in tests

c613aff

Since our MSRV is now Rust 1.54, we can rely on cargo setting `CARGO_BIN_EXE_cbindgen` to the cbindgen path in integration tests. This is more reliable than guessing the path, since cargo knows where it placed the bin.

jschwe force-pushed the depfile branch from b5ab006 to 89c4378 Compare May 29, 2023 16:18

emilio approved these changes May 29, 2023

View reviewed changes

emilio merged commit 25132a3 into mozilla:master May 29, 2023
2 checks passed

tronical mentioned this pull request May 31, 2023

Build breaks with v0.24.6 #841

Closed

jschwe deleted the depfile branch June 4, 2023 20:04

jschwe mentioned this pull request Aug 25, 2023

Improve cbindgen by regenerating bindings only when necessary corrosion-rs/corrosion#435

Merged

jschwe mentioned this pull request Apr 11, 2024

corrosion_experimental_cbindgen always triggers rebuilds of files that include the generated header corrosion-rs/corrosion#503

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --depfile option #820

Add --depfile option #820

jschwe commented Mar 9, 2023 •

edited

emilio left a comment

emilio Mar 10, 2023

jschwe Mar 10, 2023

emilio Mar 10, 2023

jschwe Mar 10, 2023 •

edited

jschwe commented Mar 10, 2023

jschwe commented Mar 11, 2023 •

edited

jschwe commented Mar 12, 2023

jschwe commented Mar 23, 2023

jschwe commented Apr 13, 2023

jschwe commented May 19, 2023

emilio left a comment

emilio May 29, 2023

jschwe May 29, 2023 •

edited

jschwe May 29, 2023

jschwe commented May 29, 2023

emilio left a comment

emilio commented May 29, 2023

jschwe commented May 29, 2023

emilio left a comment

Add --depfile option #820

Add --depfile option #820

Conversation

jschwe commented Mar 9, 2023 • edited

emilio left a comment

Choose a reason for hiding this comment

emilio Mar 10, 2023

Choose a reason for hiding this comment

jschwe Mar 10, 2023

Choose a reason for hiding this comment

emilio Mar 10, 2023

Choose a reason for hiding this comment

jschwe Mar 10, 2023 • edited

Choose a reason for hiding this comment

jschwe commented Mar 10, 2023

jschwe commented Mar 11, 2023 • edited

jschwe commented Mar 12, 2023

jschwe commented Mar 23, 2023

jschwe commented Apr 13, 2023

jschwe commented May 19, 2023

emilio left a comment

Choose a reason for hiding this comment

emilio May 29, 2023

Choose a reason for hiding this comment

jschwe May 29, 2023 • edited

Choose a reason for hiding this comment

jschwe May 29, 2023

Choose a reason for hiding this comment

jschwe commented May 29, 2023

emilio left a comment

Choose a reason for hiding this comment

emilio commented May 29, 2023

jschwe commented May 29, 2023

emilio left a comment

Choose a reason for hiding this comment

jschwe commented Mar 9, 2023 •

edited

jschwe Mar 10, 2023 •

edited

jschwe commented Mar 11, 2023 •

edited

jschwe May 29, 2023 •

edited