Add FLATCC_BUILD_INTO_SOURCE_DIR cmake option

[dbort]

FLATCC_BUILD_INTO_SOURCE_DIR controls the built destinations of libraries and executables.

Some flatcc users need both local and cross-compiled versions of the binaries, so it's helpful to build them into the cmake binary directory instead of into the source tree.

And when they're built into a directory with Release or Debug path components, the _d suffixes are not necessary.

[dbort]

Hello! I'm happy to make changes here: the name of the option, adding a separate option for the _d part, splitting out a separate PR for the _d part.

This helps avoid a race condition in our build, where we need both host and cross-compiled target builds of flatcc. Here's the patch for how I'll be able to simplify our build once this PR goes in: https://github.com/pytorch/executorch/commit/db31f4961e8a576af93af34dbbc43965ee1edec9. And simplification aside, it avoids a currently unavoidable race condition in some of our environments, depending on when the host and target binaries are built.

Some executorch PRs related to this: https://github.com/pytorch/executorch/pull/4312, https://github.com/pytorch/executorch/pull/4541

[mikkelfj]

This looks reasonable. I don't have the full overview of CMake but I can see this being useful.

It is incredibly helpful to just have $FLATCC/bin and $FLATCC/lib, and the _d suffix for debugging, but it is not a universal solution.

Another option is to copy files after building so we don't need the discrepancy between build types, at least for non-cross builds.

It seems that you have done the right thing, but please make VERY sure it will not break default behaviour.

If you think consolidation to earlier location makes more sense, per you comments, please go ahead, as long as it does not affect the intended behaviour.

[mikkelfj]

We need some update to README as well, once we settle on the solution.

[dbort]

Thank you for taking a look, @mikkelfj!

Another option is to copy files after building so we don't need the discrepancy between build types, at least for non-cross builds.

This didn't work for us because when building in parallel, both the host and cross-compiled targets will try to build to the same location under the source tree, causing a race condition. Depending on when the copy happened, we would randomly get the host binary or the cross-compiled binary.

please make VERY sure it will not break default behaviour.

(For the record, I ran the following tests on an M1 macbook using AppleClang 16.0.0.16000026, cmake version 3.29.5, ninja version 1.11.1.git.kitware.jobserver-1. The master branch was at https://github.com/dvidelabs/flatcc/commit/be12a1f74bbbda9bbad749f5fbf56c342c2878cb)

I wrote a test script at https://gist.github.com/dbort/5cc082098cfe681402c5d0ac71403615 to build with scripts/build.sh with and without this PR. https://gist.github.com/dbort/b13f59ae9b87d472c05b08369e9c3554 shows the output: the same set of files are created under the lib and bin directories.

flatcc-test:INFO: Manifests are identical:
-----BEGIN-----
bin/flatcc
bin/flatcc_d
lib/libflatcc.a
lib/libflatcc_d.a
lib/libflatccrt.a
lib/libflatccrt_d.a
-----END-----
PASS

I also tested the behavior with this new option disabled:

echo "FLATCC_BUILD_FLAGS=-DFLATCC_BUILD_INTO_SOURCE_DIR=OFF" > scripts/build.cfg.pr306
./scripts/initbuild.sh pr306
./scripts/build.sh
ls bin lib build/{Debug,Release}/{bin,lib}

This shows that the built binaries and libraries are not in the source tree, and are under the appropriate build config directories:

bin:

build/Debug/bin:
flatcc

build/Debug/lib:
libflatcc.a	libflatccrt.a

build/Release/bin:
flatcc

build/Release/lib:
libflatcc.a	libflatccrt.a

lib:

I'm happy to add more tests to these scripts, if you have other files or CMake variables that you'd like me to compare, or if you'd like me to test different build configurations.

[dbort]

Also, what parts of the README do you suggest that I should update?

[dbort]

Hi @mikkelfj, could you please take a look at my testing if you have a chance? We're running into more build issues related to binaries-in-tree and "_d" suffixes, and it would help us a lot if we were able to merge this change, if you're ok with it.

[ykhrustalev]

I am using ninja multi config setup and I have used

        "FLATCC_INSTALL": "ON",
        "FLATCC_INSTALL_LIB": "${sourceDir}/build/${presetName}/vendor/executorch/lib"

to specify the destination, FLATCC_INSTALL_LIB.

Sharing with you as I see your change does manual steps instead.

[dbort]

@ykhrustalev thanks for pointing that out! FLATCC_INSTALL_LIB looks like it would let us separate host and target libraries. I'll see if it helps us fix our current breakage.

I'd still like to merge this PR if possible so that we can build flatcc using the same conventions that most cmake projects use. FLATCC_INSTALL_LIB is relative to the repo root, so we'd need to use ../ components to put the binaries in the project-level cmake output tree. (We use flatcc as a submodule, so our build artifacts typically live in a higher-level build directory.) FLATCC_INSTALL_LIB also doesn't let us change the bin/ outputs, and it still adds the _d suffixes for debug mode.

[mikkelfj]

I have just bumped CMake to v 3.16 and am ready to reconsider this.

My main object is the name FLATCC_BUILD_INTO_SOURCE_DIR It suggests that all intermediate products are dumped into the source tree, which is not the case.

Maybe FLATCC_DIST_INTO_SOURCE_DIR?

[chenweng-quic]

Hi, I am facing the same issue that cross-compiled will conflict with different target, may I know when can we have this PR enable?

[chenweng-quic]

Hi folks, may I know if there’s any update on this PR? Thanks!

[mikkelfj]

we can figure something out soon. Any comments to the concerns I raised in earlier comment on naming? And also how it might relate to #358 ?