| Message ID | 20210806080634.11869-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f6bb2099bf0f982bd3d43fe479b8272d5bf18a6a |
| Headers | show |
| Series | makefile: update detect-compiler for newer Xcode version | expand |
On 06/08/21 15.06, Carlo Marcelo Arenas Belón wrote: > -"Apple LLVM") > +"Apple LLVM"|"Apple clang") > print_flags clang Why not just s/Apple LLVM/Apple clang/?
On Fri, Aug 6, 2021 at 5:00 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > Why not just s/Apple LLVM/Apple clang/? because that would break it for older versions of Xcode that reported themselves as "Apple LLVM" and I am not even sure when the change was made to even consider those versions as obsolete. Either way, I am sure once we figure out which versions are affected and which versions macOS developers care enough for, that would be the obvious next step. note that DEVELOPER=1 was broken for a while in macOS and that also affects CI, and it is still known to be suboptimal, since version numbers for Xcode compilers are not always representative of what features can be found in the opensource version with similar numbers. Carlo Carlo
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 1da1580e4c (Makefile: detect compiler and enable more warnings in > DEVELOPER=1, 2018-04-14) uses the output of the compiler banner to > detect the compiler family. > > Apple had since changed the wording used to refer to its compiler > as clang instead of LLVM as shown by: > > $ cc --version > Apple clang version 12.0.5 (clang-1205.0.22.9) > Target: x86_64-apple-darwin20.6.0 > Thread model: posix > InstalledDir: /Library/Developer/CommandLineTools/usr/bin > > so update the script to match, and allow DEVELOPER=1 to work as > expected again in macOS. Thanks for submitting this enhancement! For those of us using Homebrew and using the LLVM installation from there, we get: $ cc --version Homebrew clang version 12.0.1 Target: arm64-apple-darwin20.5.0 Thread model: posix InstalledDir: /opt/homebrew/opt/llvm/bin > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > detect-compiler | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/detect-compiler b/detect-compiler > index 70b754481c..c85be83c64 100755 > --- a/detect-compiler > +++ b/detect-compiler > @@ -44,7 +44,7 @@ clang) > "FreeBSD clang") > print_flags clang > ;; > -"Apple LLVM") > +"Apple LLVM"|"Apple clang") > print_flags clang > ;; > *) So maybe we could add another case for "Homebrew clang"? --- Atharva Raykar ಅಥರ್ವ ರಾಯ್ಕರ್ अथर्व रायकर
On Fri, Aug 6, 2021 at 9:33 AM Carlo Arenas <carenas@gmail.com> wrote: > On Fri, Aug 6, 2021 at 5:00 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > Why not just s/Apple LLVM/Apple clang/? > > because that would break it for older versions of Xcode that reported > themselves as "Apple LLVM" and I am not even sure when the change was > made to even consider those versions as obsolete. > > Either way, I am sure once we figure out which versions are affected > and which versions macOS developers care enough for, that would be the > obvious next step. My daily development machine still reports "Apple LLVM", so it would indeed be premature to `s/Apple LLVM/Apple clang/`. Moreover, there's simply no good reason to `s/Apple LLVM/Apple clang/` since it's not a maintenance burden to leave "Apple LLVM" in the match pattern.
Atharva Raykar <raykar.ath@gmail.com> writes: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > For those of us using Homebrew and using the LLVM installation from > there, we get: > > $ cc --version > Homebrew clang version 12.0.1 > ... >> diff --git a/detect-compiler b/detect-compiler >> index 70b754481c..c85be83c64 100755 >> --- a/detect-compiler >> +++ b/detect-compiler >> @@ -44,7 +44,7 @@ clang) >> "FreeBSD clang") >> print_flags clang >> ;; >> -"Apple LLVM") >> +"Apple LLVM"|"Apple clang") >> print_flags clang >> ;; >> *) > > So maybe we could add another case for "Homebrew clang"? $ clang --version 2>&1 | sed -ne 's/ version .*//p' Debian clang It might be necessary to cope with this "$VENDOR clang version" convention better with something like the following. I am afraid that this patch is being a bit too aggressive about LLVM, as I do not know if "$VENDOR LLVM version" is also a thing, or it is just oddity only at Apple, though. diff --git c/detect-compiler w/detect-compiler index 70b754481c..a80442a327 100755 --- c/detect-compiler +++ w/detect-compiler @@ -38,13 +38,7 @@ case "$(get_family)" in gcc) print_flags gcc ;; -clang) - print_flags clang - ;; -"FreeBSD clang") - print_flags clang - ;; -"Apple LLVM") +clang | *" clang" | *" LLVM") print_flags clang ;; *)
On Fri, Aug 06, 2021 at 11:11:31AM -0700, Junio C Hamano wrote: > > So maybe we could add another case for "Homebrew clang"? > > $ clang --version 2>&1 | sed -ne 's/ version .*//p' > Debian clang > > It might be necessary to cope with this "$VENDOR clang version" > convention better with something like the following. Good catch. Unfortunately your patch below isn't sufficient because get_family() is broken, too. :( It insists on there being an extra word after the actual version. There is for gcc, but not for clang on my system: $ CC=gcc get_version_line gcc version 10.2.1 20210110 (Debian 10.2.1-6) $ CC=clang get_version_line Debian clang version 11.0.1-2 Doing this on top of your patch makes "./detect-compiler clang" behave as expected: diff --git a/detect-compiler b/detect-compiler index a80442a327..fd388ae783 100755 --- a/detect-compiler +++ b/detect-compiler @@ -13,11 +13,11 @@ get_version_line() { } get_family() { - get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/' + get_version_line | sed 's/^\(.*\) version [0-9].*/\1/' } get_version() { - get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/' + get_version_line | sed 's/^.* version \([0-9][^ ]*\).*/\1/' } print_flags() { > I am afraid that this patch is being a bit too aggressive about > LLVM, as I do not know if "$VENDOR LLVM version" is also a thing, or > it is just oddity only at Apple, though. I think even before this issue, all of the versioning of Apple's clang is suspect. The "Apple LLVM" bit comes from Eric in: https://lore.kernel.org/git/20180318090607.GA26226@flurp.local/ But downthread we realized that the version numbers there don't match the clang ones: https://lore.kernel.org/git/CAPig+cRQXQ_DowS2Dsc1x3TAGJjnWig7P4eYS4kQ+C2piAdSWA@mail.gmail.com/ Duy indicated in the cover letter: https://lore.kernel.org/git/20180324125348.6614-1-pclouds@gmail.com/ that the apple support was probably wrong, but it looks like nobody stepped up in the meantime to fix it. In practice I think it mostly works anyway because "clang4" is the only version check we have for clang. So if we err "ahead" a few versions (which is what Apple's scheme does), you only get bit if you have a pretty old version of Xcode. I think if we wanted to get it right, we'd need to encode into the script some form of the version table here: https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_-_13.x_%28since_SwiftUI_framework%29 -Peff
diff --git a/detect-compiler b/detect-compiler index 70b754481c..c85be83c64 100755 --- a/detect-compiler +++ b/detect-compiler @@ -44,7 +44,7 @@ clang) "FreeBSD clang") print_flags clang ;; -"Apple LLVM") +"Apple LLVM"|"Apple clang") print_flags clang ;; *)
1da1580e4c (Makefile: detect compiler and enable more warnings in DEVELOPER=1, 2018-04-14) uses the output of the compiler banner to detect the compiler family. Apple had since changed the wording used to refer to its compiler as clang instead of LLVM as shown by: $ cc --version Apple clang version 12.0.5 (clang-1205.0.22.9) Target: x86_64-apple-darwin20.6.0 Thread model: posix InstalledDir: /Library/Developer/CommandLineTools/usr/bin so update the script to match, and allow DEVELOPER=1 to work as expected again in macOS. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- detect-compiler | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)