build: fix accidental cmake expansions #65534
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephank
requested review from
ahoppen,
bnbarham,
rintaro,
zoecarver,
hyp,
egorzhdan,
hborla,
slavapestov,
xedin,
CodaFi,
DougGregor,
compnerd and
eeckstein
as code owners
The way it usually works is that you get it into |
|
@swift-ci please smoke test |
|
Thanks, can confirm this significantly improves build progress on Fedora 39 (rawhide), however I still see the following building Swift: |
|
It's possible I missed some detail on the I'm currently not setup to easily test other branches of Swift than the one version we package in Nixpkgs. Adding some support for that to Nixpkgs is something I'd like, but also a bit of work. I can't really plan that work, because this is spare time effort for me. :) Perhaps a follow-up PR can fix further issues? |
|
Ok, thanks, I appreciate your efforts anyway and good to have some idea of the root cause. The above is against |
|
|
Understood, thanks! I have now tried a |
compnerd
approved these changes
May 15, 2023
| @@ -427,7 +427,7 @@ function(_add_host_variant_link_flags target) | |||
| # | |||
| # TODO: Evaluate/enable -f{function,data}-sections --gc-sections for bfd, | |||
| # gold, and lld. | |||
| if(NOT CMAKE_BUILD_TYPE STREQUAL Debug AND CMAKE_SYSTEM_NAME MATCHES Darwin) | |||
| if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND CMAKE_SYSTEM_NAME MATCHES Darwin) | |||
There was a problem hiding this comment.
Should the RHS of the MATCHES be quoted as well? While a regex, it is still stringly-typed right?
There was a problem hiding this comment.
I've not seen any breakage because of this in our Nixpkgs build. MATCHES RHS doesn't accept variable, so should be safe, I believe.
However, if you'd like me to fix this up, it's easy enough to grep. There are only 18 occurrences. (And FWIW, no cases of LINUX or ANDROID.)
|
@stephank, this cannot go anywhere till you rebase and fix any conflicts. |
|
@bnbarham, would you run the mac CI on this again? Let's see if the last failure reproduces, as I suspect it was spurious. |
|
I don't think I've seen that one (and I'm wrangling this week). But I'll re-run 👍 |
|
@swift-ci please test macOS platform |
|
OK, it reproduces. @stephank, any idea why this pull would be failing like this on macOS alone? |
|
I've been staring at the PR, but can't find my mistake. It seems the Jenkins log has expired. I'll try to find some time to reproduce it soon. |
|
@kateinoigakukun, would you run the mac CI on this again, so he can examine the log, as the prior log has been removed? |
|
@swift-ci please test macOS platform |
|
For reference: |
|
I don't think it's setting the Apple targets the way it should. I diffed a recent passing build log against this one and see two mistakes in the CMake config, both related to the simulators. The passing one says: while this pull strangely produces this, ie both simulator SDKs are not listed as simulators: |
As of CMake 3.25, there are now global variables `LINUX=1`, `ANDROID=1`,
etc. These conflict with expressions that used these names as unquoted
strings in positions where CMake accepts 'variable|string', for example:
- `if(sdk STREQUAL LINUX)` would fail, because `LINUX` is now defined and
expands to 1, where it would previously coerce to a string.
- `if(${sdk} STREQUAL "LINUX")` would fail if `sdk=LINUX`, because the
left-hand side expands twice.
In this patch, I looked for a number of patterns to fix up, sometimes a
little defensively:
- Quoted right-hand side of `STREQUAL` where I was confident it was
intended to be a string literal.
- Removed manual variable expansion on left-hand side of `STREQUAL`,
`MATCHES` and `IN_LIST` where I was confident it was unintended.
Fixes swiftlang#65028.
|
Right, it looks like Prints: Fixes are:
The |
Oh yeah! I'd forgotten about that. Another reason why I usually like to steer away from macros. Thanks for looking into this further. |
|
@bnbarham, hopefully one last CI run? |
|
@swift-ci please test |
|
Sourcekit-lsp test on linux CI timed out, appears completely unrelated, that CI alone will need to be rerun. |
|
@swift-ci please test Linux |
|
I think that's the same sourcekit-lsp timeout error again? |
|
@swift-ci please test Linux |
Yes, the revert wasn't merged until this morning. |
|
@al45tair, ready for merge. |
|
@etcwilde, ready for merge. |
|
@stephank, do you want to submit this for 5.9 next? See the 5.9 release process and a recent example pull, #67391. |
I don't have a need for it myself, so was not planning on doing it. Is there even going to be another 5.8 release? This is the patch Nixpkgs currently uses for 5.8: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/compilers/swift/compiler/patches/swift-cmake-3.25-compat.patch |
There might: the fall release like 5.9 usually releases in September, and there is sometimes an August patch release before that. @etcwilde, wdyt, should he submit this for 5.8 too? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of CMake 3.25, there are now global variables
LINUX=1,ANDROID=1, etc. These conflict with expressions that used these names as unquoted strings in positions where CMake accepts 'variable|string', for example:if(sdk STREQUAL LINUX)would fail, becauseLINUXis now defined and expands to 1, where it would previously coerce to a string.if(${sdk} STREQUAL "LINUX")would fail ifsdk=LINUX, because the left-hand side expands twice.In this patch, I looked for a number of patterns to fix up, sometimes a little defensively:
Quoted right-hand side of
STREQUALwhere I was confident it was intended to be a string literal.Removed manual variable expansion on left-hand side of
STREQUAL,MATCHESandIN_LISTwhere I was confident it was unintended.Fixes #65028.
Question: I have a branch for 5.8. This is what I've actually been testing in Nixpkgs, and that builds on x86-64 Linux and aarch64 macOS. Between 5.8 and
main, it looks like the only conflicts were CMake additions inmain. Should I open a PR againstrelease/5.8now, or after review?