Skip to content

Conversation

Erik-Lundell
Copy link
Collaborator

@Erik-Lundell Erik-Lundell commented Sep 23, 2025

This is a re-upload of #14435, with variables moved inside #ifdef to not get unused variables when building without devtools.

Add some cmake to only do this if executorch is built with bundleio.

codegen/tools subdirectory include needs to be moved in top-level CmakeLists.txt
to have access to the bundled_program target.

Follow-up patch to enable the fix in arm backend.

cc @digantdesai @freddan80 @per @zingo @oscarandersson8218

Add some cmake to only do this if executorch is built with
bundleio.

codegen/tools subdirectory needs to be moved in top-level CmakeLists.txt
to have access to the bundled_program target.

Signed-off-by: Erik Lundell <[email protected]>
Change-Id: Ic953bdfd5223eb835e46150bde19b08b082e8444
Signed-off-by: Erik Lundell <[email protected]>
Change-Id: Ib7fbdb7190312320d73a937bc285db7140d2bb1a
Copy link

pytorch-bot bot commented Sep 23, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14503

Note: Links to docs will display an error until the docs builds have been completed.

❌ 5 New Failures, 2 Cancelled Jobs, 3 Unrelated Failures

As of commit 390840c with merge base df5bfd5 (image):

NEW FAILURES - The following jobs have failed:

CANCELLED JOBS - The following jobs were cancelled. Please retry:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 23, 2025
@Erik-Lundell
Copy link
Collaborator Author

Merge should wait until #14488 to make sure Meta's internal build is not broken

@Erik-Lundell Erik-Lundell added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm release notes: none Do not include this in the release notes ciflow/trunk labels Sep 23, 2025
)

# Link against required libraries
if(TARGET bundled_program)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you check EXECUTORCH_BUILD_DEVTOOLS?

Something like

if (EXECUTORCH_BUILD_DEVTOOLS and TARGET bundled_program)
   ... 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this since it is done that way on L754 in top-level CMakeLists.txt. Is there a benefit in additionally checking the flag or just to be on the safe side?

Comment on lines +33 to +34
target_compile_definitions(selective_build PRIVATE -DET_BUNDLE_IO)
target_link_libraries(selective_build PRIVATE bundled_program)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also wanna change the targets.bzl file too

I added a CI to build buck target for selective_build

#14499

For future reference, here's how you can download buck:

python tools/cmake/resolve_buck.py --cache_dir==/tmp/
# it will download and output a buck executable file within /tmp/ file

Once you have the buck file, you can do something like this:

/tmp/buck2 build //codegen/tools/... 
# build all targets within //codegen/tools/...
/tmp/buck2 build codegen/tools:selective_build
# build all targets

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look and the bundled_program seems to have multiple targets runtime and runtime_aten, depending on some get_aten_mode_options(). I need some assistance to figure this out. From what I can tell, if I add the different targets to selective_build that target is also split which means that targets depending on it will also split and so on... That doesn't sound right.

Copy link
Contributor

@mergennachin mergennachin Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Erik-Lundell -- no need to bifurcate on aten vs not. You can add dependency on "//executorch/devtools/bundled_program:runtime

Historically, runtime_aten is mainly for internal customers when portable ops coverage was low. That's not the case, so you should be able to just depend on "//executorch/devtools/bundled_program:runtime directly

@facebook-github-bot
Copy link
Contributor

@mergennachin has imported this pull request. If you are a Meta employee, you can view this in D83148788.

Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with internal CI as well. Stamping..

@zingo zingo merged commit d0f486a into pytorch:main Sep 29, 2025
268 of 278 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm release notes: none Do not include this in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants