[cmake-developers] [PATCH for bug 15253] Ninja: Pass only the flags relevant to the language.

[Brian Smith]

On Wed, Jan 28, 2015 at 5:47 AM, Brad King <brad.king at kitware.com> wrote:
> On 01/28/2015 07:31 AM, Brian Smith wrote:
>> -  std::string flags = "$FLAGS";
>> +  std::string flags;
>> +
>> +  if (lang == "C" || lang == "CXX" || lang == "RC")
>> +    {
>> +    flags += "$FLAGS";
>> +    }
>
> Why is this hunk needed?  The $FLAGS placeholder should be
> okay for any language because the corresponding build rules
> should be generated with the proper FLAGS value.

You are probably right. In the BoringSSL build, $FLAGS consists of a
bunch of "-I <dir>" arguments that specify the paths to the C/C++
include directories. BoringSSL's assembly language code doesn't use
".include" so these arguments are unnecessary, and it seemed wrong to
me to inherit the C/C++ include directories for assembly language
compilations, so I removed it. But, this change doesn't have any
bearing on the problem described in the bug, so it is probably better
to revert it.

>>      else
>>        {
>>        deptype = "msvc";
>>        depfile = "";
>> -      flags += " /showIncludes";
>> +      if (lang == "C" || lang == "CXX")
>> +        {
>> +        flags += " /showIncludes";
>> +        }
>>        }
>
> Without /showIncludes the "msvc" deptype does not make sense.
> Should some other code path be used here to get dependencies?

Good point. Again, BoringSSL's assembly language code doesn't use
".include" so I'm not sure what is correct. However, both the nasm
documentation and the yasm documentation say that "-M" should be used
to generate makefile dependencies.

IMO, it is a good idea to fix this bug by committing a variant of the
second hunk, and then file a new bug about correctly getting
dependencies from nasm/yasm. If you agree, I'm happy to send an
updated patch that removes the first hunk and that changes the deptype
line in the second hunk to something else. In that case, what should I
change the deptype line to?

Thanks,
Brian