-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
releases: use LLVM 18 for macOS #30022
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
d4eac99
depends: fix Qt macOS build with Clang 18
fanquake a870a28
guix: bump time-machine to 1fa1325c0b4bb5c28e564526f387d82083733104
fanquake 338da55
depends: bump native_llvm up to 18.1.4
fanquake 9170786
guix: use clang-toolchain-18 for macOS build
fanquake File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
fix Qt macOS build with Clang 18 | ||
|
||
See: | ||
https://github.com/pnggroup/libpng/commit/893b8113f04d408cc6177c6de19c9889a48faa24. | ||
|
||
In a similar manner as zlib (madler/zlib#895), | ||
libpng contains a header configuration that's no longer valid and | ||
hasn't been exercised for the macOS target. | ||
|
||
- The target OS conditional macros are misused. Specifically | ||
`TARGET_OS_MAC` covers all Apple targets, including iOS, and it | ||
should not be checked with `#if defined` as they would always be | ||
defined (to either 1 or 0) on Apple platforms. | ||
- `#include <fp.h>` no longer works for the macOS target and results | ||
in a compilation failure. macOS ships all required functions in | ||
`math.h`, and clients should use `math.h` instead. | ||
|
||
--- a/qtbase/src/3rdparty/libpng/pngpriv.h | ||
+++ b/qtbase/src/3rdparty/libpng/pngpriv.h | ||
@@ -514,18 +514,8 @@ | ||
*/ | ||
# include <float.h> | ||
|
||
-# if (defined(__MWERKS__) && defined(macintosh)) || defined(applec) || \ | ||
- defined(THINK_C) || defined(__SC__) || defined(TARGET_OS_MAC) | ||
- /* We need to check that <math.h> hasn't already been included earlier | ||
- * as it seems it doesn't agree with <fp.h>, yet we should really use | ||
- * <fp.h> if possible. | ||
- */ | ||
-# if !defined(__MATH_H__) && !defined(__MATH_H) && !defined(__cmath__) | ||
-# include <fp.h> | ||
-# endif | ||
-# else | ||
-# include <math.h> | ||
-# endif | ||
+# include <math.h> | ||
+ | ||
# if defined(_AMIGA) && defined(__SASC) && defined(_M68881) | ||
/* Amiga SAS/C: We must include builtin FPU functions when compiling using | ||
* MATH=68881 |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this download link works but ubuntu-18?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in LLVM 19: https://discourse.llvm.org/t/rfc-improve-binary-security/78121/56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. While using the pre-built bins, we have to take what is available. For some reason the person building the x86_64 bins here has reverted to that. It also means we'll need to revert to installing libtinfo5 in the macOS CI, which is also annoying, because you also can't actually install that on Ubuntu 24.04 (although we currently still use 22.04 here, so it'll work for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or waiting after #21778, which should be fine as well, as there is no rush to use clang 18?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible now to nuke this and require the user (if they want to cross-compile to macOS) to type
apt install clang-18
instead?I presume leaving this to the user will allow to run the cross-compile on more distros and arches, as opposed to being possibly confined to
x86_64-linux-gnu-ubuntu
when using the llvm artefacts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, the plan is to switch away from vendoring Clang entirely, which is more straightforward now that #21778 is merged. I'll be followingup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First step in #30198.