Work around lack of embed in clang on OpenBSD and Amazon Linux 2. #1320
Conversation
3405691582
requested review from
stmontgomery,
grynspan,
briancroom,
suzannaratcliff and
jerryjrchen
as code owners
|
@swift-ci please test. |
grynspan
added
bug
| elseif(CMAKE_SYSTEM_NAME STREQUAL "OpenBSD") | ||
| target_compile_options(_TestingInternals PRIVATE | ||
| -fno-exceptions | ||
| -DSWT_TESTING_LIBRARY_VERSION="${SWT_VERSION_FILE_CONTENTS}") |
There was a problem hiding this comment.
Looks like you moved the definition of ${SWT_VERSION_FILE_CONTENTS} to CompilerSettings.cmake, but not that of SWT_TESTING_LIBRARY_VERSION.
What' I'd ask you do here is:
- Remove this
-Dline from this file, as it's inconsistent with how we define other C++ macros - Add an equivalent
add_compile_definitions()call in CompilerSettings.cmake after the call tofile(STRINGS) - And this last one is nitpicky but call the intermediate CMake value
${SWT_TESTING_LIBRARY_VERSION}instead of${SWT_VERSION_FILE_CONTENTS}for consistency and so it's clear they refer to the same value.
Thanks much! :)
There was a problem hiding this comment.
Ah, got it. I thought to keep it more local to where it is used but got this to apply neatly with some generator expression magic.
3405691582
force-pushed
the
embed
branch
2 times, most recently
from
2e3612c to
5484a49
Compare
For various reasons, the Swift toolchain for OpenBSD relies on using the platform's native clang, which is 16. clang 19 is the most recent version that will not emit an error with the new __has_embed features in C23. Since swift-testing is experimentally supported by OpenBSD and thus to make swift-testing build again on the platform, work around the issue with a platform-specific command-line specified macro override in swiftpm and in cmake. Furthermore, we can use cmake trickery to subsitute the version file contents instead of using embed. This may not be possible to do with swiftpm, but I don't know for sure.
|
I assume you've been able to test this change, yes? Ideally both the package and CMake avenues, but since OpenBSD support is experimental I'll be okay with "whatever @3405691582 thinks is sufficient." 😬 |
|
Yes, I have been running |
Always define SWT_TESTING_LIBRARY_VERSION in CMake
Check the major clang version before attempting to use `__has_embed` or `__clang_major__`.
grynspan
changed the title
|
I've updated the PR description to cover the Amazon Linux 2 failure. See here: https://ci.swift.org/job/oss-swift-package-amazon-linux-2-aarch64/4545 The buildbot over yonder has clang 13:
|
clang in CI jobs is 17.x but can handle `__has_embed`.
|
Sorry, looks like an earlier comment I made didn't land? We noticed the same issue on Amazon Linux 2 which has clang 13 😱 hence me adjusting the PR. Thanks! |
|
Out of curiosity -- do you need to adjust the Package.swift for the Linux variants? |
|
We can't reliably distinguish Amazon Linux 2 in the package, so no. The effect is that there will be a build warning when building from a package on AL2, but that's unavoidable. AL2 is quite old and an outlier in various other ways, so this doesn't bother me too too much. |
This change adds a call to `clang --version` after `swift --version` in the PR workflow YML file. The result of this command is useful for investigating issues stemming from out-of-date clang builds such as swiftlang/swift-testing#1320.
This change adds a call to `clang --version` after `swift --version` in the PR workflow YML file. The result of this command is useful for investigating issues stemming from out-of-date clang builds such as swiftlang/swift-testing#1320.
…the cause of the failure rather than #embed itself)
|
Looks like Android was impacted as well. https://ci-external.swift.org/job/swift-main-windows-toolchain/1597 |
…hey're failing on AL2" This reverts commit 7ca953a.
|
Sorry for the noise, @3405691582! Just working through CI problems here. We'll merge this change shortly. |
|
No problems, it's interesting to see the ancillary changes :D |
…ase it gets a clang update in the future
Fixes a bug introduced in #1320.
Fixes a bug introduced in #1320. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
|
@3405691582 I put together a branch here that uses |
Work around lack of embed in OpenBSD's clang.
Motivation:
For various reasons, the Swift toolchain for OpenBSD relies on using the platform's native clang, which is 16. clang 19 is the most recent version that will not emit an error with the new __has_embed features in C23.
Since swift-testing is experimentally supported by OpenBSD and thus to make swift-testing build again on the platform, work around the issue with a platform-specific command-line specified macro override in swiftpm and in cmake.
Furthermore, we can use cmake trickery to subsitute the version file contents instead of using embed. This may not be possible to do with swiftpm, but I don't know for sure.
Resolves rdar://160591315.
Modifications:
Checklist: