Unexpected --keep-prefix behavior

[calebzulawski]

After #7348, --keep-prefix has unexpected behavior.

Consider the following directory structure:

• foo/foo.fbs, which contains include 'bar/bar.fbs';
• foo/bar/bar.fbs

Run:

flatc --cpp --keep-prefix foo/foo.fbs

I'd expect the resulting foo_generated.h to contain #include "bar/bar_generated.h", however with this change it results in #include "foo/bar/bar_generated.h". While it may be useful to add the foo/ prefix, I believe this should be done with --include-prefix foo, since --keep-prefix should only add the prefix contained in the schema.

This is a change from prior versions of flatc, and breaks my out-of-source build system which does not put the generated files in the original foo directory.

[kenroser]

Since I believe this is regression, is there any chance this can be addressed quickly? https://github.com/google/flatbuffers/pull/7348

[kenroser]

@dbaileychess I would appreciate it if you would address this regression.

[dbaileychess]

@kenroser Sorry for the delay, I was out on paternity leave. I put a PR up with a fix.

[calebzulawski]

Thanks!

[kenroser]

@kenroser Sorry for the delay, I was out on paternity leave. I put a PR up with a fix.

Thanks and congratulations on having a child.

[alexames]

I think this fix is incomplete. Before #7348 flatc was able to handle full paths, but now it's getting confused. Here's a minimal example:

foo/Bar.fbs

include "Baz.fbs";

Baz.fbs

struct dummy { i: int32; }

In the header I would expect to have this #include statement:

#include "generated/Baz_generated.h"

And that's what you get prior to #7348, or after #7348 when you compile foo/Bar.fbs with relative paths like so:

flatc.exe -o build/generated `
          -I . `
          -c foo/Bar.fbs `
          --include-prefix generated `
          --keep-prefix

But if you use the full absolute paths, it breaks:

flatc.exe -o C:/Users/Alexa/Programming/fbexample/build/generated `
          -I C:/Users/Alexa/Programming/fbexample `
          -c C:/Users/Alexa/Programming/fbexample/foo/Bar.fbs `
          --include-prefix generated `
          --keep-prefix

The resulting Bar_generated.h will have this broken include in it:

#include "generated/C:/Users/Alexa/Programming/fbexample/Baz_generated.h"

CMake likes to use full paths in most contexts, so that's what I use to generate my build rules, which is how I encountered this.

[dbaileychess]

Thanks, sorry the toil. We don't have testing in this area, so it is hard to know what breaks.

Thanks for the example, I will attempt a fix.

[dbaileychess]

@alexames One question, for your last example where you have --include-prefix generated with absolute paths, what do you expect the output to be?

[alexames]

This is my understanding of the old behavior:

Suppose you have something like the following, where you run flatc from the directory schemas, with the output directory being project/build (in other words, -o ../build/)

📁 C:
└ 📁 path
  └ 📁 to
    └ 📁 project
      └ 📁 schemas (working directory)
        ├ 📁 effects
        │ ├ 🗎 particle.fbs
        │ └ 🗎 emitter.fbs
        ├ 📁 audio         
        │ ├ 🗎 source.fbs
        │ └ 🗎 listener.fbs
        │ 🗎 entity.fbs 
        └ 🗎 scene.fbs

The #include statements should have the form:

#include "<--include-prefix>/<Prefix>/<Filename>_generated.h"

Where:

C:\path\to\project\schemas\effects\particle.fbs
└─┬──────────────────────┘ └┬────┘ └─┬────┘
  └ Working directory       └ Prefix └ Filename

flatc.exe -o C:\path\to\project\build\generated `
          -I C:\path\to\project\schemas `
          -c C:\path\to\project\schemas\effects\particle.fbs `
          --include-prefix generated `
          --keep-prefix 

In this case, we would expect #include "generated/effects/particle_generated.h"

A little bit annoyingly, the --keep-prefix flag doesn't preserve the directory structure when writing the file, just when generating the #includes, so I still need to have cmake output to the correct directory, but I have some cmake tooling that takes care of that (which I want to upstream once this is fixed)

[dbaileychess]

Thanks for the detail response, I am looking into it now.

[dbaileychess]

I just ran a very similar query on my Mac and Windows command and didn't have the issue you saw.

C:\Users\Derek\Desktop\flatbuffers\tests\flatc> 
../../flatc.exe -o C:\Users\Derek\Desktop -I C:\Users\Derek\Desktop\flatbuffers\tests\flatc -c C:\Users\Derek\Desktop\flatbuffers\tests\flatc\bar\bar.fbs --include-prefix generated --keep-prefix

And the generate file was in the right location and the include statement was:

#include "generated/baz/baz_generated.h"

[dbaileychess]

@alexames Any update to this?

[alexames]

When I can find a few hours of spare time I'm going to try to recreate a minimal test case on both Windows and Mac and see if their behavior diverges somehow, or if I can cause the error to occur on a Mac as well. I'll let you know what I find out when I do.

[dbaileychess]

See https://github.com/google/flatbuffers/pull/7405 I just added a cross platform way to test this (or to add tests too). Should be pretty quick.

[alexames]

I tried pulling out my (very, very old) Macbook, but couldn't get it into a state where I could get it compiling. So instead I opened up ubuntu and tried it there. I can confirm the issue exists on both Windows and Linux (and so I'm betting it will happen on OSX too if you supply the same arguments). I have a full minimal reproduction case here, complete with flatbuffer schemas to run and how to run the compiler:

https://github.com/alexames/issue7354/blob/main/README.md

I also tried reverting back to before #7348 and the correct output is produced.

[dbaileychess]

@alexames Thanks for the detail steps, I will be taking a look.

[dbaileychess]

So your foo/Bar.fbs has an include to a relative parent file in ./Baz.fbs but then you expected include statement is just:

#include "generated/Baz_generated.h"

instead of

#include "generated/../Baz_generated.h"

So what happened to the ./foo/bar -> ./baz relationship? That ../ prefix should just be disregarded?

[dbaileychess]

@alexames Thanks for the report and repo. I made #7456 that fixes, at least, the example you shown in your previous comment. Would you like to test this out?

[alexames]

So what happened to the ./foo/bar -> ./baz relationship? That ../ prefix should just be disregarded?

My understanding of the previous behavior is that the reason there's no ../ in the resulting include is because the path to the included files is determined based on the working directory that the flatc executable was run from. So it's not the relationship between the ./foo/Bar.fbs and ./Baz.fbs that matters, but rather the relationship between . (i.e. the current directory) ./Baz.fbs

@alexames Thanks for the report and repo. I made https://github.com/google/flatbuffers/pull/7456 that fixes, at least, the example you shown in your previous comment. Would you like to test this out?

I don't have time to do it today, but I can try it out this weekend for sure.

[dbaileychess]

Please reopen if not fixed.