public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH V2] BaseTools:improve code to support C files with .C suffixes
@ 2019-05-07  2:22 Fan, ZhijuX
  2019-05-07  2:31 ` [edk2-devel] " Andrew Fish
  0 siblings, 1 reply; 13+ messages in thread
From: Fan, ZhijuX @ 2019-05-07  2:22 UTC (permalink / raw)
  To: devel@edk2.groups.io; +Cc: Gao, Liming, Feng, Bob C

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

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


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4050 bytes --]

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-07  2:22 [PATCH V2] BaseTools:improve code to support C files with .C suffixes Fan, ZhijuX
@ 2019-05-07  2:31 ` Andrew Fish
  2019-05-07  3:05   ` FW: " Fan, ZhijuX
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Fish @ 2019-05-07  2:31 UTC (permalink / raw)
  To: devel, zhijux.fan; +Cc: Gao, Liming, Feng, Bob C

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>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* FW: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-07  2:31 ` [edk2-devel] " Andrew Fish
@ 2019-05-07  3:05   ` Fan, ZhijuX
  2019-05-07 14:40     ` Leif Lindholm
  0 siblings, 1 reply; 13+ messages in thread
From: Fan, ZhijuX @ 2019-05-07  3:05 UTC (permalink / raw)
  To: devel@edk2.groups.io, afish@apple.com
  Cc: Gao, Liming, Feng, Bob C, Fan, ZhijuX

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>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: FW: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-07  3:05   ` FW: " Fan, ZhijuX
@ 2019-05-07 14:40     ` Leif Lindholm
  2019-05-08  1:57       ` Fan, ZhijuX
  2019-05-08  2:01       ` Andrew Fish
  0 siblings, 2 replies; 13+ messages in thread
From: Leif Lindholm @ 2019-05-07 14:40 UTC (permalink / raw)
  To: devel, zhijux.fan; +Cc: afish@apple.com, Gao, Liming, Feng, Bob C

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?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: FW: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-07 14:40     ` Leif Lindholm
@ 2019-05-08  1:57       ` Fan, ZhijuX
  2019-05-08  2:01       ` Andrew Fish
  1 sibling, 0 replies; 13+ messages in thread
From: Fan, ZhijuX @ 2019-05-08  1:57 UTC (permalink / raw)
  To: devel@edk2.groups.io, leif.lindholm@linaro.org
  Cc: afish@apple.com, Gao, Liming, Feng, Bob C, Fan, ZhijuX

Hi Leif,

In AutoGen\GenMake.py.   It's around line 917
if File.Ext not in [".c", ".C"] or File.Name == "AutoGen.c"
there is similar code to check the .C file.
The 'File' above refers to the source File.Some source files are named.C


Any question, please let me know. Thanks.

Best Regards
Fan Zhiju



> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, May 7, 2019 10:40 PM
> To: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>
> Cc: afish@apple.com; Gao, Liming <liming.gao@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>
> Subject: Re: FW: [edk2-devel] [PATCH V2] BaseTools:improve code to
> support C files with .C suffixes
> 
> 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?
> 
> 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>
> >
> >
> >
> >
> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-07 14:40     ` Leif Lindholm
  2019-05-08  1:57       ` Fan, ZhijuX
@ 2019-05-08  2:01       ` Andrew Fish
  2019-05-08  9:09         ` Leif Lindholm
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Fish @ 2019-05-08  2:01 UTC (permalink / raw)
  To: devel, Leif Lindholm; +Cc: zhijux.fan, Gao, Liming, Feng, Bob C

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



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

https://github.com/tianocore/edk2/blob/master/BaseTools/Conf/build_rule.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: 24464 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08  2:01       ` Andrew Fish
@ 2019-05-08  9:09         ` Leif Lindholm
  2019-05-08 10:57           ` Fan, ZhijuX
  0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2019-05-08  9:09 UTC (permalink / raw)
  To: Andrew Fish; +Cc: devel, zhijux.fan, Gao, Liming, Feng, Bob C

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08  9:09         ` Leif Lindholm
@ 2019-05-08 10:57           ` Fan, ZhijuX
  2019-05-08 12:08             ` Leif Lindholm
  0 siblings, 1 reply; 13+ messages in thread
From: Fan, ZhijuX @ 2019-05-08 10:57 UTC (permalink / raw)
  To: Leif Lindholm, Andrew Fish; +Cc: devel@edk2.groups.io, Gao, Liming, Feng, Bob C

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08 10:57           ` Fan, ZhijuX
@ 2019-05-08 12:08             ` Leif Lindholm
  2019-05-08 13:59               ` Andrew Fish
  2019-05-08 19:50               ` Laszlo Ersek
  0 siblings, 2 replies; 13+ messages in thread
From: Leif Lindholm @ 2019-05-08 12:08 UTC (permalink / raw)
  To: Fan, ZhijuX
  Cc: Andrew Fish, devel@edk2.groups.io, Gao, Liming, Feng, Bob C,
	Laszlo Ersek, Michael D Kinney

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?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08 12:08             ` Leif Lindholm
@ 2019-05-08 13:59               ` Andrew Fish
  2019-05-08 14:26                 ` Leif Lindholm
  2019-05-08 19:50               ` Laszlo Ersek
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Fish @ 2019-05-08 13:59 UTC (permalink / raw)
  To: devel, Leif Lindholm
  Cc: Fan, ZhijuX, Gao, Liming, Feng, Bob C, Laszlo Ersek, Mike Kinney

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08 13:59               ` Andrew Fish
@ 2019-05-08 14:26                 ` Leif Lindholm
  0 siblings, 0 replies; 13+ messages in thread
From: Leif Lindholm @ 2019-05-08 14:26 UTC (permalink / raw)
  To: Andrew Fish
  Cc: devel, Fan, ZhijuX, Gao, Liming, Feng, Bob C, Laszlo Ersek,
	Mike Kinney

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08 12:08             ` Leif Lindholm
  2019-05-08 13:59               ` Andrew Fish
@ 2019-05-08 19:50               ` Laszlo Ersek
  2019-05-09  4:02                 ` Bob Feng
  1 sibling, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2019-05-08 19:50 UTC (permalink / raw)
  To: Leif Lindholm, Fan, ZhijuX
  Cc: Andrew Fish, devel@edk2.groups.io, Gao, Liming, Feng, Bob C,
	Michael D Kinney

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH V2] BaseTools:improve code to support C files with .C suffixes
  2019-05-08 19:50               ` Laszlo Ersek
@ 2019-05-09  4:02                 ` Bob Feng
  0 siblings, 0 replies; 13+ messages in thread
From: Bob Feng @ 2019-05-09  4:02 UTC (permalink / raw)
  To: Laszlo Ersek, Leif Lindholm, Fan, ZhijuX
  Cc: Andrew Fish, devel@edk2.groups.io, Gao, Liming, Kinney, Michael D

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@redhat.com] 
Sent: Thursday, May 9, 2019 3:50 AM
To: Leif Lindholm <leif.lindholm@linaro.org>; Fan, ZhijuX <zhijux.fan@intel.com>
Cc: Andrew Fish <afish@apple.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Kinney, Michael D <michael.d.kinney@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@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_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@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>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> 
>>>>


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-05-09  4:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-07  2:22 [PATCH V2] BaseTools:improve code to support C files with .C suffixes Fan, ZhijuX
2019-05-07  2:31 ` [edk2-devel] " Andrew Fish
2019-05-07  3:05   ` FW: " Fan, ZhijuX
2019-05-07 14:40     ` Leif Lindholm
2019-05-08  1:57       ` Fan, ZhijuX
2019-05-08  2:01       ` Andrew Fish
2019-05-08  9:09         ` Leif Lindholm
2019-05-08 10:57           ` Fan, ZhijuX
2019-05-08 12:08             ` Leif Lindholm
2019-05-08 13:59               ` Andrew Fish
2019-05-08 14:26                 ` Leif Lindholm
2019-05-08 19:50               ` Laszlo Ersek
2019-05-09  4:02                 ` Bob Feng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox