-
Notifications
You must be signed in to change notification settings - Fork 424
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
Fixed always rebuild issue in cmake. #7512
Conversation
The generated files were located in include/executorch/schema/program_generated.h CMake was expecting files in include/executorch/program_generated.h Presumably this was a change at some point, and the expected output from cmake never got updated.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7512
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 32d0b14 with merge base ae3d558 (): NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
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.
Thanks for making the changes and for the detailed write-up! This looks good to me.
cc @dbort
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.
Thanks for fixing this! The outputs were definitely out of sync with the custom_command.
schema/CMakeLists.txt
Outdated
) | ||
endforeach() | ||
|
||
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.
Please remove these stray trailing whitespace characters
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.
oops, removed that.
schema/CMakeLists.txt
Outdated
@@ -26,10 +26,10 @@ function(generate_program_schema _schema_srcs _schema_name) | |||
foreach(fbs_file ${_schema_srcs}) | |||
string(REGEX REPLACE "[.]fbs$" "_generated.h" generated "${fbs_file}") | |||
list(APPEND _schema_outputs | |||
"${_program_schema__include_dir}/executorch/${generated}" | |||
"${_program_schema__include_dir}/executorch/schema/${generated}" |
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.
The underlying problem was that this path didn't match the path in the custom_command. To make it more clear that they need to agree, and to avoid mismatches in the future, consider using a common local variable for both of them, like
set(_include_dir "${_program_schema__include_dir}/executorch/schema")
and then
list(APPEND _schema_outputs "${_include_dir}/${generated}")
here and again using _include_dir
on line 38.
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.
Changed to this, named it _program_schema__output_dir to be a little more clear.
Thanks for the changes, and for finding/fixing this; looks great! |
Summary
Fixes #7511
CMake generates different files than expected, thus the incremental builds always rebuild.
The generated files were located in include/executorch/schema/program_generated.h CMake was expecting files in include/executorch/program_generated.h
This causes cmake to always regenerate the files, as the expected artifacts are never there.
Presumably this was a change at some point, and the expected output from CMake never got updated.
Test plan
Fixes the build issue, but I only tested it with ninja single config generator, behavior should be the same on all others though.