[Rust] flatc build script wrapper

[rjsberry]

A great improvement to the ergonomics of the Rust crate would be an additional helper crate for use in build scripts to wrap flatc.

From the docs:

To use FlatBuffers in your code, first generate the Rust modules from your schema with the --rust option to flatc. Then you can import both FlatBuffers and the generated code to read or write FlatBuffers.

Really we want to be able to do this in one go.

I can see proper usage being closely modelled after cc:

build.rs

fn main() {
    ::flatc::Build::new()
        .schema("schema/monster.fbs")
        .compile()
    }
}

main.rs

mod monster_generated {
    include!(concat!(env!("OUT_DIR"), "/monster_generated.rs"));
}

fn main() {
    let monster = monster_generated::get_root_as_monster(/* buffer */);
}

I've created a trivial wrapper by shelling out to flatc over on my flatbuffers fork. If we were to extend the FlatBuffers to be usable in this way some minor changes would need to be made to the inner attributes on the generated Rust code.

Can you see extending the Rust implementation of FlatBuffers to handle schema compilation in build scripts?

[rjsberry]

I've just seen #5124 - is there any reason this isn't merged/wasn't considered for part of the main FlatBuffers repository?

[rw]

@rjsberry We don't have any strong opinions on how to do it yet! Do you think there is a clear best way to do the codegen build.rs?

[rjsberry]

I think a builder pattern that wraps shelling out to the schema compiler is the best way to go. If you check out the link to my initial implementation in my original post you should get the gist of how I would personally go about implementing the tool.

If you have used the cc crate before for compiling C/C++ code as part of a build script that should also give you an idea of what we should try to achieve with a FlatBuffers implementation.

It seems that a lot of the command line options with the schema compiler aren't available when targetting Rust output. This simplifies the wrapper quite a bit, and means we can make sure users can only ever build valid commands for Rust output using the wrapper.

Potential Usage

If all options were available, valid usage would look like:

fn main() {
    ::flatc::Build::new()
        .schema("flatbuffers/foo.fbs")
        .schema("flatbuffers/bar.fbs")
        .include_prefix("flatbuffers/extra")
        .gen_mutable()
        .gen_onefile()
        .compile();
}

This would automatically run from CARGO_MANIFEST_DIRECTORY and expand to the shell command:

$ flatc --rust -O "<OUT_DIR>" -I "flatbuffers/foo.fbs" -I "flatbuffers/bar.fbs" --include-prefix "flatbuffers/extra" --gen-mutable --gen-onefile

Where <OUT_DIR> would be expanded from the build script.

Sourcing Generated Code

Output Location

OUT_DIR is the correct place to put the generated source files. Build scripts should not write to CARGO_MANIFEST_DIR. For this reason we shouldn't expose the -o flag, and always set this to OUT_DIR.

For each schema file added to the builder, we place the generated file in OUT_DIR with the standard conventions: filename postfixed with _generated and the extension set to .rs.

Example

As mentioned in my initial post, this will require modification of the generated Rust source files by flatc. This is due to how users will need to include generated code placed in OUT_DIR.

mod monster_generated {
    include!(concat!(env!("OUT_DIR"), "/monster_generated.rs"));
}

With the current version of flatc this expands to code with invalid inner attributes:

mod monster_generated {
    // automatically generated by the FlatBuffers compiler, do not modify


    #![allow(dead_code)] // <-- invalid in this context
    #![allow(unused_imports)] // <-- invalid in this context

    use std::mem;
    use std::cmp::Ordering;

    extern crate flatbuffers;
    use self::flatbuffers::EndianScalar;

    // etc
}

Generating something like the following should resolve this:

mod monster_generated {
    // automatically generated by the FlatBuffers compiler, do not modify


    #[allow(dead_code)]
    #[allow(unused_imports)]
    mod __flatc_codegen {
        use std::mem;
        use std::cmp::Ordering;

        extern crate flatbuffers;
        use self::flatbuffers::EndianScalar;

        // etc
    }

    pub use self::__flatc_codegen::*;
}

Users won't be able to include source files by modifying the path location with the path attribute:

#[path = "<DIR>"]
mod monster_generated;

concat and env cannot be used in place of a literal in this context.

Error Reporting

Generally I think it's safe to assume that if a user is generating code from FlatBuffers schema as part of their crate build and the generation is unsuccessful then we want to explicitly panic and kill the build.

Out of the box cargo produces nice output that the build script was the cause of the failure:

error: failed to run custom build command for `crate v0.1.0 (/home/user/crate)`
process didn't exit successfully: `/home/user/crate/target/debug/build/crate-b372802e65fea7a1/build-script-build` (exit code: 101)
--- stderr
thread 'main' panicked at '

An I/O error occurred during FlatBuffer compilation: No such file or directory (os error 2)

',
note: Run with `RUST_BACKTRACE=1` for a backtrace.

warning: build failed, waiting for other jobs to finish...
error: build failed

Other than I/O error (can't find flatc on PATH) and errors produced by flatc itself I can't think of anything else we might need to report through this interface.

Questions

• Do you agree it's right we ship an official wrapper, or should we leave it to crates like flatc-rust?

• Do you agree that a builder pattern is the best option to go with for wrapping flatc?

• How much of flatc would we need to support in the build script wrapper? Do we want to enforce the -r flag at all times?

[rw]

@aardappel Any thoughts?

[aardappel]

Not too familiar with the Rust build process.. is writing build scripts in Rust itself the regular way to do it?

Also while I understand that for Rustaceans being able to pull flatc via Cargo is awesome, do note that bad things can happen if your flatc is not in sync with the rest of your Rust FlatBuffers code or generated code.. so you'd want to somehow enforce that.

[rjsberry]

@aardappel Yeah, code generation in build scripts is a common pattern seen throughout the Rust community:

• Protobuf: prost-build, protoc-rust
• Cap'n Proto: capnpc.

These generally follow builder patterns as proposed in my original issue. There's also great documentation in the cargo book.

do note that bad things can happen if your flatc is not in sync with the rest of your Rust FlatBuffers code or generated code.. so you'd want to somehow enforce that.

I'm not sure I follow. The idea is for the flatc crate to use std::proccess::Command to call the FlatBuffers schema compiler which must supplied by the user - this isn't distributed via Cargo. Code generation then becomes part of the build process, and it is up to the user to make sure the correct compiler is available to the crate. Do you mean keeping flatc in sync with previously generated code?

Perhaps we could allow an override using an environment variable in addition to searching PATH.

We will also need to ensure that the version of the schema compiler being used is generating code that can work using the include! macro.

Contrary to my original post there is actually an ongoing Rust issue that prevents this macro working with inner attributes. Regardless of the cause we still need to work around this by modifying the schema compiler to remove inner attributes on generated Rust code for the time being.

[aardappel]

What I mean it must be ensured that whatever Cargo pulls in is the same version as whatever flatc is being built from. If the user is entirely independently installing flatc then this will likely not be the case, which will create problems.

flatc is not something you install thru your systems package manager. It must be built along with the runtime code, whenever you update the runtime code.

[rjsberry]

Thanks for the clarification. So you're saying that the build script wrapper should pull flatc source, build with CMake, then invoke its own copy of the flatc executable rather than relying on the user to have a pre-compiled copy lying around somewhere? Definitely do-able. Would you suggest we couple the version of the crate on crates.io with the flatc release number? What about building from master?

Is an officially distributed crate for managing compilation of FlatBuffers schema in Rust build scripts something you would be willing to introduce to the project once we iron out the implementation kinks? I'm certainly keen to see a way to idiomatically automate compilation of schema as part of my Rust build processes, but understand if it's something you don't think is right for the FlatBufers project itself to distribute.

[aardappel]

Yes, that would probably be ideal (compiling flatc). The version to build of flatc needs to match the exact commit of whatever the runtime files are. So if crates.io only ever gets updated upon official releases (which are infrequent), then the version number would suffice. If you want to be able to update crates.io more frequently then.. don't know. Does crates.io store the commit id it was derived from?

Depends what is involved in such an official crate. The dependency I mentioned above has to always be maintained. To me, the even more desirable way would be for whatever build process creates the crate to also include pre-built binaries of flatc, but I'm not sure if the Rust world supports that. Or failing that, that it would include the source to build flatc. In either case you wouldn't need any version matching since it all comes from the same build.

[rjsberry]

Right, so we could tie releases of the crate to releases of flatc.

Using the future release of 1.11.0 as an example, in the Cargo.toml file users would need to put:

[build-dependencies]
flatc = "1.11.0"

If we wanted to go the route of including pre-built binaries things would get complex when it comes to cross compilation. I'm not sure whether tools like cross execute build scripts on the host architecture before executing Rust builds inside its various Docker containers to compile for the different Rust targets, or if it would require a native version of flatc to be built. I think it would be best if we include the flatc source with the crate - then we'll be able to use the cmake crate to build a copy of flatc in OUT_DIR which the build script wrapper would then call to compile schemas.

If you want to be able to update crates.io more frequently then.. don't know. Does crates.io store the commit id it was derived from?

Hm. Yeah that's a tricky one. You can specify "git" dependencies in Cargo.toml files:

[build-dependencies]
flatc = { git = "https://github.com/google/flatbuffers.git", rev = "<SHA>" }

To do this we will need to add a Cargo.toml to the root of the FlatBuffers repository with the following content:

[workspace]
members = [
    "rust/flatbuffers",
    "rust/flatc",
]

Not sure what the best way for us to distribute the source code with the crate will be though. For context, typically with crates that bind to C libraries I would include the library source code as a submodule of the crate and lock the commit to the version I wanted to call from Rust. If the build script wrapper crate is a subdirectory of the FlatBuffers repository itself we will only be able to support versions of flatc after the initial release of the build script wrapper crate because we won't be able to checkout commits before the build script wrapper crate existed. This might also complicate matters if we wanted to add commits to the build script wrapper but keep using a locked version of the schema compiler source code.

We could of course put the crate in a separate repository, but from the looks of things everything is currently in the one repository.

---

@rw Do you have any additional comments? I'm happy to draft up an initial implementation based on my discussion with Wouter unless you have anything to add.

[rw]

@rjsberry No additional comments, I appreciate the thought you're putting into this. It sounds like a good idea!

[aardappel]

If you can tie a crate to a git commit, that would be more robust than versions.

[nevi-me]

prost (https://github.com/danburkert/prost/tree/master/prost-build/third-party/protobuf) bundles a version of protoc, so maybe that's something that could work instead of trying to build from sources?

[aardappel]

@nevi-me I'd be fine with that too (if it works for Rust), but then you need some off-line process to build the binaries for all those platforms?

We should really have flatc builds available as product of our CI, but I don't think we do that yet.

[rjsberry]

There have been a few issues (nothing insurmmountable) with compiling our own copies of flatc for Windows, just need to resolve the CMake build on CI.

I wouldn't be against bundling versions of flatc along with the crate. If we went with this approach how would we select what version of flatc to bundle? Would we give any ability for a user to pick a version?

[nevi-me]

Hi @rjsberry, I think the latest stable flatc at the time is fine. It's meant for users who might not have it already in their path; so the version that the users has will always precede the bundled one.

How's that?

[aardappel]

The version of flatc should match commit-wise with the Rust library files. Changes to a language's implementation often involve both code generation and libraries, sometimes in interdependent ways. If you use an older flatc it is possible to run into compile errors.

[rjsberry]

The final blocker on the current implementation (compiling flatc from source) seems to be the cmake crate and compatibility with Visual Studio 2010. Shipping the flatc executable with the crate would work as a work-around but it isn't addressing the root problem that if we want to offer versions of flatc tied to specific commits we'll have to compile from source.

Of course, if we want to limit the scope of the crate to specific versions of flatc, distributing the exectuable could work.

[jean-airoldie]

@rjsberry I would be interested in working on this. Is this commit your latest?

[rjsberry]

@jean-airoldie Sure is.

The remaining work is simply CI passing on the older VS versions pending further review from @rw. This is an issue with the cmake crate: see the issue I posted here rust-lang/cmake-rs#75. It's a relatively simple Windows only issue but unfortunately I don't have my own Windows machine to test changes nor the time (at the moment) to set it all up in a virtual environment!

This is a non-issue if we ship pre-compiled binaries.

[jean-airoldie]

I see. I don't have a windows machine either so tweaking a fork of the cmake repo until the CI builds pass would be quite painful. I might try nonetheless.

And I think building from source would be much simpler in terms a maintainability.

[stale]

This issue has been automatically marked as stale because it has not had activity for 1 year. It will be automatically closed if no further activity occurs. To keep it open, simply post a new comment. Maintainers will re-open on new activity. Thank you for your contributions.

[rw]

un-stale

[vadixidav]

Is the main blocker for this still just getting the CMake build for flatc working on Windows? I assume the code probably also needs some serious updating since the branch is 532 commits behind master as well. I might be able to take a look, as this is something that would be beneficial to me. There also seems to be two other public git repositories where they ended up doing this independently as it seems it is commonly desired:

• https://github.com/frol/flatc-rust
• https://github.com/chippers/flatc.rs

Perhaps if these crates have it building on Windows (although Windows seems to have an issue for at least one) we can bring it in. If there is significant work needed to get things building on Windows, I am not sure I want to mess with it, but if there is just a little more work to take this to completion then I can give it a go. Any thoughts?

[aardappel]

What is the problem on Windows, exactly? flatc has always built for Windows with CMake, so this must be something Rust specific?

[vadixidav]

Seems it is this issue (mentioned above): rust-lang/cmake-rs#75

So far it has remained stagnant. I can try and build the flatbuffers branch and see what happens.

Edit: It seems the primary issue is that nobody has a Windows machine to test on and CI isn't working for this.

[vadixidav]

@aardappel So I haven't tested on Windows yet, but I set up a draft PR here to track my progress: #6453

If I can find some time I will take a look later today at any improvements I can make on top of this. It works for me on Linux (Ubuntu 20.10), but no Windows testing yet. Let me know if this seems good to you. I just took the branch from @rjsberry, rebased it, and then made several changes (and made a few fixes). I added myself as a co-author.

Also, I will note that flatc is a taken crate name: https://crates.io/crates/flatc. Should I reach out and ask if we can take over the crate? Also, if I do that, who needs ownership? I assume its the same people as own here: https://crates.io/crates/flatbuffers.

[aardappel]

@CasperN can probably review the PR.

Yes, we should ask to take that over, and it should have the same authors/owners. Also is there a way to lock the versions between these two crates? People should never mix versions.

[vadixidav]

@aardappel I believe we can achieve version locking by having some piece of generated code that calls a macro from flatbuffers that might look like this:

mod version_check { flatbuffers::version_check!("1.2.3"); }

By wrapping it in a module, we could make sure including the file cant allow one to accidentally have, say, a flatbuffers module in scope (by including the file). It would be utilizing a crate (although someone could swap the crate out and that could lie about the version).

In this way the macro would fail at build time if the generated version didnt match the library version. Does this work?

[aardappel]

Yup we have such checks in other languages already (C++, Java, ..). The problem is of course is that they need numbered versions which are infrequent. Would be much better if it could be be "1.2.3.$COMMIT_HASH" or whatever. But yeah, better than nothing.

Up to @CasperN

[vadixidav]

I had to leave this as it started to intersect with my day job and legal is going to take months to get back to me to give me an okay. I closed my PR, but I am leaving my branch up. Anyone that wants to pick this back up feel free to start there and check out the review and above comments.

[github-actions]

This issue is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days.

[vadixidav]

Still relevant. I haven't been keeping up with this, but I assume this issue is not resolved?

[CasperN]

Nope, it is not yet resolved. Although, @frol has made https://github.com/frol/flatc-rust which could be of use. Though, I have not tried it.

[metasim]

I'm interested in this feature as well.