CargoCallbacks does not trigger rerun if the wrapper header changes

[CrendKing]

Input C/C++ Header

wrapper.h:

const int test = 0;

Bindgen Invocation

bindgen::Builder::default()
    .header("wrapper.h")
    .parse_callbacks(Box::new(bindgen::CargoCallbacks))
    .generate()
    .unwrap()

Actual Results

If I change the test value to 1, the generated binding should update.

Expected Results

No rerun happens at all.

It seems only the headers transitively included by the wrapper.h trigger rerun. Shouldn't it also automatically include the wrapper header?

[pvdrz]

I think including the wrapper could be a sensible default but I'm not sure as this behavior has not been the default and it could break other's people code in unexpected ways (I'm amazed to see what people do with bindgen 🤣)

One possible justification for this behavior is that the wrapper.h file should be minimal and just #include the actual libraries you want to use. However, it is a bit annoying having to cargo clean every time you change your wrapper.h if you want to ensure that your changes are taken into account.

A possible solution here would be to document this more explicitly on the CargoCallbacks docs by mentioning that the files passed to .header are not included in this check and that a rerun-if-changed line for that file must be printed manually.

[CrendKing]

wrapper.h file should be minimal

That's exactly my case, except when something like #753 exists, and I need to add workaround like #753 (comment), which triggered me to discover that I also need explicit println!("cargo:rerun-if-changed=wrapper.h"); line in main().

Granted, pretty much every tutorial about bindgen out there already has that line. Heck, those tutorials also have CargoCallbacks. Otherwise I think it is sensible to both include the rerun and CargoCallbacks as default. I mean, when do you NOT want the recompilation if the main header changes?

[pvdrz]

I mean, when do you NOT want the recompilation if the main header changes?

The issue is that I don't have an answer for this 😅. I don't see any reason to not recompile when the input header changes but somebody might be relying on this for some weird reason.

[CrendKing]

Is there backward incompatible way to change the default and give people option to turn it off? If not, I still suggest at least explicitly mention the necessity of the wrapper rerun line in doc. The current tutorial doesn't

[pvdrz]

There might be a not so pretty but backwards compatible way to implement this:

pub trait ParseCallbacks {}

#[allow(dead_code)]
pub struct CargoCallbacks { include_input_headers: bool }

#[allow(non_upper_case_globals)]
const CargoCallbacks: CargoCallbacks = CargoCallbacks { include_input_headers: false };

impl CargoCallbacks {
    fn include_input_headers(self) -> Self {
        Self { include_input_headers: true } 
    }
}

impl ParseCallbacks for CargoCallbacks {}

pub fn parse_callbacks(_cb: Box<dyn ParseCallbacks>) {}

fn main() {
    // This still works
    parse_callbacks(Box::new(CargoCallbacks));
    // But you can use this
    parse_callbacks(Box::new(CargoCallbacks.include_input_headers()));
}

[CrendKing]

Sure, but then how good is the discoverability of include_input_headers()? It might be equivalent if we add a rerun-if-changed-main-header() in the Builder and leave CargoCallbacks intact and be cleaner than this. Then again, how to make sure unawared users would know the main header is not monitored and should call that to begin with? That's why I was suggesting changing the default.

[pvdrz]

Well... one option would be to mark the CargoCallbacks constructor as deprecated and introduce a new builder CargoCallbacks::new or maybe just CargoCallbacks::default where include_input_headers is set to true. In that way we wouldn't break anyone's code relying on the old behavior and warn them that this will change in the future using the deprecation message.

[pvdrz]

@CrendKing I implemented this on #2653. Could you test it to see if it does what you expect?

[CrendKing]

Yes, it solves the issue. My only nitpick is that I hope people understand that the file they put in header() is the same as input_file in the callbacks. It'd really nice if they are the same, or at least easy to connect (e.g. input_header etc.). Otherwise, thank you!

[pvdrz]

I renamed the input_file method to header_file and explicitly documented that the filename passed to it are the ones the user passed to Builder::header.