Repository: google/autocxx (rust)

Issue:
Rebuild when any included header file changes
We should ensure we output `cargo:rerun-if-changed=src/input.h` for each header file which is transitively included. We can do this using https://docs.rs/bindgen/0.56.0/bindgen/struct.CargoCallbacks.html but we probably want a more generic interface such that `gen/cmd` can do something sensible (e.g. generate a `.d` file) as well as `gen/build`.

Pull request:
Request cargo rebuild whenever any header changes.
We use bindgen callbacks to detect all header files
transitively 'included' within our Rust files, and
issue appropriate println!s to cargo such that it
knows to rerun the build script every time any such
file changes.

Surprisingly complex for such a simple task.

Fixes #173.

Patch:
diff --git a/engine/src/builder.rs b/engine/src/builder.rs
index 8121c2e0..7c913234 100644
--- a/engine/src/builder.rs
+++ b/engine/src/builder.rs
@@ -15,7 +15,7 @@
 use autocxx_parser::file_locations::FileLocationStrategy;
 use proc_macro2::TokenStream;
 
-use crate::{ParseError, ParsedFile};
+use crate::{ParseError, ParsedFile, RebuildDependencyRecorder};
 use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::{ffi::OsStr, io, process};
@@ -62,24 +62,32 @@ pub type BuilderResult = Result<BuilderSuccess, BuilderError>;
 /// more from a build.rs file.
 /// You need to provide the Rust file path and the iterator of paths
 /// which should be used as include directories.
-pub fn build<P1, I, T>(rs_file: P1, autocxx_incs: I) -> BuilderResult
+pub fn build<P1, I, T>(
+    rs_file: P1,
+    autocxx_incs: I,
+    dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
+) -> BuilderResult
 where
     P1: AsRef<Path>,
     I: IntoIterator<Item = T>,
     T: AsRef<OsStr>,
 {
-    build_to_custom_directory(rs_file, autocxx_incs, None)
+    build_to_custom_directory(rs_file, autocxx_incs, None, dependency_recorder)
 }
 
 /// Builds successfully, or exits the process displaying a suitable
 /// message.
-pub fn expect_build<P1, I, T>(rs_file: P1, autocxx_incs: I) -> BuilderSuccess
+pub fn expect_build<P1, I, T>(
+    rs_file: P1,
+    autocxx_incs: I,
+    dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
+) -> BuilderSuccess
 where
     P1: AsRef<Path>,
     I: IntoIterator<Item = T>,
     T: AsRef<OsStr>,
 {
-    build(rs_file, autocxx_incs).unwrap_or_else(|err| {
+    build(rs_file, autocxx_incs, dependency_recorder).unwrap_or_else(|err| {
         let _ = writeln!(io::stderr(), "\n\nautocxx error: {}\n\n", report(err));
         process::exit(1);
     })
@@ -95,6 +103,7 @@ pub(crate) fn build_to_custom_directory<P1, I, T>(
     rs_file: P1,
     autocxx_incs: I,
     custom_gendir: Option<PathBuf>,
+    dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
 ) -> BuilderResult
 where
     P1: AsRef<Path>,
@@ -124,7 +133,7 @@ where
 
     let mut parsed_file = crate::parse_file(rs_file).map_err(BuilderError::ParseError)?;
     parsed_file
-        .resolve_all(&autocxx_inc)
+        .resolve_all(&autocxx_inc, dependency_recorder)
         .map_err(BuilderError::ParseError)?;
     build_with_existing_parsed_file(parsed_file, cxxdir, incdir, rsdir)
 }
diff --git a/engine/src/lib.rs b/engine/src/lib.rs
index 7e860811..e73b5010 100644
--- a/engine/src/lib.rs
+++ b/engine/src/lib.rs
@@ -19,6 +19,7 @@ mod conversion;
 mod function_wrapper;
 mod known_types;
 mod parse;
+mod parse_callbacks;
 mod rust_pretty_printer;
 mod type_to_cpp;
 mod typedef_analyzer;
@@ -32,6 +33,7 @@ mod integration_tests;
 
 use autocxx_parser::{CppInclusion, IncludeCppConfig, UnsafePolicy};
 use conversion::BridgeConverter;
+use parse_callbacks::AutocxxParseCallbacks;
 use proc_macro2::TokenStream as TokenStream2;
 use std::{
     collections::hash_map::DefaultHasher,
@@ -129,6 +131,15 @@ enum State {
     Generated(Box<GenerationResults>),
 }
 
+/// Implement to learn of header files which get included
+/// by this build process, such that your build system can choose
+/// to rerun the build process if any such file changes in future.
+pub trait RebuildDependencyRecorder: std::fmt::Debug {
+    /// Records that this autocxx build depends on the given
+    /// header file. Full paths will be provided.
+    fn record_header_file_dependency(&self, filename: &str);
+}
+
 /// Core of the autocxx engine. See `generate` for most details
 /// on how this works.
 ///
@@ -274,7 +285,11 @@ impl IncludeCppEngine {
     /// Along the way, the `bridge_converter` might tell us of additional
     /// C++ code which we should generate, e.g. wrappers to move things
     /// into and out of `UniquePtr`s.
-    pub fn generate(&mut self, inc_dirs: &str) -> Result<()> {
+    pub fn generate(
+        &mut self,
+        inc_dirs: &str,
+        dep_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
+    ) -> Result<()> {
         let inc_dirs = Self::canonize_inc_dirs(inc_dirs)?;
 
         // If we are in parse only mode, do nothing. This is used for
@@ -290,7 +305,10 @@ impl IncludeCppEngine {
             return Err(Error::NoGenerationRequested);
         }
 
-        let builder = self.make_bindgen_builder(&inc_dirs);
+        let mut builder = self.make_bindgen_builder(&inc_dirs);
+        if let Some(dep_recorder) = dep_recorder {
+            builder = builder.parse_callbacks(Box::new(AutocxxParseCallbacks(dep_recorder)));
+        }
         let bindings = self
             .inject_header_into_bindgen(builder)
             .generate()
diff --git a/engine/src/parse.rs b/engine/src/parse.rs
index 33c0327e..992379a5 100644
--- a/engine/src/parse.rs
+++ b/engine/src/parse.rs
@@ -12,11 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::{Error as EngineError, IncludeCppEngine};
+use crate::{Error as EngineError, IncludeCppEngine, RebuildDependencyRecorder};
 use proc_macro2::TokenStream;
 use quote::ToTokens;
-use std::path::Path;
 use std::{fmt::Display, io::Read};
+use std::{panic::UnwindSafe, path::Path, rc::Rc};
 use syn::Item;
 
 /// Errors which may occur when parsing a Rust source file to discover
@@ -112,10 +112,23 @@ impl ParsedFile {
             .collect()
     }
 
-    pub fn resolve_all(&mut self, autocxx_inc: &str) -> Result<(), ParseError> {
+    pub fn resolve_all(
+        &mut self,
+        autocxx_inc: &str,
+        dep_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
+    ) -> Result<(), ParseError> {
+        let inner_dep_recorder: Option<Rc<dyn RebuildDependencyRecorder>> =
+            dep_recorder.map(Rc::from);
         for include_cpp in self.get_autocxxes_mut() {
+            let dep_recorder: Option<Box<dyn RebuildDependencyRecorder>> = match &inner_dep_recorder
+            {
+                None => None,
+                Some(inner_dep_recorder) => Some(Box::new(CompositeDepRecorder::new(
+                    inner_dep_recorder.clone(),
+                ))),
+            };
             include_cpp
-                .generate(autocxx_inc)
+                .generate(autocxx_inc, dep_recorder)
                 .map_err(ParseError::MacroParseFail)?
         }
         Ok(())
@@ -135,3 +148,22 @@ impl ToTokens for ParsedFile {
         }
     }
 }
+
+/// Shenanigans required to share the same RebuildDependencyRecorder
+/// with all of the include_cpp instances in this one file.
+#[derive(Debug, Clone)]
+struct CompositeDepRecorder(Rc<dyn RebuildDependencyRecorder>);
+
+impl CompositeDepRecorder {
+    fn new(inner: Rc<dyn RebuildDependencyRecorder>) -> Self {
+        CompositeDepRecorder(inner)
+    }
+}
+
+impl UnwindSafe for CompositeDepRecorder {}
+
+impl RebuildDependencyRecorder for CompositeDepRecorder {
+    fn record_header_file_dependency(&self, filename: &str) {
+        self.0.record_header_file_dependency(filename);
+    }
+}
diff --git a/engine/src/parse_callbacks.rs b/engine/src/parse_callbacks.rs
new file mode 100644
index 00000000..a4292c3a
--- /dev/null
+++ b/engine/src/parse_callbacks.rs
@@ -0,0 +1,29 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::panic::UnwindSafe;
+
+use crate::RebuildDependencyRecorder;
+use autocxx_bindgen::callbacks::ParseCallbacks;
+
+#[derive(Debug)]
+pub(crate) struct AutocxxParseCallbacks(pub Box<dyn RebuildDependencyRecorder>);
+
+impl UnwindSafe for AutocxxParseCallbacks {}
+
+impl ParseCallbacks for AutocxxParseCallbacks {
+    fn include_file(&self, filename: &str) {
+        self.0.record_header_file_dependency(filename);
+    }
+}
diff --git a/gen/build/src/lib.rs b/gen/build/src/lib.rs
index 65b94fb0..efac1387 100644
--- a/gen/build/src/lib.rs
+++ b/gen/build/src/lib.rs
@@ -14,6 +14,7 @@
 
 use autocxx_engine::{
     build as engine_build, expect_build as engine_expect_build, BuilderBuild, BuilderError,
+    RebuildDependencyRecorder,
 };
 use std::io::Write;
 use std::{ffi::OsStr, path::Path};
@@ -29,7 +30,12 @@ where
     T: AsRef<OsStr>,
 {
     setup_logging();
-    engine_build(rs_file, autocxx_incs).map(|r| r.0)
+    engine_build(
+        rs_file,
+        autocxx_incs,
+        Some(Box::new(CargoRebuildDependencyRecorder)),
+    )
+    .map(|r| r.0)
 }
 
 /// Builds successfully, or exits the process displaying a suitable
@@ -41,7 +47,12 @@ where
     T: AsRef<OsStr>,
 {
     setup_logging();
-    engine_expect_build(rs_file, autocxx_incs).0
+    engine_expect_build(
+        rs_file,
+        autocxx_incs,
+        Some(Box::new(CargoRebuildDependencyRecorder)),
+    )
+    .0
 }
 
 fn setup_logging() {
@@ -49,3 +60,12 @@ fn setup_logging() {
         .format(|buf, record| writeln!(buf, "cargo:warning=MESSAGE:{}", record.args()))
         .init();
 }
+
+#[derive(Debug)]
+struct CargoRebuildDependencyRecorder;
+
+impl RebuildDependencyRecorder for CargoRebuildDependencyRecorder {
+    fn record_header_file_dependency(&self, filename: &str) {
+        println!("cargo:rerun-if-changed={}", filename);
+    }
+}
diff --git a/gen/cmd/src/main.rs b/gen/cmd/src/main.rs
index c5960f68..9059d9cd 100644
--- a/gen/cmd/src/main.rs
+++ b/gen/cmd/src/main.rs
@@ -89,8 +89,10 @@ fn main() {
     let mut parsed_file = parse_file(matches.value_of("INPUT").unwrap())
         .expect("Unable to parse Rust file and interpret autocxx macro");
     let incs = matches.value_of("inc").unwrap_or("");
+    // TODO - in future, we should provide an option to write a .d file here
+    // by passing a callback into the dep_recorder parameter here.
     parsed_file
-        .resolve_all(incs)
+        .resolve_all(incs, None)
         .expect("Unable to resolve macro");
     let outdir: PathBuf = matches.value_of_os("outdir").unwrap().into();
     if let Some(matches) = matches.subcommand_matches("gen-cpp") {


Test patch:
diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs
index c7a476ca..33beec17 100644
--- a/engine/src/integration_tests.rs
+++ b/engine/src/integration_tests.rs
@@ -229,6 +229,7 @@ fn do_run_test(
         &rs_path,
         &[tdir.path()],
         Some(target_dir.clone()),
+        None,
     )
     .map_err(TestError::AutoCxx)?;
     let mut b = build_results.0;