Re: [FFmpeg-devel] [PATCH] configure: Fix Microsoft tools detection

From: "Martin Storsjö" <martin@martin.st>
To: Kacper Michajlow <kasper93@gmail.com>
Cc: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] configure: Fix Microsoft tools detection
Date: Thu, 3 Feb 2022 13:34:18 +0200 (EET)
Message-ID: <22c9e8f-9eb4-b9b0-5613-f5bf2ae9b388@martin.st> (raw)
In-Reply-To: <CABPLASR3dvpqmVaH+9c=DaCnbx8okq4b6HtLfvqb9G0qi=jGkw@mail.gmail.com>

On Thu, 3 Feb 2022, Kacper Michajlow wrote:

> On Wed, 26 Jan 2022 at 15:00, Martin Storsjö <martin@martin.st> wrote:
>>
>> Hi,
>>
>> On Sat, 22 Jan 2022, Kacper Michajłow wrote:
>>
>>> LLVM tools print installation path upon execution. If one uses LLVM
>>> tools bundled with Microsoft Visual Studio installation, they would be
>>> incorrectly detected as Microsoft's ones.
>>>
>>> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
>>> ---
>>> configure | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> While the patch description seems to make sense, I wanted to try it out to
>> see the practical effect for myself, and I fail to observe any difference.
>>
>> Can you provide your exact configure command line you use, where it makes
>> a difference? I tried with "--cc=clang-cl --ld=lld-link --toolchain=msvc"
>> and that works just as fine before this patch.
>>
>> In particular, the commands that you adjust run "$_cc -nologo-" and grep
>> for "Microsoft" in the output of that. When I run that with clang-cl, it
>> doesn't print a string containing "Microsoft".
>>
>> // Martin
>
> Hi,
>
> Yes you are right. In case of CC it doesn't change anything. clang-cl
> prints installation dir only with `-v`. The main thing this patch
> fixes is `--ar=llvm-ar` where it is mistaken for lib.exe and used with
> wrong parameters. While fixing this I figured to make CC check also
> more strict, because at some point it could be a problem. Sync all of
> them to have same style as one that was already there

Oh, ok, with the reference to llvm-ar, I see what it fixes. Thanks! The 
reference to llvm-ar absolutely needs to be in the patch description then.

I remember that there has been some variance throughout the versions for 
exactly what MSVC prints as the identification thoughout the versions, but 
I think 'Microsoft.*Optimizing.*Compiler' should be safe.

So, the patch is ok if you equip it with a more detailed commit message.

Relatedly, I figured that it would be even more consistent to use 
--ar=llvm-lib instead of --ar=llvm-ar, when working in an MSVC style 
configuration, but we don't identify llvm-lib as the right kind of tool. 
Would you be interested in fixing that too? :-)

>> elif $_cc 2>&1 | grep -q 'Microsoft.*ARM.*Assembler'; then
>
> just for consistency...
>
> Also I noticed that latest MSVC (19.30.30709) complains about
>> cl : Command line warning D9035 : option 'nologo-' has been deprecated and will be removed in a future release
>
> But it doesn't affect anything, even if it were to be removed. Since
> banner is shown always by default, unless `-nologo`. Just a side note
> :)

Yep - I think this has been an attempt to make sure it does get printed, 
even if someone passes e.g. "--cc='cl -nologo'". As long as it doesn't 
break, I guess it's no rush to change this.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".