-
Notifications
You must be signed in to change notification settings - Fork 650
Add check_for_installed_private_headers_in_cmake_out #13485
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
base: main
Are you sure you want to change the base?
Conversation
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
This adds a test that will run in CI to make sure that we don't install private headers. I fixed the existing places where we were installing private headers; reviewers, please confirm that we are OK with this technical break of source-level backward compatibility. ghstack-source-id: ca0dbcd ghstack-comment-id: 3198151825 Pull-Request: #13485
This adds a test that will run in CI to make sure that we don't install private headers. I fixed the existing places where we were installing private headers; reviewers, please confirm that we are OK with this technical break of source-level backward compatibility. ghstack-source-id: 733e1e5 ghstack-comment-id: 3198151825 Pull-Request: #13485
noting CI looks good |
…ripts/build_apple_frameworks.sh This removes the need to use Buck in this script. The previous PR (#13485) adds enforcement that we don't install non-public headers from CMake. I built the frameworks before and after. Here is a diff of all headers present in the frameworks: https://gist.github.com/swolchok/70647b551da7827a9cbdb083fad1e209 Generally, we now include more headers than before. The only header we used to include that we no longer include is schema/extended_header.h. It looks like we are not including the rest of the headers for the PTE schema anyway, so I propose that we fix installation of the schema headers separately. ghstack-source-id: 626397b ghstack-comment-id: 3207551649 Pull-Request: #13559
) | ||
install( | ||
DIRECTORY runtime/executor/ | ||
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/executorch/runtime/executor | ||
FILES_MATCHING | ||
PATTERN "*.h" | ||
PATTERN "test" EXCLUDE | ||
PATTERN "platform_memory_allocator.h" EXCLUDE |
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.
I need to follow up with Greg on what this even is used for.
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.
Only one that is maybe used would be testing_util but I really hope no one would be using that lol. The rest seem totally fine bc breaks.
This adds a test that will run in CI as part of unittest jobs to make sure that we don't install private headers.
I fixed the existing places where we were installing private headers; reviewers, please confirm that we are OK with this technical break of source-level backward compatibility.