Embed custom files.#306
Embed custom files.#306ralfbiedert wants to merge 3 commits intomozilla:masterfrom ralfbiedert:embed
Conversation
|
Note this also addresses #211. |
| let src_file = self | ||
| .root | ||
| .as_ref() | ||
| .expect("Bindings::root must be set when embedding files.") |
There was a problem hiding this comment.
I don't know, using cwd by default, or config location otherwise doesn't seem too bad.
If I specify the path as an absolute file, which is more than likely when ran from build.rs, it doesn't seem necessary to have to provide a root path... But I'm very bad at API design myself, and this seems ok I guess.
Maybe @eqrion can take a look and comment on this?
There was a problem hiding this comment.
I feel strongly that the directory hosting the used cbindgen.toml must be the relative anchor for embeds, as that's the only thing you can rely on without thinking too much. Any working dir is random, and the Cargo.toml can get mildly ambiguous when generating bindings for a sub-project.
There was a problem hiding this comment.
I think having these paths be relative to 'cbindgen.toml' makes sense. I'm not great at API design either, so it's not a strong feeling.
eqrion
left a comment
There was a problem hiding this comment.
This looks good, just one comment. Apologies for the delay.
| let src_file = self | ||
| .root | ||
| .as_ref() | ||
| .expect("Bindings::root must be set when embedding files.") |
There was a problem hiding this comment.
I think having these paths be relative to 'cbindgen.toml' makes sense. I'm not great at API design either, so it's not a strong feeling.
|
|
||
| /// A utility function for build scripts to generate bindings for a crate with a | ||
| /// custom config. | ||
| pub fn generate_with_config<P: AsRef<Path>>( |
There was a problem hiding this comment.
Can we keep this API unchanged and default to no root when providing an in-memory configuration?
There was a problem hiding this comment.
I can do that, but then I would make embed panic if an embed was requested but no root was given.
|
Just an update. After the problem with the unit tests on Mac #330 I finally got my hands on a Windows machine. When I try to run the test cases there ( While the specific error message changes with the type of environment I load, the result is basically that I can't run the test cases to verify what I do works. I will close this pull request for now because we worked around the original embedding problem and I'm just sidetracking. I would like to keep #303 open though because it still has value, and it would have saved us work in the first place. |
Addresses #303.
Note that this is a breaking change, as
generate_with_confignow requires aconfig_rootto be passed, which specifies relative to what embed requests should be loaded.It could theoretically be done without breaking that call, but then either
configwould have to contain implementation details about where it was loaded from, or[embed]is likely to break when used as it doesn't know where to get files from