Skip to content

executorch-the-CMake-target should re-export executorch_core, shouldn't it? #8404

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

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Feb 12, 2025

This seems to fix QNN build failures, and it doesn't make sense to me that every executorch (the CMake target) consumer should also have to explicitly link executorch_core

[ghstack-poisoned]
[ghstack-poisoned]
@swolchok
Copy link
Contributor Author

swolchok commented Feb 12, 2025

@facebook-github-bot facebook-github-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 Feb 12, 2025
Copy link

pytorch-bot bot commented Feb 12, 2025

🔗 Helpful Links

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

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

❌ 2 Cancelled Jobs

As of commit aa28e1c with merge base ee7d388 (image):

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

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

swolchok added a commit that referenced this pull request Feb 12, 2025
suspect this might fix qnn build failures on main

ghstack-source-id: ef74e78
ghstack-comment-id: 2652553698
Pull Request resolved: #8404
@swolchok swolchok marked this pull request as draft February 12, 2025 03:07
@swolchok swolchok changed the title executorch should re-export executorch_core, shouldn't it? executorch-the-CMake-target should re-export executorch_core, shouldn't it? Feb 12, 2025
@swolchok swolchok marked this pull request as ready for review February 12, 2025 03:39
Copy link
Contributor

@cccclai cccclai left a comment

Choose a reason for hiding this comment

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

Accept to unblock

@@ -596,7 +596,7 @@ endif()
# any backends.
#
add_library(executorch ${_executorch__srcs})
target_link_libraries(executorch PRIVATE executorch_core)
target_link_libraries(executorch PUBLIC executorch_core)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure how this change fixes the error, but looks like CI is green

Base automatically changed from gh/swolchok/255/head to main February 12, 2025 16:01
@swolchok
Copy link
Contributor Author

landing despite two job cancellations for timeout because it seems unlikely this would be related and it's an unbreak

@swolchok swolchok merged commit 5dd2ed3 into main Feb 12, 2025
106 of 108 checks passed
@swolchok swolchok deleted the gh/swolchok/256/head branch February 12, 2025 16:02
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants