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

Bob Feng bob.c.feng at intel.com
Thu May 9 04:02:17 UTC 2019 

I agree with your point. I'll not push this patch to edk2 master.

So far I've seen there is only one file with extension ".C" in one platform package. I've asked the owner to change it to lower case ".c".

I enter an BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1790 to clean up the ".C" support in BaseTools.

Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek [mailto:lersek at redhat.com] 
Sent: Thursday, May 9, 2019 3:50 AM
To: Leif Lindholm <leif.lindholm at linaro.org>; Fan, ZhijuX <zhijux.fan at intel.com>
Cc: Andrew Fish <afish at apple.com>; devel at edk2.groups.io; Gao, Liming <liming.gao at intel.com>; Feng, Bob C <bob.c.feng at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>
Subject: Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes

On 05/08/19 14:08, Leif Lindholm 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?

I agree -- on any case-sensitive filesystem, the ".C" suffix implies the
C++ language. (Minimally for gcc, not sure about the rest.)

In the first place, we should never *have* a file in the edk2 tree that ends with the upper-case ".C" suffix, due to the above. (C++ is not a supported language in edk2.) And so we don't need any BaseTools support for ".C" either.

If an external platform of Packages tree contains ".C" files, then those files should be renamed to ".c", in my opinion. (Sooner or later someone will try to build that tree with gcc or clang, and then stuff will break
hard.)

Just my 2 cents, since I was asked.

Thanks
Laszlo

> 
> 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 at linaro.org]
>>> Sent: Wednesday, May 8, 2019 5:09 PM
>>> To: Andrew Fish <afish at apple.com>
>>> Cc: devel at edk2.groups.io; Fan, ZhijuX <zhijux.fan at intel.com>; Gao, 
>>> Liming <liming.gao at intel.com>; Feng, Bob C <bob.c.feng at 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 at 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_r
>>> ul
>>>> 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 at apple.com [mailto:afish at apple.com]
>>>>>> Sent: Tuesday, May 7, 2019 10:31 AM
>>>>>> To: devel at edk2.groups.io; Fan, ZhijuX <zhijux.fan at intel.com>
>>>>>> Cc: Gao, Liming <liming.gao at intel.com>; Feng, Bob C 
>>>>>> <bob.c.feng at 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 at 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 at intel.com>
>>>>>>> Cc: Liming Gao <liming.gao at intel.com>
>>>>>>> Signed-off-by: Zhiju.Fan <zhijux.fan at 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>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> 
>>>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#40243): https://edk2.groups.io/g/devel/message/40243
Mute This Topic: https://groups.io/mt/31539232/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

More information about the edk2-devel-archive mailing list