-
Notifications
You must be signed in to change notification settings - Fork 536
Remove usage of external flatc in builds and scripts #9306
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9306
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 3ec8bd8 with merge base 4c54bab ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
8c9b1c8
to
609c745
Compare
31830f5
to
2a5d7d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheked/reviewed the Arm files, thanks for fixing this. I hope we can use the same solution for flatcc down the line if this work out good. Great work getting this annoyance fixed.
2a5d7d7
to
ac6b9fd
Compare
@jathu are the CI failures related? |
8acf205
to
aa73a64
Compare
4943be0
to
4e3f774
Compare
bfb21f9
to
e2c853d
Compare
e2c853d
to
a40e1e4
Compare
a40e1e4
to
3ec8bd8
Compare
@mergennachin should be good now. The two failing arm tests are failing on main |
Has not even run at all (or logs are missing?) there seem to be some major flakeness just running the arm test e.g. the CI seem to not start them as expected :( The other job run but had a strange error, I retriggered both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we delete EXECUTORCH_BUILD_HOST_TARGETS option altogether from the repo?
There are still some remaining references un CmakeLists.txt and Utils.cmake
@mergennachin yes, #9404 |
### Summary * Remove the dependence of `EXECUTORCH_BUILD_HOST_TARGETS` since we always build flatc for the host now * Turn on `EXECUTORCH_BUILD_FLATC` if `FLATC_EXECUTABLE` is not set * Build flatc from source for Apple builds ### Test Plan Now macOS/Linux targeting macOS/Linux/iOS/Android should all build flatc from source: ``` $ rm -rf pip-out && ./install_executorch.sh $ rm -rf cmake-out* && ./build/build_apple_frameworks.sh $ rm -rf cmake-out* && ./build/build_android_library.sh ``` cc @larryliu0820 @lucylq
### Summary * Remove the dependence of `EXECUTORCH_BUILD_HOST_TARGETS` since we always build flatc for the host now * Turn on `EXECUTORCH_BUILD_FLATC` if `FLATC_EXECUTABLE` is not set * Build flatc from source for Apple builds ### Test Plan Now macOS/Linux targeting macOS/Linux/iOS/Android should all build flatc from source: ``` $ rm -rf pip-out && ./install_executorch.sh $ rm -rf cmake-out* && ./build/build_apple_frameworks.sh $ rm -rf cmake-out* && ./build/build_android_library.sh ``` cc @larryliu0820 @lucylq
### Summary Removes EXECUTORCH_BUILD_HOST_TARGETS per @mergennachin [comment](#9306 (review)). Fixes #9404 ### Test plan Ensure executorch and cadence builds successfully: ``` ./install_executorch.sh mkdir cmake-out cmake -DCMAKE_TOOLCHAIN_FILE=backends/cadence/cadence.cmake \ -DCMAKE_INSTALL_PREFIX=cmake-out \ -DCMAKE_BUILD_TYPE=Debug \ -DPYTHON_EXECUTABLE=python3 \ -DEXECUTORCH_BUILD_EXTENSION_RUNNER_UTIL=ON \ -DEXECUTORCH_BUILD_HOST_TARGETS=ON \ -DEXECUTORCH_BUILD_EXECUTOR_RUNNER=OFF \ -DEXECUTORCH_BUILD_PTHREADPOOL=OFF \ -DEXECUTORCH_BUILD_CPUINFO=OFF \ -Bcmake-out . cmake --build cmake-out -j8 --target install --config Debug cmake -DCMAKE_BUILD_TYPE=Debug \ -DCMAKE_TOOLCHAIN_FILE=examples/backends/cadence.cmake \ -DCMAKE_PREFIX_PATH=cmake-out \ -DMODEL_PATH=add.pte \ -DNXP_SDK_ROOT_DIR=/home/user_name/nxp-sdk \ -DNN_LIB_BASE_DIR=/home/user_name/nnlib-hifi4 \ -Bcmake-out/examples/cadence \ examples/cadence cmake --build cmake-out/examples/cadence -j8 -t cadence_executorch_example ls cmake-xt/*.bin ``` * Test plan assumes nxp-sdk and nnlib-hifir are in user's home directory and Xtensa compiler is available (env vars set). See [cadence docs](https://github.com/pytorch/executorch/blob/main/docs/source/backends-cadence.md) * Since these are modifications to the build system, I leave it to discretion if CI builds provide enough risk protection to merge. * Happy to write tests if needed - I'll need a little direction here if needed. #9306 ran `./install_executorch.sh`, `build_apple_frameworks.sh` and `build_android_library.sh`. --------- Co-authored-by: Mergen Nachin <[email protected]>
Summary
EXECUTORCH_BUILD_HOST_TARGETS
since we always build flatc for the host nowEXECUTORCH_BUILD_FLATC
ifFLATC_EXECUTABLE
is not setTest Plan
Now macOS/Linux targeting macOS/Linux/iOS/Android should all build flatc from source:
cc @larryliu0820 @lucylq