Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rust] Not importing all files #6800

Closed
krojew opened this issue Aug 19, 2021 · 12 comments
Closed

[Rust] Not importing all files #6800

krojew opened this issue Aug 19, 2021 · 12 comments
Labels

Comments

@krojew
Copy link
Contributor

krojew commented Aug 19, 2021

When building multiple files from a given directory (let's call it a) which import files from another directory b, there is a mod.rs file produced which only imports mods from the last file in a and last file in b, yet all _generated.rs files are produced.

@CasperN
Copy link
Collaborator

CasperN commented Aug 19, 2021

Can you give more details to help reproduce this? Were there multiple invocations of flatc --rust? There should only be one invocation so flatc will know about all the _generated.rs and generate mod.rs correctly.

@krojew
Copy link
Contributor Author

krojew commented Aug 19, 2021

In the example flatc was run once in a with *.fbs.

@CasperN
Copy link
Collaborator

CasperN commented Aug 23, 2021

Ok, I was able to reproduce this. My current implementation assumes that Parser contains the full type system by the end of the invocation of flatc. This is wrong because the Parser is reset between different .fbs files.

Workaround: Write a new .fbs file that imports all your other .fbs files and invoke flatc with that.

I'll have to think about how to get around this reset behavior without breaking something else. We, unfortunately, rely on some pretty non-trivial mutable state in Parser.

@krojew
Copy link
Contributor Author

krojew commented Nov 8, 2021

Any news on this front?

@CasperN
Copy link
Collaborator

CasperN commented Nov 8, 2021

Unfortunately, I am not working on this and don't really have time to for the foreseeable future. :(

@TheButlah
Copy link

TheButlah commented Apr 2, 2022

I believe I just ran into this. I've tried so many different incantations of flatc that its not even funny. This one got me the closest, with all imports between the various files correct, and its kinda nice that each type seems to be put in its own file (even though thats not how the fbs files were written...). However, the toplevel mod.rs only generates a pub use statement for the last type in the last file passed to flatc.

flatc --rust --rust-module-root-file --gen-all -o protocol/rust/src/generated -I ./protocol/flatbuffers ./protocol/flatbuffers/server.fbs ./protocol/flatbuffers/firmware.fbs ./protocol/flatbuffers/commons/datatypes.fbs ./protocol/flatbuffers/commons/hardware_info.fbs ./protocol/flatbuffers/commons/misc.fbs

The repository/commit in question is here: https://github.com/SlimeVR/slimevr_protocol/tree/6c96aabcf17c095ac648583f674a45681533190a

I will try the approach of having a .fbs file that imports all the others. Although I'd really appreciate it if this sort of stuff could be solved! It seems firmly in the expected use of flatbuffers.

@CasperN
Copy link
Collaborator

CasperN commented Apr 2, 2022

Yeah, sorry. There's a lot of legacy cruft implementation in there and the original variations made problematic assumptions. I need to deprecate the foot guns

and its kinda nice that each type seems to be put in its own file (even though thats not how the fbs files were written...).
Although I'd really appreciate it if this sort of stuff could be solved! It seems firmly in the expected use of flatbuffers.

This is required to correctly map flatbuffer's c++ style namespaces onto Rust modules... it's a long story. The old thing was broken too - it basically didn't support multiple .fbs files.


My current implementation assumes that Parser contains the full type system by the end of the invocation of flatc. This is wrong because the Parser is reset between different .fbs files.

Now that I'm revisiting this, I think I can just delete this line

if (symbol.generated) continue;

which will ignore the fact that Parser thinks the type is generated. This will remove the need for the magic "import all the rest" file.

nvm that's wrong

@github-actions
Copy link

github-actions bot commented Mar 4, 2023

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

@github-actions github-actions bot added the stale label Mar 4, 2023
@github-actions
Copy link

This issue was automatically closed due to no activity for 6 months plus the 14 day notice period.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2023
@vtstanescu
Copy link

Is this going to be addressed? My team find themselves in the position to manually write the mod.rs file.

@CasperN
Copy link
Collaborator

CasperN commented Jun 7, 2023

Does the "Only generate one .fbs that includes all your other .fbs files" solution not work for your team?

Flatbuffers is a volunteer-only project, I don't have the time in my life to provide maintenance anymore, and I don't know who else may be interested, so unfortunately, I recommend contributing a fix, or at least to documentation for workarounds.

Though there are a lot of considerations that influence what the right solution is, and I'm not sure how it all shakes out


A potential solution could be to generate a module namespace hierarchy file for each .fbs file, with appropriate imports/reexports. This will prevent library users from stomping on the mod.rs file but requires us to know which .fbs file every symbol comes from and what symbols are visible in each .fbs file including all their imports. This would be crazy complex to do with the existing code.

// file1.fbs
include "file2.fbs";
namespace a;
table Foo { bar: b.Bar; }
// my_dir/file2.fbs
namespace a.b;
table Bar { }

will generate

// file1.rs

pub mod a {
  generate_foo_here!();
  // Note that using `my_dir::file2::*` at root of this file will define `a` twice.
  // Therefore, we have to be specific with what we `use`.
  pub mod b {
    pub use my_dir::file2::a::b::Bar;
  }
}
// my_dir/file2.rs
pub mod a {
  pub mod b {
    generate_bar_here!();
  }
}

A usability downside of this approach is that all the symbols are locked behind the file1 and file2 modules rather than being global. file1::a::b::Bar and my_dir::file2::a::b::Bar are the same, but you can't do the following

use file1::*;
use my_dir::file2::*;
fn make_bar() -> a::b::Bar<'static> {
  todo!()
}

Rust will error and say a is ambiguous.

Also, what if you don't want to generate your schema files in the same crate? Then the import/reexport declarations need to be adjusted accordingly - should that be allowed, and if so, how would it be configured?

There are likely more complications to this approach that aren't thought about. I don't know what other solutions people are using either, or their implicit requirements...

Footnotes

  1. Another complication: Bazel needs to know, from the .fbs filenames, what output files will be generated, which means generating a file per flatbuffer symbol will make Bazel sad. I think there are workarounds within Bazel itself, including tarballs and "tree artifacts", but I haven't verified those solutions. Generating one .rs per .fbs file is compatible with Bazel, which is nice.

@vtstanescu
Copy link

Hi, @CasperN!

Sorry, I just now realize my reply might denote some degree of attitude.

Thank you for the through explanation, this helps us in understanding the issue at hand and also the possible workaround, as I missed your previous comment with the workaround.

Initially I got confused by the flat compiler, thinking if it goes the way of generating a mod.rs, why doesn't it do it properly and kind of expected to either not generate it and leave that to the user, or do it properly.

Understanding the issue in-depth now, we can take an action and know why we need to do it that way.
We are going to either use suggested workaround of building one .fbs that includes all the others or handle the mod.rs ourselves however we see fit for our use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants