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

Support emitting Makefile-syntax depfiles like gcc/clang/rustc. #2026

Merged
merged 2 commits into from Apr 23, 2021

Conversation

anp
Copy link
Member

@anp anp commented Apr 8, 2021

The motivation here is being able to use bindgen during a ninja build without losing dependency information.

I'm not particularly happy with the approach I've taken of wrapping ParseCallbacks. The main alternative I see is to have the parse callbacks separated into a concrete type and a trait object, and to always collect filenames in the struct. I'm open to suggestions for better ways to collect the needed metadata, though.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

So the goal of this PR seems worth it, but I believe this could be much simpler, I'm not a fan of the way this has to wrap the parsing callbacks to work. Is there any reason this actually needs to use the parse callbacks mechanism at all?

It seems it only cares about include_file() (and the options), right? It should be straight-forward to implement it using something like this, in a way that involves no breaking changes, or am I missing something?

diff --git a/src/ir/context.rs b/src/ir/context.rs
index ccb05e75..8b3da71d 100644
--- a/src/ir/context.rs
+++ b/src/ir/context.rs
@@ -465,6 +465,10 @@ pub struct BindgenContext {
     /// Populated when we enter codegen by `compute_has_float`; always `None`
     /// before that and `Some` after.
     has_float: Option<HashSet<ItemId>>,
+
+    /// The file dependencies for the bindings, if we're using the `depfile`
+    /// option.
+    deps: BTreeSet<String>,
 }
 
 /// A traversal of allowlisted items.
@@ -579,6 +583,8 @@ If you encounter an error missing from this list, please file an issue or a PR!"
             have_destructor: None,
             has_type_param_in_array: None,
             has_float: None,
+            // Or maybe account for the options here instead of using default?
+            deps: Default::default(),
         }
     }
 
@@ -632,6 +638,16 @@ If you encounter an error missing from this list, please file an issue or a PR!"
         self.options().parse_callbacks.as_ref().map(|t| &**t)
     }
 
+    /// Called on header inclusion.
+    pub fn include_file(&mut self, filename: String) {
+        if let Some(cbs) = self.parse_callbacks() {
+            cbs.include_file(&filename);
+        }
+        if self.options.depfile.is_some() {
+            self.deps.insert(filename);
+        }
+    }
+
     /// Define a new item.
     ///
     /// This inserts it into the internal items set, and its type into the
diff --git a/src/ir/item.rs b/src/ir/item.rs
index 45415045..d7c92ab4 100644
--- a/src/ir/item.rs
+++ b/src/ir/item.rs
@@ -1415,9 +1415,7 @@ impl ClangItemParser for Item {
                             );
                         }
                         Some(filename) => {
-                            if let Some(cb) = ctx.parse_callbacks() {
-                                cb.include_file(&filename)
-                            }
+                            ctx.include_file(filename);
                         }

src/deps.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
@anp
Copy link
Member Author

anp commented Apr 15, 2021

Yeah, that approach is much cleaner, thanks! Addressed comment and have a question on testing (see review comment).

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Thanks, this looks really close!

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
@anp anp force-pushed the depfiles branch 3 times, most recently from 539a905 to e2f6730 Compare April 23, 2021 20:19
bindgen-integration/cpp/Test.h Outdated Show resolved Hide resolved
@anp
Copy link
Member Author

anp commented Apr 23, 2021

OK, I think that's all the review feedback and my TODO resolved. This should be good for another pass.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@emilio emilio merged commit 2cfc64e into rust-lang:master Apr 23, 2021
@anp anp deleted the depfiles branch April 24, 2021 00:28
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

3 participants