Don't use iOS toolchain on macOS - #12578

Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom
gh/swolchok/516/head
Closed

Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom
gh/swolchok/516/head

Conversation

@swolchok

@swolchokswolchok commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

I don't know why we were doing this, but it seems wrong. Let's see if tests pass.

@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok

swolchok commented Jul 17, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Stack from ghstack (oldest at bottom):

@pytorch-bot

pytorch-botBot commented Jul 17, 2025•
edited
Loading

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 11 New Failures, 3 Unrelated Failures

As of commit 4ac87e3 with merge base 986be2a (image):

NEW FAILURES - The following jobs have failed:

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-clameta-claBot 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 Jul 17, 2025
swolchok added a commit that referenced this pull request Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578
This was referenced Jul 17, 2025
Closed
Closed
Closed
Merged
Closed
Merged
Merged
Merged
@swolchok
swolchok requested a review from jathuJuly 17, 2025 00:51
@swolchokswolchok added the release notes: build Changes related to build, including dependency upgrades, build flags, optimizations, etc. label Jul 17, 2025
@swolchok

swolchok commented Jul 17, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

might want to pick this to release, not sure. comes from #10768

@swolchokswolchok mentioned this pull request Jul 17, 2025
Merged
@jathu

jathu commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake

@swolchok

swolchok commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake

using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?

@jathu

jathu commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?

shoumikhin
shoumikhin reviewed Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
"inherits": ["common"],
"generator": "Xcode",
"cacheVariables": {
"CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",

@shoumikhinshoumikhinJul 17, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.
It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.

@swolchokswolchokJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?

@shoumikhinshoumikhinJul 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.
I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40

@swolchokswolchokJul 17, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.

I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.

What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)

@shoumikhinshoumikhinJul 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.
Just a few quick notes:

  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.

Maybe just disable that particular flag like in #12594

@swolchokswolchokJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.

@swolchokswolchokJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.

@swolchok

swolchok commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

build-frameworks-ios error:

2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?

@swolchokswolchok mentioned this pull request Jul 17, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees

No one assigned

Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Don't use iOS toolchain on macOS - #12578
Closed
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Closed
Don't use iOS toolchain on macOS#12578
swolchok wants to merge 1 commit into
gh/swolchok/515/headfrom 
gh/swolchok/516/head
Conversation
@swolchok
@swolchokswolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
@swolchok
Update
4ac87e3
[ghstack-poisoned]
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Stack from ghstack (oldest at bottom):
@pytorch-bot
pytorch-botBot
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12578
Note: Links to docs will display an error until the docs builds have been completed.
❌ 11 New Failures, 3 Unrelated Failures
As of commit 4ac87e3 with merge base 986be2a (image):
NEW FAILURES - The following jobs have failed:
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-clameta-claBot
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
Jul 17, 2025
swolchok added a commit
that referenced
this pull request
Jul 17, 2025
@swolchok
Don't use iOS toolchain on macOS
a4a6444
I don't know why we were doing this, but it seems wrong. Let's see if tests pass.
ghstack-source-id: 26d2882
ghstack-comment-id: 3081988867
Pull-Request: #12578

This was referenced Jul 17, 2025
 Closed

 Closed

 Closed

 Merged

 Closed

 Merged

 Merged

 Merged

@swolchok
swolchok
requested a review
from jathuJuly 17, 2025 00:51
@swolchokswolchok
added
the 
release notes: build
Changes related to build, including dependency upgrades, build flags, optimizations, etc.
label
Jul 17, 2025
@swolchok
swolchok
commented
Jul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
might want to pick this to release, not sure. comes from #10768
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
I think ios.toolchain.cmake is probably poor naming, but it is a general Apple cmake toolchain to build for all of their platforms (controlled via PLATFORMS): https://github.com/leetal/ios-cmake
using this toolchain breaks sentencepiece's build in the next PR. Why aren't CMake's default settings on macOS sufficient?
@jathu
jathu
commented
Jul 17, 2025

Copy link


Copy Markdown

Contributor
@shoumikhin do you have context on why we use the toolchain instead of relying the on the default host configs?
shoumikhin
shoumikhin
reviewed
Jul 17, 2025
View reviewed changes
Comment threadCMakePresets.json
 "inherits": ["common"],
 "generator": "Xcode",
 "cacheVariables": {
 "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/third-party/ios-cmake/ios.toolchain.cmake",
@shoumikhinshoumikhinJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

iirc, this is not iOS-only toolchain, we use it to build mac targets too.

It contains a bunch of useful declarations to simplify our live, e.g. the very MAC_ARM64 platform definition.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not the normal way that CMake projects expect to build for Mac though. concretely, sentencepiece's build does not like CMAKE_MACOSX_BUNDLE being set on non-iOS platforms, and it wouldn't be set on macOS if not for the use of this toolchain. IMO we should keep everything standard and defaulted unless we have a reason not to. Why do we need to use this to build for Mac?
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

So we rely on that toolchain platform definitions with the extra cmake flags they add to build frameworks for iOS and macOS. So far they worked well. If you believe there's a better approach, let's explore it too.

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
Screenshot 2025-07-16 at 18 40 40
@swolchokswolchokJul 17, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I guess we can disabled CMAKE_MACOSX_BUNDLE for sentencepiece specifically when building for mac, that's what we do with the custom script build phase in Xcode when building extension/llm third-party deps.
I agree that we can work around this and it's straightforward to do so. My concern is that the setting of this toolchain cost me half an hour of debugging time to track down, because it was surprising that 1) any toolchain was active for macos, since builds work fine without it when not using presets, and 2) "ios.toolchain.cmake" in particular was active. This will presumably continue to be surprising to others in the future.
What if instead we set the CMAKE_TOOLCHAIN_FILE only in build_apple_frameworks.sh? Do we need it anywhere else? (Long-term I think we should investigate using CMake's built-in support for producing Apple frameworks, but I don't see a particular reason to prioritize that.)
@shoumikhinshoumikhinJul 17, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don’t have a strong preference - so long as we can build the frameworks as usual, I’m happy either way.

Just a few quick notes:
  • Despite its name, this toolchain isn’t limited to iOS.
  • The last time I tried building the frameworks by manually setting all the macOS-specific flags myself - it ended up costing way more time 😅
  • Otherwise, this toolchain has worked well so far.
Maybe just disable that particular flag like in #12594
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, maybe #12594 will unblock me. If not I'll see if I can find another way to unblock export since I've found out that this is here for a reason and not just a mistake.
@swolchokswolchokJul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep, I rebased and it looks like we don't need this.
@swolchok
swolchok
commented
Jul 17, 2025

Copy link


Copy Markdown

ContributorAuthor
build-frameworks-ios error:
2025-07-17T01:17:59.6638640Z Merging libraries:
2025-07-17T01:17:59.6638760Z libexecutorch.a
2025-07-17T01:17:59.6638860Z libexecutorch_core.a
2025-07-17T01:17:59.6638970Z libextension_apple.a
2025-07-17T01:17:59.6639080Z libextension_data_loader.a
2025-07-17T01:17:59.6639210Z libextension_flat_tensor.a
2025-07-17T01:17:59.6639330Z libextension_module.a
2025-07-17T01:17:59.6639440Z libextension_tensor.a
2025-07-17T01:17:59.6639560Z from simulator/Release
2025-07-17T01:17:59.6639880Z into library /Users/runner/work/executorch/executorch/pytorch/executorch/cmake-out/libexecutorch_simulator.a
2025-07-17T01:17:59.6640280Z File simulator/Release/libextension_apple.a does not exist

so I guess this script might need CMAKE_TOOLCHAIN_FILE put back?
@swolchokswolchok
mentioned this pull request
Jul 17, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@shoumikhinshoumikhinshoumikhin left review comments
@larryliu0820larryliu0820Awaiting requested review from larryliu0820
@kirklandsignkirklandsignAwaiting requested review from kirklandsign
@jathujathuAwaiting requested review from jathu

Assignees


No one assigned


Labels

ciflow/binariesciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: buildChanges related to build, including dependency upgrades, build flags, optimizations, etc.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@swolchok@jathu@shoumikhin