Skip to content

CMake: fix build for Apple Silicon hosts#34998

Merged
MaxDesiatov merged 2 commits intomainfrom
maxd/fix-apple-silicon
Jan 6, 2021
Merged

CMake: fix build for Apple Silicon hosts#34998
MaxDesiatov merged 2 commits intomainfrom
maxd/fix-apple-silicon

Conversation

@MaxDesiatov
Copy link
Copy Markdown
Contributor

@MaxDesiatov MaxDesiatov commented Dec 8, 2020

When building with build-script using these arguments

utils/build-script --skip-build-benchmarks
  --skip-ios --skip-watchos --skip-tvos
  --swift-darwin-supported-archs "arm64"
  --sccache --release-debuginfo --test

the build fails with

ninja: error: 'stdlib/swift-test-stdlib-macosx-x86_64',
needed by 'stdlib/CMakeFiles/swift-test-stdlib', missing and no known rule to make it

I think that the "Getting Started" guide should avoid hardcoding x86_64 arguments, and suggest using $(uname -m) instead. SWIFT_PRIMARY_VARIANT_ARCH_default could also get its value from CMAKE_HOST_SYSTEM_PROCESSOR in the root CMakeLists.txt.

Resolves SR-13943.

@MaxDesiatov
Copy link
Copy Markdown
Contributor Author

@swift-ci please smoke test

@stephentyrone
Copy link
Copy Markdown
Contributor

Tagging @compnerd and @gottesmm, since they're my usual go-tos for CMake-fu.

Copy link
Copy Markdown
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Id want an okay from Michael on this as well; the CMake side of the change is proper, the question is, is there anything that depends on this?

@varungandhi-apple
Copy link
Copy Markdown
Contributor

Thanks for the PR. There are some other places where macosx-x86_64 is specified directly and you may want to update those. Specifically:

  1. utils/coverage/coverage-generate-data
  2. benchmark/README.md
  3. clang-tidy instructions in docs/DebuggingTheCompiler.md.

@MaxDesiatov MaxDesiatov marked this pull request as ready for review December 8, 2020 17:12
@MaxDesiatov
Copy link
Copy Markdown
Contributor Author

MaxDesiatov commented Dec 8, 2020

@varungandhi-apple thanks for highlighting those, updated now.

@MaxDesiatov
Copy link
Copy Markdown
Contributor Author

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov changed the title CMake: fix builds for Apple Silicon hosts CMake: fix build for Apple Silicon hosts Dec 8, 2020
@varungandhi-apple
Copy link
Copy Markdown
Contributor

@MaxDesiatov can this be merged/are you waiting on something specific?

@MaxDesiatov
Copy link
Copy Markdown
Contributor Author

IDK, do I need for an explicit approval from @gottesmm before this can be merged? As @compnerd mentioned above:

Id want an okay from Michael on this as well

MaxDesiatov added a commit to swiftwasm/swift that referenced this pull request Dec 27, 2020
CMake: fix builds for Apple Silicon hosts

When building with `build-script` using these arguments

```
utils/build-script --skip-build-benchmarks
  --skip-ios --skip-watchos --skip-tvos
  --swift-darwin-supported-archs "arm64"
  --sccache --release-debuginfo --test
```

the build fails with

```
ninja: error: 'stdlib/swift-test-stdlib-macosx-x86_64',
needed by 'stdlib/CMakeFiles/swift-test-stdlib', missing and no known rule to make it
```

I think that the "Getting Started" guide should avoid hardcoding `x86_64` arguments, and suggest using `$(uname -m)` instead. `SWIFT_PRIMARY_VARIANT_ARCH_default` could also get its value from `CMAKE_HOST_SYSTEM_PROCESSOR` in the root `CMakeLists.txt`.

Same as upstream swiftlang#34998.
@davidungar
Copy link
Copy Markdown
Contributor

davidungar commented Jan 5, 2021

@MaxDesiatov @compnerd @varungandhi-apple I independently stumbled across this issue over break. (Too bad this PR wasn't merged then.) My version of a fix is in #35231 .
It uses SWIFT_HOST_VARIANT_ARCH instead of the fix in this PR. Which PR would be better to merge?
@gottesmm , want to weigh in?

@varungandhi-apple
Copy link
Copy Markdown
Contributor

IMO we should merge this, since it also updates the documentation. We don't need to be blocked on review, we can change things after-the-fact if there's an issue.

@MaxDesiatov
Copy link
Copy Markdown
Contributor Author

@swift-ci please smoke test macOS platform

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants