Should have better error messages when input files aren't files

[helloqirun]

The hashtag for my bindgen version is 4dd4ac7 .

folder is just an empty directory. Should we provide a more friendly error message?

$ RUST_BACKTRACE=1 bindgen folder/

thread 'main' panicked at 'TranslationUnit::parse failed', /checkout/src/libcore/option.rs:819:4
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at /checkout/src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at /checkout/src/libstd/sys_common/backtrace.rs:60
             at /checkout/src/libstd/panicking.rs:380
   3: std::panicking::default_hook
             at /checkout/src/libstd/panicking.rs:396
   4: std::panicking::rust_panic_with_hook
             at /checkout/src/libstd/panicking.rs:611
   5: std::panicking::begin_panic_new
             at /checkout/src/libstd/panicking.rs:553
   6: std::panicking::begin_panic_fmt
             at /checkout/src/libstd/panicking.rs:521
   7: rust_begin_unwind
             at /checkout/src/libstd/panicking.rs:497
   8: core::panicking::panic_fmt
             at /checkout/src/libcore/panicking.rs:92
   9: core::option::expect_failed
             at /checkout/src/libcore/option.rs:819
  10: <core::option::Option<T>>::expect
             at /checkout/src/libcore/option.rs:302
  11: bindgen::ir::context::BindgenContext::new
             at src/ir/context.rs:348
  12: bindgen::Bindings::generate
             at src/lib.rs:1447
  13: bindgen::Builder::generate
             at src/lib.rs:978
  14: bindgen::main::{{closure}}
             at src/main.rs:55
  15: std::panicking::try::do_call
             at /checkout/src/libstd/panicking.rs:479
  16: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:98
  17: std::panicking::try
             at /checkout/src/libstd/panicking.rs:458
  18: std::panic::catch_unwind
             at /checkout/src/libstd/panic.rs:361
  19: bindgen::main
             at src/main.rs:54
  20: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:98
  21: std::rt::lang_start
             at /checkout/src/libstd/panicking.rs:458
             at /checkout/src/libstd/panic.rs:361
             at /checkout/src/libstd/rt.rs:59
  22: main
  23: __libc_start_main
  24: _start

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[fitzgen]

Thanks for the bug report!

To whoever wants to pick this up: the Bindings::generate function in src/lib.rs should loop through each of the options.input_headers and verify that they are paths to files (not folders; see std::fs::Metadata::is_file) and that we have read permissions to access them (std::fs::Permissions from std::fs::Metadata::permissions). If we don't have read access, or one isn't a file, then we should print a relevant error message.

[ghost]

@highfive: assign me
I would like to have a go at this

[highfive]

Hey @seemyvest! Thanks for your interest in working on this issue. It's now assigned to you!

[fitzgen]

@seemyvest if you have any questions don't hesitate to ask :)

[ghost]

Sorry it's been a while, I do have a few questions.

• Do we still want to panic? I was thinking that we catch it here before generating the bindings.
• Is only <header> to be checked, or the arguments passed to clang as well? As far as I can tell there's only one <header> argument, but the way you described it it sounds like there can be more than one.
• The only way to check permissions with std::fs::Permissions is with readonly, which actually only returns true if no write permissions are set (the Linux implementation). Without using std::os::unix::PermissionsExt I'm not sure how to check if the read bit is set, but that won't work with Windows.

Thanks for the help :)

[fitzgen]

Do we still want to panic? I was thinking that we catch it here before generating the bindings.

Ideally we won't panic, and will return an Err instead. This is why I pointed out Bindings::generate in src.lib.rs as a good place to put these checks, because it returns a Result.

Is only

to be checked, or the arguments passed to clang as well? As far as I can tell there's only one argument, but the way you described it it sounds like there can be more than one.

The other benefit to doing the checking inside Bindings::generate is that the checking can apply to both when folks are using bindgen as an executable and when using it as a library in build.rs. Indeed, the executable only takes one header, but the bindgen::Builder::header method, when using bindgen as a library, can be called any number of times to generate bindings to multiple header files. That's why options.input_headers is a Vec.

The only way to check permissions with std::fs::Permissions is with readonly, which actually only returns true if no write permissions are set (the Linux implementation). Without using std::os::unix::PermissionsExt I'm not sure how to check if the read bit is set, but that won't work with Windows.

readonly clearly isn't what we want :-p

How about two versions of the function?

#[cfg(unix)]
fn can_read(perms: &std::fs::Permissions) -> bool {
    use std::os::unix::PermissionsExt;
    perms.mode() & 0o0444 > 0
}

#[cfg(not(unix))]
fn can_read(_: &std::fs::Permissions) -> bool {
    true
}

Thanks for the help :)

No problem! Let me know if anything I said isn't clear :)

[fitzgen]

Fixed in #1092