Skip to content

Build flatc for the host instead of the target platform #9077

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

Merged
merged 6 commits into from
Mar 13, 2025

Conversation

jathu
Copy link
Contributor

@jathu jathu commented Mar 10, 2025

Summary

  • Fixes Flatc should be built for the host rather than the target when cross-compiling #7260
    • We use flatc as part of the build step to generate some files from FlatBuffer definitions. This implies the tool runs on the host. However, we currently build flatc as part of the main build — which propagates the target CXX flags to flatc. Thus, flatc gets built targeting the target platform. To ensure flatc is built for the host, we can use include flatc as an ExternalProject. This does not propagate the CXX flags
  • Some targets implicitly depended on flatc, we now make that requirement explicit
  • We currently spread the FLATC_EXECUTABLE defaulting across the project. Let's just centralize this at the root

Test plan

$ ./install_executorch.sh

# Previously flatc was built against the Android target, now they target the host
$ ./build/build_android_library.sh
$ file /Users/jathu/executorch/cmake-out-android-arm64-v8a/third-party/flatbuffers/flatc
/Users/jathu/executorch/cmake-out-android-arm64-v8a/third-party/flatbuffers/flatc: Mach-O 64-bit executable arm64

# Apple builds work as usual, but they use flatc from pip
$ ./build/build_apple_frameworks.sh

cc @larryliu0820 @lucylq @swolchok @dbort

Copy link

pytorch-bot bot commented Mar 10, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9077

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 1 Cancelled Job, 2 Unrelated Failures

As of commit 4faa443 with merge base cb3ec19 (image):

NEW FAILURE - The following job has failed:

CANCELLED JOB - The following job was cancelled. Please retry:

FLAKY - The following job failed but was likely due to flakiness present on trunk:

BROKEN TRUNK - The following job failed but was present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 10, 2025
@jathu jathu force-pushed the jathu/use-host-flatc branch 7 times, most recently from dad16ab to 50abb35 Compare March 10, 2025 21:21
@jathu jathu added the module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch label Mar 10, 2025
@jathu jathu requested review from swolchok and larryliu0820 March 10, 2025 21:54
@jathu jathu marked this pull request as ready for review March 10, 2025 22:27
@jathu jathu force-pushed the jathu/use-host-flatc branch from 50abb35 to 2646908 Compare March 10, 2025 23:06
Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

looks great, just the one minor comment

@jathu jathu force-pushed the jathu/use-host-flatc branch from e840167 to 6030f4d Compare March 11, 2025 16:20
@jathu jathu marked this pull request as draft March 11, 2025 16:42
@jathu jathu marked this pull request as ready for review March 11, 2025 17:05
@swolchok swolchok added the release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc. label Mar 11, 2025
@swolchok swolchok requested review from JacobSzwejbka and removed request for JacobSzwejbka March 11, 2025 17:38
@jathu jathu force-pushed the jathu/use-host-flatc branch from 755573c to 6f0bff2 Compare March 11, 2025 19:35
@mergennachin
Copy link
Contributor

@jathu I added 'ciflow/trunk' label, which triggers all trunk jobs

Looks like there are a bunch of CI failures

Please fix them.

@jathu jathu force-pushed the jathu/use-host-flatc branch 3 times, most recently from 55bcf67 to 1c4bbaa Compare March 12, 2025 22:53
@jathu jathu force-pushed the jathu/use-host-flatc branch from 1c4bbaa to 3ec03d7 Compare March 13, 2025 15:43
@jathu jathu force-pushed the jathu/use-host-flatc branch from 3ec03d7 to 4faa443 Compare March 13, 2025 16:39
@jathu
Copy link
Contributor Author

jathu commented Mar 13, 2025

These failing tests are unrelated to this diff:

@jathu jathu merged commit 570e06c into main Mar 13, 2025
127 of 131 checks passed
@jathu jathu deleted the jathu/use-host-flatc branch March 13, 2025 20:29
@swolchok
Copy link
Contributor

Now seeing this:

$ cmake . \
                                                                 -DCMAKE_INSTALL_PREFIX=cmake-out \
                                                                 -DEXECUTORCH_BUILD_KERNELS_CUSTOM=ON \
                                                                 -DEXECUTORCH_BUILD_KERNELS_CUSTOM_AOT=ON \
                                                                 -DEXECUTORCH_BUILD_KERNELS_OPTIMIZED=ON \
                                                                 -DEXECUTORCH_BUILD_KERNELS_QUANTIZED=ON \
                                                                 -DEXECUTORCH_BUILD_EXTENSION_DATA_LOADER=ON \
                                                                 -DEXECUTORCH_BUILD_EXTENSION_MODULE=ON \
                                                                 -DEXECUTORCH_BUILD_EXTENSION_RUNNER_UTIL=ON \
                                                                 -DEXECUTORCH_BUILD_EXTENSION_TENSOR=ON \
                                                             -DEXECUTORCH_BUILD_EXTENSION_FLAT_TENSOR=ON \
                                                                 -DEXECUTORCH_BUILD_DEVTOOLS=ON \
                                                                 -DEXECUTORCH_BUILD_XNNPACK=ON \
                                                                 -DEXECUTORCH_BUILD_TESTS=ON \
                                                             -DCMAKE_BUILD_TYPE=Release \
                                                             -DEXECUTORCH_ENABLE_LOGGING=ON \
                                                                 -Bcmake-out -GNinja
$  cmake --build cmake-out
ninja: error: 'third-party/flatbuffers/flatc', needed by 'schema/include/executorch/schema/program_generated.h', missing and no known rule to make it

Repros on this commit and not the previous one.

@swolchok
Copy link
Contributor

Ninja is extra-picky about dependencies. #9246 fixes

swolchok added a commit that referenced this pull request Mar 13, 2025
Needed to tell CMake to tell Ninja where flatc comes from.

ghstack-source-id: d700821
ghstack-comment-id: 2722835544
Pull Request resolved: #9246
swolchok added a commit that referenced this pull request Mar 13, 2025
Needed to tell CMake to tell Ninja where flatc comes from.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flatc should be built for the host rather than the target when cross-compiling
5 participants