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

Add FLATCC_BUILD_INTO_SOURCE_DIR cmake option #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dbort
Copy link

@dbort dbort commented Oct 23, 2024

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.

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
Copy link
Author

dbort commented Oct 23, 2024

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: pytorch/executorch@db31f49. 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: pytorch/executorch#4312, pytorch/executorch#4541


if (CMAKE_BUILD_TYPE MATCHES "Debug")
set(CMAKE_EXECUTABLE_SUFFIX "_d${CMAKE_EXECUTABLE_SUFFIX}")
if (FLATCC_BUILD_INTO_SOURCE_DIR)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also let me know if you'd rather that I folded this into the if (FLATCC_BUILD_INTO_SOURCE_DIR) on line 171 to consolidate the logic.

@mikkelfj
Copy link
Contributor

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
Copy link
Contributor

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

@dbort
Copy link
Author

dbort commented Nov 2, 2024

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 be12a1f)

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
Copy link
Author

dbort commented Nov 2, 2024

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

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

Successfully merging this pull request may close these issues.

2 participants