Skip to content
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

[5.9] CMake: fix missing SWIFT_CONCURRENCY_GLOBAL_EXECUTOR #65824

Merged
merged 5 commits into from
May 26, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented May 10, 2023

Explanation: Resolves issues with static linking on Linux
Risk: Medium, affects Linux builds and top-level CMake declarations.
Original PR: #65795.
Reviewed by: @al45tair @drexin @etcwilde
Resolves: some of the issues reported in #65097
Tests: Added in swiftlang/swift-integration-tests#115

SWIFT_CONCURRENCY_GLOBAL_EXECUTOR is defined in stdlib/cmake/modules/StdlibOptions.cmake, which is not included during the first pass of evaluation of the root CMakeLists.txt. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where usr/lib/swift_static/linux/static-stdlib-args.lnk didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds.

Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux.

Additionally, since are trivial tests previously didn't link Dispatch statically, the didn't expose a bug where %import-static-libdispatch substitution had a missing value. To fix that I had to update lit.cfg and clean up some of the related path computations to infer a correct substitution value.

Resolves some of the errors reported in #65097.

`SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` is defined in `stdlib/cmake/modules/StdlibOptions.cmake`, which is not included during the first pass of evaluation of the root `CMakeLists.txt`. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where `usr/lib/swift_static/linux/static-stdlib-args.lnk` didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds.

Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux.
@MaxDesiatov MaxDesiatov self-assigned this May 10, 2023
@MaxDesiatov MaxDesiatov added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. Linux Platform: Linux build-script Area → utils: The build script autolinking static libraries swift 5.9 swift-autolink-extract Area → compiler → legacy driver: the 'swift-autolink-extract' mode labels May 10, 2023
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

swiftlang/swift-integration-tests#78

@swift-ci build toolchain

It is only used in the stdlib build, so really has no business being set in the root `CMakeLists.txt`.
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci build toolchain

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci build toolchain

@MaxDesiatov MaxDesiatov marked this pull request as ready for review May 11, 2023 14:25
@MaxDesiatov MaxDesiatov requested a review from a team as a code owner May 11, 2023 14:25
@MaxDesiatov
Copy link
Contributor Author

swiftlang/swift-integration-tests#115

@swift-ci build toolchain

@MaxDesiatov MaxDesiatov requested a review from tomerd May 11, 2023 20:37
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

swiftlang/swift-integration-tests#115

@swift-ci build toolchain

@MaxDesiatov
Copy link
Contributor Author

swiftlang/swift-integration-tests#115

@swift-ci build toolchain macOS

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented May 24, 2023

Current build toolchain macos failure is unrelated and is tracked in rdar://109774179

@MaxDesiatov
Copy link
Contributor Author

swiftlang/swift-integration-tests#115

@swift-ci build toolchain macOS

@MaxDesiatov MaxDesiatov merged commit f3a3eef into release/5.9 May 26, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/5.9-toolchain-dispatch branch May 26, 2023 14:01
MaxDesiatov added a commit that referenced this pull request Jun 1, 2023
[5.8] CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR`

Explanation: Resolves issues with static linking on Linux
Risk: Medium, affects Linux builds and top-level CMake declarations.
Original PRs: #65795 and #64312 for `main`, #65824 and #64633 for `release/5.9`
Reviewed by: @al45tair @drexin @etcwilde 
Resolves: some of the issues reported in #65097, also resolves #58380
Tests: Added in swiftlang/swift-integration-tests#118

`SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` is defined in `stdlib/cmake/modules/StdlibOptions.cmake`, which is not included during the first pass of evaluation of the root `CMakeLists.txt`. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where `usr/lib/swift_static/linux/static-stdlib-args.lnk` didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds.

Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux.

Additionally, since our trivial lit tests previously didn't link Dispatch statically, they didn't expose a bug where `%import-static-libdispatch` substitution had a missing value. To fix that I had to update `lit.cfg` and clean up some of the related path computations to infer a correct substitution value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autolinking bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. build-script Area → utils: The build script Linux Platform: Linux static libraries swift 5.9 swift-autolink-extract Area → compiler → legacy driver: the 'swift-autolink-extract' mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants