Stop using autoglob mode constants#18022
Conversation
Summary: There's no point to using constants for this, it's an immediate Buck parse error if you use an invalid mode. It's an extra step the AI needs to figure out (and humans!) - just use strings. Reviewed By: d16r Differential Revision: D95399679
manuelcandales
requested review from
larryliu0820,
mergennachin and
shoumikhin
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18022
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
@manuelcandales has exported this pull request. If you are a Meta employee, you can view the originating Diff in D95399679. |
There was a problem hiding this comment.
Pull request overview
This PR replaces the use of the EXPORT_UNLESS_INTERNAL Starlark constant with its equivalent string literal "EXPORT_UNLESS_INTERNAL" in two Apple BUCK build files. The motivation is to simplify the build files: using the string directly is more readable, reduces indirection, and eliminates the need to import an external constant just for this purpose. An invalid string would be caught as an immediate Buck parse error anyway, so there's no safety benefit to using the constant.
Changes:
extension/llm/apple/BUCK: Changedautoglob_modefrom the constantEXPORT_UNLESS_INTERNALto the string"EXPORT_UNLESS_INTERNAL".extension/apple/BUCK: Same change as above for theExecuTorchlibrary target.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
extension/llm/apple/BUCK |
Replaces autoglob_mode = EXPORT_UNLESS_INTERNAL with autoglob_mode = "EXPORT_UNLESS_INTERNAL" |
extension/apple/BUCK |
Replaces autoglob_mode = EXPORT_UNLESS_INTERNAL with autoglob_mode = "EXPORT_UNLESS_INTERNAL" |
Note: Both files still retain the now-unused load("@fbsource//tools/build_defs/apple:autoglob.bzl", "EXPORT_UNLESS_INTERNAL") statement on line 3. Since the constant is no longer referenced after this change, these load statements should also be removed to avoid dead imports.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| non_fbcode_target(_kind = fb_apple_library, | ||
| name = "ExecuTorchLLM", | ||
| autoglob_mode = EXPORT_UNLESS_INTERNAL, | ||
| autoglob_mode = "EXPORT_UNLESS_INTERNAL", |
There was a problem hiding this comment.
After replacing the constant with the string literal "EXPORT_UNLESS_INTERNAL", the load statement on line 3 that imports EXPORT_UNLESS_INTERNAL from autoglob.bzl is now unused. This dead import should be removed to keep the file clean and avoid any potential lint/parse warnings about unused symbols.
| non_fbcode_target(_kind = fb_apple_library, | ||
| name = "ExecuTorch", | ||
| autoglob_mode = EXPORT_UNLESS_INTERNAL, | ||
| autoglob_mode = "EXPORT_UNLESS_INTERNAL", |
There was a problem hiding this comment.
After replacing the constant with the string literal "EXPORT_UNLESS_INTERNAL", the load statement on line 3 that imports EXPORT_UNLESS_INTERNAL from autoglob.bzl is now unused. This dead import should be removed to keep the file clean and avoid any potential lint/parse warnings about unused symbols.
manuelcandales
added
the
release notes: none
Summary: There's no point to using constants for this, it's an immediate Buck parse error if you use an invalid mode. It's an extra step the AI needs to figure out (and humans!) - just use strings. Reviewed By: d16r Differential Revision: D95399679 Co-authored-by: Adam Ernst <adamjernst@meta.com>
Summary: There's no point to using constants for this, it's an immediate Buck parse error if you use an invalid mode. It's an extra step the AI needs to figure out (and humans!) - just use strings.
Reviewed By: d16r
Differential Revision: D95399679