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

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Wed, May 08, 2019 at 06:59:40AM -0700, Andrew Fish wrote:
> 
> 
> > 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
> 

So did I :)
Still...

> 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?

Yeah, that would work for me.

It's just I've had a few instances of people moving existing drivers
developed with VS (for x86) to GCC and run across issues - so getting a
warning would at least convey the fact that there *is* a portability
issue here.

Regards,

Leif

> 
> 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>
> >>>>>> 
> >>>>>> 
> >>>>>> 
> >>>>>> 
> >>>>> 
> >>>>> 
> >>>> 
> > 
> > 
>