Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 6750 bytes --]



> On May 8, 2019, at 5:08 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 
> Hi Fan Zhiju,
> 
> I understand your reasoning, but that strengthens my view that we
> should leave this change out.
> 
> Actually, could we consider *dropping* support for .C files
> altogether, since we are striving to support multiple toolchains, and
> the two major families of toolchains we use consider .C files to be
> fundamentally different things?
> 
> Andrew, Laszlo, Mike?
> 

Leif,

I missed that ARM had extra restrictions on file extensions. 

[C-Code-File]
    <InputFile>
        ?.c
        ?.C
        ?.cc
        ?.CC
        ?.cpp
        ?.Cpp
        ?.CPP

[C-Code-File.BASE.AARCH64,C-Code-File.SEC.AARCH64,C-Code-File.PEI_CORE.AARCH64,C-Code-File.PEIM.AARCH64,C-Code-File.BASE.ARM,C-Code-File.SEC.ARM,C-Code-File.PEI_CORE.ARM,C-Code-File.PEIM.ARM]
    <InputFile>
        ?.c

I think there are people who try to roll there own C++ support  and that is why the build system will pass the C++ files to the C compiler. 

At this point I think we should probably act more like a compiler. First add a warning, and down the road remove the support?

Thanks,

Andrew Fish

> Best Regards,
> 
> Leif
> 
> On Wed, May 08, 2019 at 10:57:09AM +0000, Fan, ZhijuX wrote:
>> Hi Leif,
>> 
>> This patch is going to fix the bug in commit 05217d210e.
>> this patch and commit 05217d210e is just for MSVC. It doesn't have any effect on GCC.
>> this patch does not imply to compile .C as C instead of C++.
>> We just found out that they build break because the.C file was in the source file,
>> But the MSVC compiler allows.C files,So I fixed this bug to Change the original logic to support.C files.
>> 
>> 
>> Any question, please let me know. Thanks.
>> 
>> Best Regards
>> Fan Zhiju
>> 
>> 
>> 
>> 
>>> -----Original Message-----
>>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>>> Sent: Wednesday, May 8, 2019 5:09 PM
>>> To: Andrew Fish <afish@apple.com>
>>> Cc: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>; Gao, Liming
>>> <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>
>>> Subject: Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C
>>> files with .C suffixes
>>> 
>>> On Tue, May 07, 2019 at 07:01:26PM -0700, Andrew Fish wrote:
>>>> 
>>>> 
>>>>> On May 7, 2019, at 7:40 AM, Leif Lindholm <leif.lindholm@linaro.org>
>>> wrote:
>>>>> 
>>>>> Hi Fan Zhiju,
>>>>> 
>>>>> But where does the string come from that contains a .C suffix?
>>>>> Is the tool internally converting things to uppercase, or is some
>>>>> source file in the build incorrectly named?
>>>>> 
>>>> 
>>>> Leif,
>>>> 
>>>> Our build system defines .C as correct! I think it has been that way a very
>>> long time.
>>> 
>>> .C is valid, but at least for GCC it is equivalent to all of the other non-.c
>>> options - i.e., interpret as c++. Which is why I am wondering about the case
>>> that ends up with the build system internally processing this.
>>> 
>>> If it is changed to permitting .C files to be compiled as C instead of
>>> C++ (which the patch seems to imply), that sounds incorrect to me.
>>> 
>>> /
>>>    Leif
>>> 
>>>> 
>>> https://github.com/tianocore/edk2/blob/master/BaseTools/Conf/build_rul
>>>> e.template#L109
>>>> 
>>>> [C-Code-File]
>>>>    <InputFile>
>>>>        ?.c
>>>>        ?.C
>>>>        ?.cc
>>>>        ?.CC
>>>>        ?.cpp
>>>>        ?.Cpp
>>>>        ?.CPP
>>>> 
>>>> Thanks,
>>>> 
>>>> Andrew Fish
>>>> 
>>>> 
>>>>> I am asking because it is not clear to me whether the patch resolves
>>>>> a problem or hides one.
>>>>> 
>>>>> Best Regards,
>>>>> 
>>>>> Leif
>>>>> 
>>>>> On Tue, May 07, 2019 at 03:05:02AM +0000, Fan, ZhijuX wrote:
>>>>>> This problem has nothing to do with the file system, We just use
>>>>>> the filename as a string to compare with other strings Our unittest
>>>>>> tested minplatform, Ovmf. This problem was found when building a
>>>>>> platform inside Intel.
>>>>>> We've tested it on Linux and Windows.
>>>>>> 
>>>>>> Any question, please let me know. Thanks.
>>>>>> 
>>>>>> Best Regards
>>>>>> Fan Zhiju
>>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: afish@apple.com [mailto:afish@apple.com]
>>>>>> Sent: Tuesday, May 7, 2019 10:31 AM
>>>>>> To: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>
>>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Feng, Bob C
>>>>>> <bob.c.feng@intel.com>
>>>>>> Subject: Re: [edk2-devel] [PATCH V2] BaseTools:improve code to
>>>>>> support C files with .C suffixes
>>>>>> 
>>>>>> This brings up a question? Do we tests on a file system that is case
>>> sensitive? Is this just lack of a test case?
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>>> Andrew Fish
>>>>>> 
>>>>>>> On May 6, 2019, at 7:22 PM, Fan, ZhijuX <zhijux.fan@intel.com> wrote:
>>>>>>> 
>>>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1773
>>>>>>> 
>>>>>>> Build break if C file suffixes of named .C instead of .c Code not
>>>>>>> recognize filenames with .C suffixes.
>>>>>>> 
>>>>>>> This patch adds code to Support both .c file and .C file
>>>>>>> 
>>>>>>> Cc: Bob Feng <bob.c.feng@intel.com>
>>>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>>>> Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
>>>>>>> ---
>>>>>>> BaseTools/Source/Python/AutoGen/GenMake.py | 3 ++-
>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>> 
>>>>>>> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
>>>>>>> b/BaseTools/Source/Python/AutoGen/GenMake.py
>>>>>>> index 0e0f9fd9b0..858ddedf8e 100644
>>>>>>> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
>>>>>>> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
>>>>>>> @@ -1035,7 +1035,8 @@ cleanlib:
>>>>>>>                       CmdTargetDict[CmdSign] = "%s %s" %
>>> (CmdTargetDict[CmdSign], SingleCommandList[-1])
>>>>>>>                   Index = CommandList.index(Item)
>>>>>>>                   CommandList.pop(Index)
>>>>>>> -                    if SingleCommandList[-1].endswith("%s%s.c" % (TAB_SLASH,
>>> CmdSumDict[CmdSign.lstrip('/Fo').rsplit(TAB_SLASH, 1)[0]])):
>>>>>>> +                    if SingleCommandList[-1].endswith("%s%s.c" %
>>> (TAB_SLASH, CmdSumDict[T.Target.SubDir])) or \
>>>>>>> +                            SingleCommandList[-1].endswith("%s%s.C" %
>>> (TAB_SLASH, CmdSumDict[T.Target.SubDir])):
>>>>>>>                       Cpplist = CmdCppDict[T.Target.SubDir]
>>>>>>>                       Cpplist.insert(0, '$(OBJLIST_%d): $(COMMON_DEPS)' %
>>> list(self.ObjTargetDict.keys()).index(T.Target.SubDir))
>>>>>>>                       T.Commands[Index] = '%s\n\t%s' % ('
>>>>>>> \\\n\t'.join(Cpplist), CmdTargetDict[CmdSign])
>>>>>>> --
>>>>>>> 2.14.1.windows.1
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> <winmail.dat>
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>> 
>>>>> 
>>>> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 23128 bytes --]