Repository navigation
Rename executorch cmake target to prim_ops_lib - #23177
TejasweeSulekh wants to merge 3 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23177
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 056bfe5 with merge base 78a4734 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude review this code. Does _executorch__srcs (which defined the old executorch library) really only contain primp op sources? If so, should _executorch__srcs be renamed? |
|
cc @JacobSzwejbka for core runtime |
|
Claude finished @metascroy's task in 2m 5s —— View job Review: Rename
|
|
Thanks for the thorough review, and my bad I completely missed the downstream linking impacts and hardcoded script names. I'll push an update tomorrow to fix the broken references and add the INTERFACE compatibility target so we don't break existing users. |
90acc60 to
0059406
Compare
|
@pytorchbot label "release notes: build" |
|
Hey @nil-is-all, I've pushed a new commit which is addressing the feedback provided above. Which are as follows:
Just to confirm this is working correctly, I ended up executing a dummy I also took the liberty to update the main description of the PR to be more aligned to the final changes. |
0059406 to
d1f2e41
Compare
|
I added a new commit because the CI failed on the MLX, VULKAN, and Android configuration due to an edge case with CMake generator expressions, which I have now fixed across the repository. Basically, by making However, several custom commands in the examples (like MLX and Android) were using the low-level To fix this, I did a sweep across all Because |
|
Also @nil-is-all, is it possible for me to run the CI tests in a draft PR? Such that I could confirm the changes are valid and then move the PR to ready for review? As I would feel more comfortable running the CI tests workflow in that setting. |
Few workflows based on GitHub automations are triggered only when a PR is non-draft; but given that we can review once you flag a PR ready for review. Most contributors address CI test logs on non-draft PRs and ping us once they are satisfied with the PR to be reviewed. |
Thanks for the clarification :D |
d1f2e41 to
056bfe5
Compare
|
Hi @nil-is-all, I've rebased the branch onto the latest
Whenever you get the time can you please run the CI test workflow on this PR? |
|
Hey @JacobSzwejbka and @nil-is-all, I think the CI tests ended up passing. Let me know if there is something more that might be required in this PR |
Summary
This PR renames the confusing
executorchCMake target toprim_ops_lib. This change clearly reflects its purpose as the Primitive Operations Library and brings it in line with the naming convention ofportable_ops_lib.Key Changes
executorchtarget definition inCMakeLists.txttoprim_ops_lib, alongside its source file arrays (_prim_ops_lib__srcsandPRIM_OPS_LIB_SRCS).INTERFACECMake target namedexecutorchthat proxies toprim_ops_lib. Downstream consumers callingtarget_link_libraries(myapp executorch)will not break.set_target_properties(... OUTPUT_NAME "executorch")to ensure the compiled archive is still namedlibexecutorch.a. This prevents breakages in the dozens of internal CI shell scripts and iOS Xcode.pbxprojconfigurations that explicitly look for that file.prim_ops_lib.executorch_target_link_options_shared_lib(prim_ops_lib)to its CMake file.Fixes #11761
Test plan
Configured, built, and executed a model at runtime to guarantee no primitive operator symbols were stripped by the linker.
cc @nil-is-all