Add error message for invalid include! paths #368
Conversation
Extracts the used include paths in the parsing step, and propagates them up to the cxxbridge build, which performs the actual check.
| @@ -28,6 +29,7 @@ impl<'a> Types<'a> { | |||
| let mut aliases = Map::new(); | |||
| let mut untrusted = Map::new(); | |||
| let mut explicit_impls = Set::new(); | |||
| let mut include_dirs = UnorderedSet::new(); | |||
There was a problem hiding this comment.
This needs to be an ordered set because gen/src/mod.rs contains code that iterates over it. The only valid thing to do with an unordered set is testing for membership. Otherwise you end up with the code generator being nondeterministic which is never good.
| } | ||
|
|
||
| // Include prefix is stored with backslashes under Windows -> very basic slash conversion (use path-slash for more sophisticated one) | ||
| panic!("include!(\"{}\") is invalid:\n path must either start with include prefix '{}' or be an absolute path.\n\n", |
There was a problem hiding this comment.
This shouldn't requiring panicking the build script. It should report the error the same way that type errors are reported, which displays the line number and source line of code similar to rustc diagnostics.
| @@ -298,7 +302,61 @@ fn generate_bridge(prj: &Project, build: &mut Build, rust_source_file: &Path) -> | |||
| Ok(()) | |||
| } | |||
|
|
|||
| // Ensures that the argument of the include!() macro is either starting with the crate name, or an absolute path | |||
There was a problem hiding this comment.
I think this check is somewhat too strict. For example the caller's build script is free to make any other directories available here and that wouldn't be a problem. Using https://crates.io/crates/pkg-config this could be a common pattern:
let python3 = pkg_config::probe_library("python3")?;
cxx_build::bridge("src/bridge.rs")
.includes(python3.include_paths)
.compile("repro");extern "C" {
include("Python.h");
...
}There was a problem hiding this comment.
Under that circumstance, would it even be possible to check extra include directories inside cxx?
We give the cc::Build object to the user and effectively lose control of how it's further configured.
|
Okay. Do you think the aggregation code I added in syntax/types.rs to collect all |
|
Probably not. We do almost nothing with includes in this crate, and all of it is only iterating over them (to insert them into the output code). Iterating is easily done on only the syntax tree -- it doesn't require a more structured representation than that. The sets/maps in syntax/types.rs are for things where the generator needs to do more complicated lookup of type information. Lines 25 to 29 in 353d98c |
|
Makes sense, thanks for the explanation! 🙂 |
As promised in #367, here's the pull request for an error message when
include!receives a path that it cannot handle. This is to avoid that such errors propagate further down and only fail at the C++ compile step (and was a reason I spend a lot of time debugging my cxx examples).The logic is: path must begin with the configured include prefix (crate name by default) or must be an absolute path.
Remarks:
include!means depends on the user of the parser (can be an external crate such as autocxx). So I implemented it close to the bridge generation.include!statements, but integrate them with theTypesstruct and carry that information upward several levels.verify_include_dir()function, but I'm not sure what's the best location for them. I cannot move them to another crate without making the function under test public.tests/cxx_gen.rs). I did some manual tests however. Probably you know much better how and where to add extra tests, if it's even necessary (this PR is primarily a quality-of-life/diagnostic improvement)