Skip to content

Conversation

swolchok
Copy link
Contributor

By accidentally fixing the code in test/build_size_test.sh to correctly detect release builds (EQUAL is for numbers; you need STREQUAL for strings), #12045 broke that script on Mac because --gc-sections is not a valid linker flag there. This PR creates a general utility function for stripping dead code in linker flags and uses it everywhere we currently set --gc-sections.

@swolchok
Copy link
Contributor Author

swolchok commented Jun 28, 2025

Copy link

pytorch-bot bot commented Jun 28, 2025

🔗 Helpful Links

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

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

❌ 2 New Failures, 7 Pending, 1 Unrelated Failure

As of commit 665c8f0 with merge base cf0bfd2 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job 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.

swolchok added a commit that referenced this pull request Jun 28, 2025
By accidentally fixing the code in test/build_size_test.sh to correctly detect release builds (EQUAL is for numbers; you need STREQUAL for strings), #12045 broke that script on Mac because `--gc-sections` is not a valid linker flag there. This PR creates a general utility function for stripping dead code in linker flags and uses it everywhere we currently set `--gc-sections`.


ghstack-source-id: 58a2fd2
ghstack-comment-id: 3014707984
Pull-Request-resolved: #12089
@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 Jun 28, 2025
@swolchok swolchok added the release notes: none Do not include this in the release notes label Jun 28, 2025
[ghstack-poisoned]
@mergennachin
Copy link
Contributor

For low level build stuff, let's add ciflow/trunk labels so that we can catch them earlier

@mergennachin mergennachin merged commit b42dc6d into main Jun 28, 2025
198 of 201 checks passed
@mergennachin mergennachin deleted the gh/swolchok/484/head branch June 28, 2025 05:03
mergennachin added a commit that referenced this pull request Jun 28, 2025
Fix llava test target cmake

#12089 had a typo
@swolchok
Copy link
Contributor Author

For low level build stuff, let's add ciflow/trunk labels so that we can catch them earlier

I didn't realize this failure showed up on CI somewhere; I noticed because I was running the script locally. I thought that we only ran size tests on Linux.

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. release notes: none Do not include this in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants