flattests fails if out-of-source build directory is used

[Tatsh]

This is on Linux x86-64, 2.0.6 release. 2.0.0 does not have this issue.

cd flatbuffers
mkdir build
cmake -G Ninja ..
ninja
ninja test

Output:

0/1] Running tests...
Test project /home/tatsh/dev/flatbuffers/build
    Start 1: flattests
1/1 Test #1: flattests ........................Subprocess aborted***Exception:   0.05 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) =   0.05 sec

The following tests FAILED:
          1 - flattests (Subprocess aborted)
Errors while running CTest
Output from these tests are in: /home/tatsh/dev/flatbuffers/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
FAILED: CMakeFiles/test.util
cd /home/tatsh/dev/flatbuffers/build && /usr/bin/ctest --force-new-ctest-process
ninja: build stopped: subcommand failed.

The paths that are set in various fields such as declaration_file are relative, and the tests are not expecting this.

EXPECTED: "//../../tests/monster_test.fbs"
VALUE: "//monster_test.fbs"
TEST FAILED: /home/tatsh/dev/flatbuffers/tests/test.cpp:932, 'root_table->declaration_file()->c_str()' != '"//monster_test.fbs"' in
flattests: /home/tatsh/dev/flatbuffers/tests/test_assert.cpp:24: void TestFail(const char*, const char*, const char*, const char*, int, const char*): Assertion `0' failed.
Aborted (core dumped)

https://github.com/google/flatbuffers/blob/master/tests/test.cpp#L952=

https://bugs.gentoo.org/842060

[dbaileychess]

Yeah, our out-of-source build setup is not good at the moment. I would welcome any PRs to fix this.

[musicinmybrain]

The error message is confusing because the positions of expected and actual value parameters to TEST_EQ_STR() are reversed. This seems to be the case for a lot of the assertions in tests/test_assert.h.

[musicinmybrain]

For the Fedora Linux flatbuffers package, the following allows tests to succeed with an out-of-source build on release 2.0.6:

# flattests fails if out-of-source build directory is used
# https://github.com/google/flatbuffers/issues/7282
#
# This downstream-only patch is not general; it makes assumptions about the
# out-of-source build path and would break in-source builds.
sed -r -i 's@//((include|monster)_test)@//../../tests/\1@g' tests/test.cpp

The result looks like:

From 064f64a42e081ac9d1b4c6e16af92ed08b660d1e Mon Sep 17 00:00:00 2001
From: "Benjamin A. Beasley" <[email protected]>
Date: Wed, 22 Jun 2022 08:31:59 -0400
Subject: [PATCH] Downstream-only patch for out-of-source builds

---
 tests/test.cpp | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/test.cpp b/tests/test.cpp
index 530bcbc8..42b2b84c 100644
--- a/tests/test.cpp
+++ b/tests/test.cpp
@@ -929,51 +929,51 @@ void ReflectionTest(uint8_t *flatbuf, size_t length) {
 
   // Check the declaration files.
   TEST_EQ_STR(root_table->name()->c_str(), "MyGame.Example.Monster");
-  TEST_EQ_STR(root_table->declaration_file()->c_str(), "//monster_test.fbs");
+  TEST_EQ_STR(root_table->declaration_file()->c_str(), "//../../tests/monster_test.fbs");
   TEST_EQ_STR(
       schema.objects()->LookupByKey("TableA")->declaration_file()->c_str(),
-      "//include_test/include_test1.fbs");
+      "//../../tests/include_test/include_test1.fbs");
   TEST_EQ_STR(schema.objects()
                   ->LookupByKey("MyGame.OtherNameSpace.Unused")
                   ->declaration_file()
                   ->c_str(),
-              "//include_test/sub/include_test2.fbs");
+              "//../../tests/include_test/sub/include_test2.fbs");
   TEST_EQ_STR(schema.enums()
                   ->LookupByKey("MyGame.OtherNameSpace.FromInclude")
                   ->declaration_file()
                   ->c_str(),
-              "//include_test/sub/include_test2.fbs");
+              "//../../tests/include_test/sub/include_test2.fbs");
 
   // Check scheam filenames and their includes.
   TEST_EQ(schema.fbs_files()->size(), 3);
 
   const auto fbs0 = schema.fbs_files()->Get(0);
-  TEST_EQ_STR(fbs0->filename()->c_str(), "//include_test/include_test1.fbs");
+  TEST_EQ_STR(fbs0->filename()->c_str(), "//../../tests/include_test/include_test1.fbs");
   const auto fbs0_includes = fbs0->included_filenames();
   TEST_EQ(fbs0_includes->size(), 2);
 
   // TODO(caspern): Should we force or disallow inclusion of self?
   TEST_EQ_STR(fbs0_includes->Get(0)->c_str(),
-              "//include_test/include_test1.fbs");
+              "//../../tests/include_test/include_test1.fbs");
   TEST_EQ_STR(fbs0_includes->Get(1)->c_str(),
-              "//include_test/sub/include_test2.fbs");
+              "//../../tests/include_test/sub/include_test2.fbs");
 
   const auto fbs1 = schema.fbs_files()->Get(1);
   TEST_EQ_STR(fbs1->filename()->c_str(),
-              "//include_test/sub/include_test2.fbs");
+              "//../../tests/include_test/sub/include_test2.fbs");
   const auto fbs1_includes = fbs1->included_filenames();
   TEST_EQ(fbs1_includes->size(), 2);
   TEST_EQ_STR(fbs1_includes->Get(0)->c_str(),
-              "//include_test/include_test1.fbs");
+              "//../../tests/include_test/include_test1.fbs");
   TEST_EQ_STR(fbs1_includes->Get(1)->c_str(),
-              "//include_test/sub/include_test2.fbs");
+              "//../../tests/include_test/sub/include_test2.fbs");
 
   const auto fbs2 = schema.fbs_files()->Get(2);
-  TEST_EQ_STR(fbs2->filename()->c_str(), "//monster_test.fbs");
+  TEST_EQ_STR(fbs2->filename()->c_str(), "//../../tests/monster_test.fbs");
   const auto fbs2_includes = fbs2->included_filenames();
   TEST_EQ(fbs2_includes->size(), 1);
   TEST_EQ_STR(fbs2_includes->Get(0)->c_str(),
-              "//include_test/include_test1.fbs");
+              "//../../tests/include_test/include_test1.fbs");
 
   // Check Root table fields
   auto fields = root_table->fields();
-- 
2.36.1

I’m not sure what the best way to make this work in the general case would be. You could perhaps use add_compile_definitions to pass in the values of CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_BINARY_DIR as preprocessor macros, but you would still need to construct the relative path, either in CMake or in C++. Or, perhaps it would be sufficient for the tests to evaluate a predicate function that looks for the // prefix and the expected suffix, like include_test/include_test1.fbs, without validating anything in between.

[CasperN]

I think the best fix would be to use for cmake to invoke the bfbs-filenames flag

https://github.com/google/flatbuffers/blob/83a43fc7977b67259c6a78147cf1c09fe6af85f7/src/flatc.cpp#L163

This controls what schema filenames are relative to

[musicinmybrain]

I think the best fix would be to use for cmake to invoke the bfbs-filenames flag

https://github.com/google/flatbuffers/blob/83a43fc7977b67259c6a78147cf1c09fe6af85f7/src/flatc.cpp#L163

This controls what schema filenames are relative to

Interesting! Looking at scripts/generate_code.py, it seems like this approach is already used in a few places, but I haven’t—so far—figured out where else it should be added.

[musicinmybrain]

This seems to be fixed in 22.11.23. I haven’t investigated to see what was changed. Does 22.11.23 work for you too, @Tatsh? If so, I guess this can be closed…

[Tatsh]

Works fine now.