-
-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
llvm: add config file, update caveats #192709
Conversation
3da97da
to
24b7c0f
Compare
24b7c0f
to
9bb969a
Compare
9bb969a
to
9b4bb2d
Compare
9b4bb2d
to
89c1ea6
Compare
Adding a system config file will allow us to stop baking in the `DEFAULT_SYSROOT` and allow users on Xcode-only installations to use the bottles. It will also make it easier to update this value when users upgrade the major version of their macOS. Before this change, this required a full reinstall. After, users will simply have to `brew postinstall llvm`. The config file change requires an upstream patch. We'll probably have to wait a little bit for it to merge, and then wait another few days to make sure it doesn't get reverted. Following discussion at #192505, let's also update the caveats to notify users of potential changes regarding `LLVM_ENABLE_EH` in LLVM 20.
89c1ea6
to
60603e8
Compare
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.
postinstall
kinda sucks to rely on for run-later code since it can go stale but is better than what we have at least.
return unless (macos_sdk = MacOS.sdk_path_if_needed) | ||
|
||
clang_config_file_dir.mkpath | ||
%w[clang.cfg clang++.cfg].each do |config_file| |
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.
Does clang-cl
, clang-cpp
and flang
work with this?
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.
I don't think we want to set --sysroot
for clang-cl
, do we? Need to check clang-cpp
, but flang
seems to work fine based on dependent testing.
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.
No sorry, didn't mean to mention clang-cl
. Not sure why I typed that.
clang-cpp
is the main concern to check.
@@ -445,10 +463,28 @@ def install | |||
end | |||
end | |||
|
|||
def post_install | |||
return unless OS.mac? | |||
return unless (macos_sdk = MacOS.sdk_path_if_needed) |
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.
Note that this is best effort and might point to a newer SDK if CLT is not installed which wouldn't be great.
There's also the Xcode-beta.app -> Xcode.app case.
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.
Fair enough, but note this is the path we set DEFAULT_SYSROOT
to so this is strictly an improvement over existing builds (because users can now fix a broken --sysroot
flag when previously they couldn't).
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's a relocatable bottle. Anybody building from source are long past the days of updating Xcode and are required by brew to have the CLT installed.
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.
So should we just require the CLT regardless?
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.
Probably. If we pursue a patch for target matching then we'll probably have to in the end anyway as we'll ship config files for all OS versions (pkg-config
style except we don't need to bake in the OS version to clang).
Additional thought to upstream: clang does support and read e.g. I can probably look into this more. |
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 2aaa52072b03..ae25b4a71c6a 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1222,6 +1222,23 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
return readConfigFile(CfgFilePath, ExpCtx);
+ auto pos = Triple.find("-apple-darwin");
+ if (pos != Triple.npos) {
+ // First, find the major-versioned triple.
+ // E.g. Triple = arm64-apple-darwin24.0.0; T = arm64-apple-darwin24
+ auto T = Triple.substr(0, Triple.find(".", pos));
+ CfgFileName = T + ".cfg";
+ if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+ return readConfigFile(CfgFilePath, ExpCtx);
+
+ // If that is not available, try an unversioned triple.
+ // E.g. Triple = x86_64-apple-darwin24.0.0; T = arm64-apple-darwin
+ T = Triple.substr(0, pos + 13); // pos + strlen("-apple-darwin")
+ CfgFileName = T + ".cfg";
+ if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+ return readConfigFile(CfgFilePath, ExpCtx);
+ }
+
// If we were unable to find a config file deduced from executable name,
// that is not an error.
return false; Will probably need some tests. |
You can probably actually make that not Darwin specific: diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 2aaa52072b03..142b8bb6b1a8 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1147,6 +1147,34 @@ bool Driver::loadConfigFiles() {
return false;
}
+static bool findTripleConfigFile(llvm::cl::ExpansionContext &ExpCtx,
+ SmallString<128> &ConfigFilePath,
+ llvm::Triple Triple, std::string Suffix) {
+ // First, try the full unmodified triple.
+ if (ExpCtx.findConfigFile(Triple.str() + Suffix, ConfigFilePath))
+ return true;
+
+ // Don't continue if we didn't find a parsable version in the triple.
+ VersionTuple OSVersion = Triple.getOSVersion();
+ if (!OSVersion.getMinor().has_value())
+ return false;
+
+ std::string BaseOSName = Triple.getOSTypeName(Triple.getOS()).str();
+
+ // Next try strip the version to only include the major component.
+ // e.g. arm64-apple-darwin23.6.0 -> arm64-apple-darwin23
+ if (OSVersion.getMajor() != 0) {
+ Triple.setOSName(BaseOSName + llvm::utostr(OSVersion.getMajor()));
+ if (ExpCtx.findConfigFile(Triple.str() + Suffix, ConfigFilePath))
+ return true;
+ }
+
+ // Finally, try without any version suffix at all.
+ // e.g. arm64-apple-darwin23.6.0 -> arm64-apple-darwin
+ Triple.setOSName(BaseOSName);
+ return ExpCtx.findConfigFile(Triple.str() + Suffix, ConfigFilePath);
+}
+
bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
// Disable default config if CLANG_NO_DEFAULT_CONFIG is set to a non-empty
// value.
@@ -1158,7 +1186,7 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
return false;
std::string RealMode = getExecutableForDriverMode(Mode);
- std::string Triple;
+ llvm::Triple Triple;
// If name prefix is present, no --target= override was passed via CLOptions
// and the name prefix is not a valid triple, force it for backwards
@@ -1169,15 +1197,13 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
llvm::Triple PrefixTriple{ClangNameParts.TargetPrefix};
if (PrefixTriple.getArch() == llvm::Triple::UnknownArch ||
PrefixTriple.isOSUnknown())
- Triple = PrefixTriple.str();
+ Triple = PrefixTriple;
}
// Otherwise, use the real triple as used by the driver.
- if (Triple.empty()) {
- llvm::Triple RealTriple =
- computeTargetTriple(*this, TargetTriple, *CLOptions);
- Triple = RealTriple.str();
- assert(!Triple.empty());
+ if (Triple.str().empty()) {
+ Triple = computeTargetTriple(*this, TargetTriple, *CLOptions);
+ assert(!Triple.str().empty());
}
// Search for config files in the following order:
@@ -1192,21 +1218,21 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
// Try loading <triple>-<mode>.cfg, and return if we find a match.
SmallString<128> CfgFilePath;
- std::string CfgFileName = Triple + '-' + RealMode + ".cfg";
- if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+ if (findTripleConfigFile(ExpCtx, CfgFilePath, Triple,
+ "-" + RealMode + ".cfg"))
return readConfigFile(CfgFilePath, ExpCtx);
bool TryModeSuffix = !ClangNameParts.ModeSuffix.empty() &&
ClangNameParts.ModeSuffix != RealMode;
if (TryModeSuffix) {
- CfgFileName = Triple + '-' + ClangNameParts.ModeSuffix + ".cfg";
- if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+ if (findTripleConfigFile(ExpCtx, CfgFilePath, Triple,
+ "-" + ClangNameParts.ModeSuffix + ".cfg"))
return readConfigFile(CfgFilePath, ExpCtx);
}
// Try loading <mode>.cfg, and return if loading failed. If a matching file
// was not found, still proceed on to try <triple>.cfg.
- CfgFileName = RealMode + ".cfg";
+ std::string CfgFileName = RealMode + ".cfg";
if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath)) {
if (readConfigFile(CfgFilePath, ExpCtx))
return true;
@@ -1218,8 +1244,7 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
}
// Try loading <triple>.cfg and return if we find a match.
- CfgFileName = Triple + ".cfg";
- if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+ if (findTripleConfigFile(ExpCtx, CfgFilePath, Triple, ".cfg"))
return readConfigFile(CfgFilePath, ExpCtx);
// If we were unable to find a config file deduced from executable name,
diff --git a/clang/test/Driver/config-file3.c b/clang/test/Driver/config-file3.c
index a0b8062c60ce..395c31ce04b6 100644
--- a/clang/test/Driver/config-file3.c
+++ b/clang/test/Driver/config-file3.c
@@ -226,3 +226,26 @@
//
// RUN: HOME=%S/Inputs/config %clang -### --config-user-dir=~ -v 2>&1 | FileCheck %s --check-prefix=CHECK-TILDE
// CHECK-TILDE: User configuration file directory: {{.*}}/Inputs/config
+
+//--- Fallback to stripping OS versions
+//
+// RUN: touch %t/testdmode/x86_64-apple-darwin23.6.0-clang.cfg
+// RUN: touch %t/testdmode/x86_64-apple-darwin23-clang.cfg
+// RUN: touch %t/testdmode/x86_64-apple-darwin-clang.cfg
+// RUN: %clang -target x86_64-apple-darwin23.6.0 --config-system-dir=%t/testdmode --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix DARWIN --implicit-check-not 'Configuration file:'
+//
+// DARWIN: Configuration file: {{.*}}/testdmode/x86_64-apple-darwin23.6.0-clang.cfg
+
+//--- DARWIN + no full version
+//
+// RUN: rm %t/testdmode/x86_64-apple-darwin23.6.0-clang.cfg
+// RUN: %clang -target x86_64-apple-darwin23.6.0 --config-system-dir=%t/testdmode --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix DARWIN-MAJOR --implicit-check-not 'Configuration file:'
+//
+// DARWIN-MAJOR: Configuration file: {{.*}}/testdmode/x86_64-apple-darwin23-clang.cfg
+
+//--- DARWIN + no version
+//
+// RUN: rm %t/testdmode/x86_64-apple-darwin23-clang.cfg
+// RUN: %clang -target x86_64-apple-darwin23.6.0 --config-system-dir=%t/testdmode --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix DARWIN-VERSIONLESS --implicit-check-not 'Configuration file:'
+//
+// DARWIN-VERSIONLESS: Configuration file: {{.*}}/testdmode/x86_64-apple-darwin-clang.cfg |
Just saw this, but I've already opened a PR. Feel free to send that upstream and I can close my PR. |
Might also be nicer to pass |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Moved to #196094. |
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?Adding a system config file will allow us to stop baking in the
DEFAULT_SYSROOT
and allow users on Xcode-only installations to use thebottles. It will also make it easier to update this value when users
upgrade the major version of their macOS. Before this change, this
required a full reinstall. After, users will simply have to
brew postinstall llvm
.The config file change requires an upstream patch. We'll probably have
to wait a little bit for it to merge, and then wait another few days to
make sure it doesn't get reverted.
Following discussion at #192505, let's also update the caveats to notify
users of potential changes regarding
LLVM_ENABLE_EH
in LLVM 20.Marked as syntax-only for now to avoid duplicating the build at #192505.