[v2,10/10] detect-compiler: detect clang even if it found CUDA

Commit Message

Johannes Schindelin March 25, 2025, 11:32 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In my setup, clang finds `/usr/local/cuda` and hence the output of
`clang -v` ends with this line:

	Found CUDA installation: /usr/local/cuda, version

This confuses the `detect-compiler` script because it matches _all_
lines that contain the needle "version" surrounded by spaces. As a
consequence, the `get_family` function returns two lines: "Ubuntu clang"
and above-mentioned line, which the `case` statement does not handle
well and hence reports "unknown compiler family" instead of the expected
set of "clang14", "clang13", ..., "clang1" output.

Let's unconfuse the script by letting it parse the first matching line
and ignore the rest.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 detect-compiler | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King March 26, 2025, 5:41 p.m. UTC | #1

On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:

> In my setup, clang finds `/usr/local/cuda` and hence the output of
> `clang -v` ends with this line:
> 
> 	Found CUDA installation: /usr/local/cuda, version
> 
> This confuses the `detect-compiler` script because it matches _all_
> lines that contain the needle "version" surrounded by spaces. As a
> consequence, the `get_family` function returns two lines: "Ubuntu clang"
> and above-mentioned line, which the `case` statement does not handle
> well and hence reports "unknown compiler family" instead of the expected
> set of "clang14", "clang13", ..., "clang1" output.
> 
> Let's unconfuse the script by letting it parse the first matching line
> and ignore the rest.

Makes sense. I wondered if this:

>  get_version_line() {
> -	LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> +	LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'

might be more readable with "grep -m1", but it looks like "-m" is not in
POSIX. So what you wrote is probably safer.

-Peff

Eric Sunshine March 26, 2025, 6:07 p.m. UTC | #2

On Wed, Mar 26, 2025 at 1:44 PM Jeff King <[e-mail]> wrote:
> On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > Let's unconfuse the script by letting it parse the first matching line
> > and ignore the rest.
>
> Makes sense. I wondered if this:
>
> >  get_version_line() {
> > -     LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> > +     LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
>
> might be more readable with "grep -m1", but it looks like "-m" is not in
> POSIX. So what you wrote is probably safer.

It's probably an indication that I've done too much `sed` programming,
but I find Dscho's version more obvious. That aside, your response
made me take a closer look at what Dscho wrote and I noticed that it
is syntactically flawed, at least for BSD-lineage `sed`. Testing on
macOS reveals that this is indeed so:

    % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
    sed: 1: "/ version /{p;q}": extra characters at the end of q command

The problem is that the `q` function takes no arguments, but
BSD-lineage `sed` thinks that the closing `}` is an argument rather
than a terminator. Fixing this requires inserting a terminator after
`q`, which will be either a newline character or a semicolon. So, the
correct form is:

    sed -n '/ version /{p;q;}

Jeff King March 27, 2025, 5:14 a.m. UTC | #3

On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:

> On Wed, Mar 26, 2025 at 1:44 PM Jeff King <[e-mail]> wrote:
> > On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > Let's unconfuse the script by letting it parse the first matching line
> > > and ignore the rest.
> >
> > Makes sense. I wondered if this:
> >
> > >  get_version_line() {
> > > -     LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> > > +     LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
> >
> > might be more readable with "grep -m1", but it looks like "-m" is not in
> > POSIX. So what you wrote is probably safer.
> 
> It's probably an indication that I've done too much `sed` programming,
> but I find Dscho's version more obvious. That aside, your response
> made me take a closer look at what Dscho wrote and I noticed that it
> is syntactically flawed, at least for BSD-lineage `sed`. Testing on
> macOS reveals that this is indeed so:
> 
>     % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
>     sed: 1: "/ version /{p;q}": extra characters at the end of q command
> 
> The problem is that the `q` function takes no arguments, but
> BSD-lineage `sed` thinks that the closing `}` is an argument rather
> than a terminator. Fixing this requires inserting a terminator after
> `q`, which will be either a newline character or a semicolon. So, the
> correct form is:
> 
>     sed -n '/ version /{p;q;}

Heh, I think it was the braces and semicolons that made my spider-sense
tingle, probably because I've been bitten by those subtleties in the
past.

I think just "/foo/p;q" works on GNU sed, but no idea if it does
elsewhere. What you wrote seems the safest.

-Peff

Eric Sunshine March 27, 2025, 5:21 a.m. UTC | #4

On Thu, Mar 27, 2025 at 1:14 AM Jeff King <[e-mail]> wrote:
> On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:
> > It's probably an indication that I've done too much `sed` programming,
> > but I find Dscho's version more obvious. That aside, your response
> > made me take a closer look at what Dscho wrote and I noticed that it
> > is syntactically flawed, at least for BSD-lineage `sed`. Testing on
> > macOS reveals that this is indeed so:
> >
> >     % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
> >     sed: 1: "/ version /{p;q}": extra characters at the end of q command
> >
> > The problem is that the `q` function takes no arguments, but
> > BSD-lineage `sed` thinks that the closing `}` is an argument rather
> > than a terminator. Fixing this requires inserting a terminator after
> > `q`, which will be either a newline character or a semicolon. So, the
> > correct form is:
> >
> >     sed -n '/ version /{p;q;}
>
> Heh, I think it was the braces and semicolons that made my spider-sense
> tingle, probably because I've been bitten by those subtleties in the
> past.
>
> I think just "/foo/p;q" works on GNU sed, but no idea if it does
> elsewhere. What you wrote seems the safest.

That's not quite the same, though. The patternless `q` will cause
`sed` to terminate upon reading the first line of input, not upon the
first line which contains " version ". This matters, for instance, if
the first line output by `$CC -v` is not the version string (i.e. it
might be a copyright notice).

Jeff King March 27, 2025, 6:35 a.m. UTC | #5

On Thu, Mar 27, 2025 at 01:21:03AM -0400, Eric Sunshine wrote:

> > I think just "/foo/p;q" works on GNU sed, but no idea if it does
> > elsewhere. What you wrote seems the safest.
> 
> That's not quite the same, though. The patternless `q` will cause
> `sed` to terminate upon reading the first line of input, not upon the
> first line which contains " version ". This matters, for instance, if
> the first line output by `$CC -v` is not the version string (i.e. it
> might be a copyright notice).

Oh, duh, you're right. I even tested to make sure were still quitting
after matching, but of course that is because we were quitting even when
_not_ matching.

For some reason I also thought /foo/pq would work, but it doesn't. I
wonder if it used to under some implementations, or if I'm simply going
senile.

(The point still remains that what you wrote is the correct thing).

-Peff

Patch

diff --git a/detect-compiler b/detect-compiler
index a87650b71bb..01eca3a781d 100755
--- a/detect-compiler
+++ b/detect-compiler
@@ -9,7 +9,7 @@  CC="$*"
 #
 # FreeBSD clang version 3.4.1 (tags/RELEASE...)
 get_version_line() {
-	LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
+	LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
 }
 
 get_family() {