[Rust] Implement flatc build script wrapper #5252
Conversation
| /// .compile(); | ||
| /// } | ||
| /// ``` | ||
| pub fn schema<P: AsRef<Path>>(&mut self, file: P) -> &mut Self { |
There was a problem hiding this comment.
Wasn't sure what to name this argument as (adding FBS files). I think it might be better of as distinct from --schema.
rust/flatc/Cargo.toml
Outdated
| license = "Apache-2.0" | ||
| edition = "2018" | ||
| build = "build.rs" | ||
| description = "flatc Rust build script wrapper" |
There was a problem hiding this comment.
I think we need to make it clear that this new Cargo package is only part of the story -- how about making this description read: Companion package to flatbuffers, to generate code at compile time.?
rust/flatc/src/lib.rs
Outdated
|
|
||
| //! # flatc | ||
| //! | ||
| //! A library for build scripts to compile FlatBuffer schemas. |
There was a problem hiding this comment.
Again I think we need to point explicitly to the main flatbuffers package so that users don't get confused. Maybe a description like: Companion package to flatbuffers, to generate code at compile time.
rust/flatc/src/lib.rs
Outdated
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| write!( | ||
| f, | ||
| "An {} error occurred during FlatBuffer compilation: {}", |
There was a problem hiding this comment.
How about we add a phrase here to say: "with crate-provided flatc"? Might save users a lot of time debugging flatc versions, especially if they have it installed in /usr/local/bin or something, as mac users do.
There was a problem hiding this comment.
I've extended the error reporting to include the path of the executable compiled by the crate and the commit SHA that it was built from.
|
@rjsberry Also there is a CI failure related to how this PR is adding a Cargo Workspace to the project. WDYT about just keeping the crates totally separate? https://travis-ci.org/google/flatbuffers/jobs/509127091#L1018 |
This can be removed. I have always used a workspace when I have multiple crates in one repository but cargo can locate the crate under |
|
wrt. testing there is a small test in |
|
|
|
||
| impl Build { | ||
| #[doc(hidden)] | ||
| pub fn output<P: AsRef<Path>>(&mut self, dir: P) -> &mut Self { |
There was a problem hiding this comment.
Not happy with having this as part of the public interface and simply hidden from the docs (as it would permit misuse of Builder), but we need to do this to test the crate from /tests/rust_usage_test.
|
Is there a reason why the Travis pipeline is running with |
|
It seems that the CMake components in How do you think we should resolve this? A couple of ideas come to mind:
|
|
Not sure, yet. @aardappel any thoughts? |
|
I'm not an expert on the technologies involved, but would it make most sense to have the build for the crate be completely isolated in a subdir, such that it builds its own Or, if we don't care to test that, can it reuse the existing |
|
We could use the existing flatc, but I think it's important we verify the CMake build done by the crate as part of CI. Seems like there's also an open issue for arch handling of Visual Studio generators in the |
rjsberry
force-pushed
the
rust/flatc
branch
2 times, most recently
from
6d6d267
to
db32c26
Compare
rjsberry
force-pushed
the
rust/flatc
branch
4 times, most recently
from
514fd05
to
0dc343c
Compare
| use std::{env, fs, path::Path}; | ||
|
|
||
| #[test] | ||
| fn build_script_wrapper() { |
There was a problem hiding this comment.
@rjsberry WDYT about comparing that the contents are the same as monster_test_generated.rs?
There was a problem hiding this comment.
Sounds reasonable. I've run a quick check locally and am seeing quite a few differences. When and how is monster_test_generated.rs generated?
There was a problem hiding this comment.
@rjsberry It's the result of generate_code.sh
aardappel
force-pushed
the
master
branch
2 times, most recently
from
f6ab5c4
to
f381fd7
Compare
aardappel
force-pushed
the
master
branch
5 times, most recently
from
f116b80
to
f12cca8
Compare
|
This pull request is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days. |
See also #5216.
This PR implements a wrapper around
flatcusing Rust code which is intended to be used in Rust build script files. Once merged it will be immediately useable:@aardappel @rw Do you have any thoughts on distribution via crates.io?
For working with a specific commit of
flatc, the above is sufficient and pushing to crates.io is not required. However with the current structure (crate as a subdirectory) it will not be possible to use the build script with revisions offlatccommitted before the wrapper.For crates.io distribution we could push on releases of FlatBuffers and tie the commit of this wrapper with the release version.