public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH edk2-InfSpecification] Drop statement on package ordering
@ 2020-05-29 14:02 Leif Lindholm
  2020-05-31 22:19 ` Michael D Kinney
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Leif Lindholm @ 2020-05-29 14:02 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Andrew Fish, Laszlo Ersek, Pankaj Bansal

The description of [Packages] sections stated that
"Packages must be listed in the order that may be required for specifying
 include path statements for a compiler. For example, the
 MdePkg/MdePkg.dec file must be listed before the
 MdeModulePkg/MdeModulePkg.dec file."

Drop it.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---

Surely this isn't something we take seriously?
If there is a measurable performance impact to the order of -I option
on the compiler command line, we should approach this programmatically.

 3_edk_ii_inf_file_format/37_[packages]_sections.md | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/3_edk_ii_inf_file_format/37_[packages]_sections.md b/3_edk_ii_inf_file_format/37_[packages]_sections.md
index 17a8d91..c09112b 100644
--- a/3_edk_ii_inf_file_format/37_[packages]_sections.md
+++ b/3_edk_ii_inf_file_format/37_[packages]_sections.md
@@ -42,11 +42,8 @@ Defines the `[Packages]` section tag that is used in EDK II module INF files.
 Each entry in this section contains a directory name, forward slash character
 and the name of the DEC file contained in the directory name.
 
-Packages must be listed in the order that may be required for specifying
-include path statements for a compiler. For example, the _MdePkg/MdePkg.dec_
-file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file. If there
-are PCDs listed in the generated "As Built" INF, the packages that declare any
-PCDs must be listed in this section.
+If there are PCDs listed in the generated "As Built" INF, the packages that
+declare any PCDs must be listed in this section.
 
 Each package filename must be listed only once per section. Package filenames
 listed in architectural sections are not permitted to be listed in the common
-- 
2.20.1


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

* Re: [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-05-29 14:02 [PATCH edk2-InfSpecification] Drop statement on package ordering Leif Lindholm
@ 2020-05-31 22:19 ` Michael D Kinney
  2020-05-31 22:43   ` [edk2-devel] " Leif Lindholm
  2020-06-03  3:33 ` Andrew Fish
       [not found] ` <1614EB3F428C08F5.21938@groups.io>
  2 siblings, 1 reply; 18+ messages in thread
From: Michael D Kinney @ 2020-05-31 22:19 UTC (permalink / raw)
  To: Leif Lindholm, devel@edk2.groups.io, Kinney, Michael D
  Cc: Andrew Fish, Laszlo Ersek, Pankaj Bansal

Hi Leif,

The reason for this statement is not for performance.

It is if the same include file exists in the same path
in more than one package.  Defining this behavior makes
the build system deterministic.

There is use case where a platform package can provide
an include override of a common package and the platform
modules list the platform package before the common
package in the [Packages] section.

So deterministic build when there are include file 
name collisions and overrides are 2 reasons to keep
the currently defined behavior.

With this background, perhaps some clarification or
rewording of the spec is required?  Do you have suggestions?

Thanks,

Mike

  This is not a common use case,
but one that does apply is a platform module that wants
to use an override of a standard include file in a platform
package.

For the build system autogen stage, this statement 

Mike

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Friday, May 29, 2020 7:03 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> Andrew Fish <afish@apple.com>; Laszlo Ersek
> <lersek@redhat.com>; Pankaj Bansal
> <pankaj.bansal@oss.nxp.com>
> Subject: [PATCH edk2-InfSpecification] Drop statement
> on package ordering
> 
> The description of [Packages] sections stated that
> "Packages must be listed in the order that may be
> required for specifying
>  include path statements for a compiler. For example,
> the
>  MdePkg/MdePkg.dec file must be listed before the
>  MdeModulePkg/MdeModulePkg.dec file."
> 
> Drop it.
> 
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> 
> Surely this isn't something we take seriously?
> If there is a measurable performance impact to the
> order of -I option
> on the compiler command line, we should approach this
> programmatically.
> 
>  3_edk_ii_inf_file_format/37_[packages]_sections.md | 7
> ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git
> a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> index 17a8d91..c09112b 100644
> ---
> a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> +++
> b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> @@ -42,11 +42,8 @@ Defines the `[Packages]` section tag
> that is used in EDK II module INF files.
>  Each entry in this section contains a directory name,
> forward slash character
>  and the name of the DEC file contained in the
> directory name.
> 
> -Packages must be listed in the order that may be
> required for specifying
> -include path statements for a compiler. For example,
> the _MdePkg/MdePkg.dec_
> -file must be listed before the
> `MdeModulePkg/MdeModulePkg.dec` file. If there
> -are PCDs listed in the generated "As Built" INF, the
> packages that declare any
> -PCDs must be listed in this section.
> +If there are PCDs listed in the generated "As Built"
> INF, the packages that
> +declare any PCDs must be listed in this section.
> 
>  Each package filename must be listed only once per
> section. Package filenames
>  listed in architectural sections are not permitted to
> be listed in the common
> --
> 2.20.1


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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-05-31 22:19 ` Michael D Kinney
@ 2020-05-31 22:43   ` Leif Lindholm
  2020-06-01  3:39     ` Pankaj Bansal
  2020-06-02 13:29     ` Laszlo Ersek
  0 siblings, 2 replies; 18+ messages in thread
From: Leif Lindholm @ 2020-05-31 22:43 UTC (permalink / raw)
  To: devel, michael.d.kinney; +Cc: Andrew Fish, Laszlo Ersek, Pankaj Bansal

Hi Mike,

Ok, I'm happy to hear that.

I agree that the overriding behaviour is useful, and it would be good
to document it. The problem is that the current wording does not say
that (in a way that is useful to anyone who does not already know what
it means). And the MdePkg/MdeModulePkg example sounds positively
horrific when interpreted in this light.

Clearly, my proposed modification is not the right thing to do here.

The problem with the document implying that the order should reflect
some sort of hierarchy *apart from when explicitly overriding* is that
this is asking a human to do the thing that humans are bad at and
computers are good at. It can't scale where humans are reviewing ports
that they understand less well than the people contributing them.

I think we should find a wording that explains the behaviour instead
of explaining some potential derivative of the behaviour, as well as
providing a realistic example instead of the MdePkg/MdeModulePkg
statament.

My suggestion is to keep it simple: say something like "where there is
a need to override an include file provided by one package with one
provided by another package, know that the compiler invocation will
list the include directories in the same order as the .dec files are
listed in the .inf".

Regards,

Leif

On Sun, May 31, 2020 at 22:19:24 +0000, Michael D Kinney wrote:
> Hi Leif,
> 
> The reason for this statement is not for performance.
> 
> It is if the same include file exists in the same path
> in more than one package.  Defining this behavior makes
> the build system deterministic.
> 
> There is use case where a platform package can provide
> an include override of a common package and the platform
> modules list the platform package before the common
> package in the [Packages] section.
> 
> So deterministic build when there are include file 
> name collisions and overrides are 2 reasons to keep
> the currently defined behavior.
> 
> With this background, perhaps some clarification or
> rewording of the spec is required?  Do you have suggestions?
> 
> Thanks,
> 
> Mike
> 
>   This is not a common use case,
> but one that does apply is a platform module that wants
> to use an override of a standard include file in a platform
> package.
> 
> For the build system autogen stage, this statement 
> 
> Mike
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Friday, May 29, 2020 7:03 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> > Andrew Fish <afish@apple.com>; Laszlo Ersek
> > <lersek@redhat.com>; Pankaj Bansal
> > <pankaj.bansal@oss.nxp.com>
> > Subject: [PATCH edk2-InfSpecification] Drop statement
> > on package ordering
> > 
> > The description of [Packages] sections stated that
> > "Packages must be listed in the order that may be
> > required for specifying
> >  include path statements for a compiler. For example,
> > the
> >  MdePkg/MdePkg.dec file must be listed before the
> >  MdeModulePkg/MdeModulePkg.dec file."
> > 
> > Drop it.
> > 
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> > 
> > Surely this isn't something we take seriously?
> > If there is a measurable performance impact to the
> > order of -I option
> > on the compiler command line, we should approach this
> > programmatically.
> > 
> >  3_edk_ii_inf_file_format/37_[packages]_sections.md | 7
> > ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git
> > a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > index 17a8d91..c09112b 100644
> > ---
> > a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > +++
> > b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > @@ -42,11 +42,8 @@ Defines the `[Packages]` section tag
> > that is used in EDK II module INF files.
> >  Each entry in this section contains a directory name,
> > forward slash character
> >  and the name of the DEC file contained in the
> > directory name.
> > 
> > -Packages must be listed in the order that may be
> > required for specifying
> > -include path statements for a compiler. For example,
> > the _MdePkg/MdePkg.dec_
> > -file must be listed before the
> > `MdeModulePkg/MdeModulePkg.dec` file. If there
> > -are PCDs listed in the generated "As Built" INF, the
> > packages that declare any
> > -PCDs must be listed in this section.
> > +If there are PCDs listed in the generated "As Built"
> > INF, the packages that
> > +declare any PCDs must be listed in this section.
> > 
> >  Each package filename must be listed only once per
> > section. Package filenames
> >  listed in architectural sections are not permitted to
> > be listed in the common
> > --
> > 2.20.1
> 
> 
> 
> 

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-05-31 22:43   ` [edk2-devel] " Leif Lindholm
@ 2020-06-01  3:39     ` Pankaj Bansal
  2020-06-01  5:15       ` Michael D Kinney
  2020-06-02 13:29     ` Laszlo Ersek
  1 sibling, 1 reply; 18+ messages in thread
From: Pankaj Bansal @ 2020-06-01  3:39 UTC (permalink / raw)
  To: Leif Lindholm, devel@edk2.groups.io, michael.d.kinney@intel.com
  Cc: Andrew Fish, Laszlo Ersek, Pankaj Bansal (OSS)

Hi Mike,

This means any port of edk2, should it so wish to override the include file provided by edk2 packages (MdePkg or MdeModulePkg),
must be listed after these dec files (MdePkg.dec or MdeModulePkg.dec) in an inf file?

Regards,
Pankaj Bansal

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Monday, June 1, 2020 4:14 AM
> To: devel@edk2.groups.io; michael.d.kinney@intel.com
> Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Pankaj
> Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Subject: Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on
> package ordering
> 
> Hi Mike,
> 
> Ok, I'm happy to hear that.
> 
> I agree that the overriding behaviour is useful, and it would be good
> to document it. The problem is that the current wording does not say
> that (in a way that is useful to anyone who does not already know what
> it means). And the MdePkg/MdeModulePkg example sounds positively
> horrific when interpreted in this light.
> 
> Clearly, my proposed modification is not the right thing to do here.
> 
> The problem with the document implying that the order should reflect
> some sort of hierarchy *apart from when explicitly overriding* is that
> this is asking a human to do the thing that humans are bad at and
> computers are good at. It can't scale where humans are reviewing ports
> that they understand less well than the people contributing them.
> 
> I think we should find a wording that explains the behaviour instead
> of explaining some potential derivative of the behaviour, as well as
> providing a realistic example instead of the MdePkg/MdeModulePkg
> statament.
> 
> My suggestion is to keep it simple: say something like "where there is
> a need to override an include file provided by one package with one
> provided by another package, know that the compiler invocation will
> list the include directories in the same order as the .dec files are
> listed in the .inf".
> 
> Regards,
> 
> Leif
> 
> On Sun, May 31, 2020 at 22:19:24 +0000, Michael D Kinney wrote:
> > Hi Leif,
> >
> > The reason for this statement is not for performance.
> >
> > It is if the same include file exists in the same path
> > in more than one package.  Defining this behavior makes
> > the build system deterministic.
> >
> > There is use case where a platform package can provide
> > an include override of a common package and the platform
> > modules list the platform package before the common
> > package in the [Packages] section.
> >
> > So deterministic build when there are include file
> > name collisions and overrides are 2 reasons to keep
> > the currently defined behavior.
> >
> > With this background, perhaps some clarification or
> > rewording of the spec is required?  Do you have suggestions?
> >
> > Thanks,
> >
> > Mike
> >
> >   This is not a common use case,
> > but one that does apply is a platform module that wants
> > to use an override of a standard include file in a platform
> > package.
> >
> > For the build system autogen stage, this statement
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Leif Lindholm <leif@nuviainc.com>
> > > Sent: Friday, May 29, 2020 7:03 AM
> > > To: devel@edk2.groups.io
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > Andrew Fish <afish@apple.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Pankaj Bansal
> > > <pankaj.bansal@oss.nxp.com>
> > > Subject: [PATCH edk2-InfSpecification] Drop statement
> > > on package ordering
> > >
> > > The description of [Packages] sections stated that
> > > "Packages must be listed in the order that may be
> > > required for specifying
> > >  include path statements for a compiler. For example,
> > > the
> > >  MdePkg/MdePkg.dec file must be listed before the
> > >  MdeModulePkg/MdeModulePkg.dec file."
> > >
> > > Drop it.
> > >
> > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > ---
> > >
> > > Surely this isn't something we take seriously?
> > > If there is a measurable performance impact to the
> > > order of -I option
> > > on the compiler command line, we should approach this
> > > programmatically.
> > >
> > >  3_edk_ii_inf_file_format/37_[packages]_sections.md | 7
> > > ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git
> > > a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > index 17a8d91..c09112b 100644
> > > ---
> > > a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > +++
> > > b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > @@ -42,11 +42,8 @@ Defines the `[Packages]` section tag
> > > that is used in EDK II module INF files.
> > >  Each entry in this section contains a directory name,
> > > forward slash character
> > >  and the name of the DEC file contained in the
> > > directory name.
> > >
> > > -Packages must be listed in the order that may be
> > > required for specifying
> > > -include path statements for a compiler. For example,
> > > the _MdePkg/MdePkg.dec_
> > > -file must be listed before the
> > > `MdeModulePkg/MdeModulePkg.dec` file. If there
> > > -are PCDs listed in the generated "As Built" INF, the
> > > packages that declare any
> > > -PCDs must be listed in this section.
> > > +If there are PCDs listed in the generated "As Built"
> > > INF, the packages that
> > > +declare any PCDs must be listed in this section.
> > >
> > >  Each package filename must be listed only once per
> > > section. Package filenames
> > >  listed in architectural sections are not permitted to
> > > be listed in the common
> > > --
> > > 2.20.1
> >
> >
> > 
> >

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-01  3:39     ` Pankaj Bansal
@ 2020-06-01  5:15       ` Michael D Kinney
  2020-06-01  7:01         ` Pankaj Bansal
  0 siblings, 1 reply; 18+ messages in thread
From: Michael D Kinney @ 2020-06-01  5:15 UTC (permalink / raw)
  To: Pankaj Bansal (OSS), Leif Lindholm, devel@edk2.groups.io,
	Kinney, Michael D
  Cc: Andrew Fish, Laszlo Ersek

Pankaj Bansal,

The override package should be listed before the standard package
in the [Packages] section of an INF file.

The internal behavior of the build system is to generate makefiles
that invoke compilers.  The list of -I include paths passed to
compilers is generated from [Includes] sections of the package DEC
files listed in an INF [Packages] section.  The order that the
-I directives are listed is based is the same order that the
packages are listed in the INF [Packages] section.

Mike

> -----Original Message-----
> From: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Sent: Sunday, May 31, 2020 8:39 PM
> To: Leif Lindholm <leif@nuviainc.com>;
> devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> <lersek@redhat.com>; Pankaj Bansal (OSS)
> <pankaj.bansal@oss.nxp.com>
> Subject: RE: [edk2-devel] [PATCH edk2-InfSpecification]
> Drop statement on package ordering
> 
> Hi Mike,
> 
> This means any port of edk2, should it so wish to
> override the include file provided by edk2 packages
> (MdePkg or MdeModulePkg),
> must be listed after these dec files (MdePkg.dec or
> MdeModulePkg.dec) in an inf file?
> 
> Regards,
> Pankaj Bansal
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Monday, June 1, 2020 4:14 AM
> > To: devel@edk2.groups.io; michael.d.kinney@intel.com
> > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> <lersek@redhat.com>; Pankaj
> > Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > Subject: Re: [edk2-devel] [PATCH edk2-
> InfSpecification] Drop statement on
> > package ordering
> >
> > Hi Mike,
> >
> > Ok, I'm happy to hear that.
> >
> > I agree that the overriding behaviour is useful, and
> it would be good
> > to document it. The problem is that the current
> wording does not say
> > that (in a way that is useful to anyone who does not
> already know what
> > it means). And the MdePkg/MdeModulePkg example sounds
> positively
> > horrific when interpreted in this light.
> >
> > Clearly, my proposed modification is not the right
> thing to do here.
> >
> > The problem with the document implying that the order
> should reflect
> > some sort of hierarchy *apart from when explicitly
> overriding* is that
> > this is asking a human to do the thing that humans
> are bad at and
> > computers are good at. It can't scale where humans
> are reviewing ports
> > that they understand less well than the people
> contributing them.
> >
> > I think we should find a wording that explains the
> behaviour instead
> > of explaining some potential derivative of the
> behaviour, as well as
> > providing a realistic example instead of the
> MdePkg/MdeModulePkg
> > statament.
> >
> > My suggestion is to keep it simple: say something
> like "where there is
> > a need to override an include file provided by one
> package with one
> > provided by another package, know that the compiler
> invocation will
> > list the include directories in the same order as the
> .dec files are
> > listed in the .inf".
> >
> > Regards,
> >
> > Leif
> >
> > On Sun, May 31, 2020 at 22:19:24 +0000, Michael D
> Kinney wrote:
> > > Hi Leif,
> > >
> > > The reason for this statement is not for
> performance.
> > >
> > > It is if the same include file exists in the same
> path
> > > in more than one package.  Defining this behavior
> makes
> > > the build system deterministic.
> > >
> > > There is use case where a platform package can
> provide
> > > an include override of a common package and the
> platform
> > > modules list the platform package before the common
> > > package in the [Packages] section.
> > >
> > > So deterministic build when there are include file
> > > name collisions and overrides are 2 reasons to keep
> > > the currently defined behavior.
> > >
> > > With this background, perhaps some clarification or
> > > rewording of the spec is required?  Do you have
> suggestions?
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > >   This is not a common use case,
> > > but one that does apply is a platform module that
> wants
> > > to use an override of a standard include file in a
> platform
> > > package.
> > >
> > > For the build system autogen stage, this statement
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Leif Lindholm <leif@nuviainc.com>
> > > > Sent: Friday, May 29, 2020 7:03 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Kinney, Michael D
> <michael.d.kinney@intel.com>;
> > > > Andrew Fish <afish@apple.com>; Laszlo Ersek
> > > > <lersek@redhat.com>; Pankaj Bansal
> > > > <pankaj.bansal@oss.nxp.com>
> > > > Subject: [PATCH edk2-InfSpecification] Drop
> statement
> > > > on package ordering
> > > >
> > > > The description of [Packages] sections stated
> that
> > > > "Packages must be listed in the order that may be
> > > > required for specifying
> > > >  include path statements for a compiler. For
> example,
> > > > the
> > > >  MdePkg/MdePkg.dec file must be listed before the
> > > >  MdeModulePkg/MdeModulePkg.dec file."
> > > >
> > > > Drop it.
> > > >
> > > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > > ---
> > > >
> > > > Surely this isn't something we take seriously?
> > > > If there is a measurable performance impact to
> the
> > > > order of -I option
> > > > on the compiler command line, we should approach
> this
> > > > programmatically.
> > > >
> > > >
> 3_edk_ii_inf_file_format/37_[packages]_sections.md | 7
> > > > ++-----
> > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git
> > > >
> a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > >
> b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > index 17a8d91..c09112b 100644
> > > > ---
> > > >
> a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > +++
> > > >
> b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > @@ -42,11 +42,8 @@ Defines the `[Packages]`
> section tag
> > > > that is used in EDK II module INF files.
> > > >  Each entry in this section contains a directory
> name,
> > > > forward slash character
> > > >  and the name of the DEC file contained in the
> > > > directory name.
> > > >
> > > > -Packages must be listed in the order that may be
> > > > required for specifying
> > > > -include path statements for a compiler. For
> example,
> > > > the _MdePkg/MdePkg.dec_
> > > > -file must be listed before the
> > > > `MdeModulePkg/MdeModulePkg.dec` file. If there
> > > > -are PCDs listed in the generated "As Built" INF,
> the
> > > > packages that declare any
> > > > -PCDs must be listed in this section.
> > > > +If there are PCDs listed in the generated "As
> Built"
> > > > INF, the packages that
> > > > +declare any PCDs must be listed in this section.
> > > >
> > > >  Each package filename must be listed only once
> per
> > > > section. Package filenames
> > > >  listed in architectural sections are not
> permitted to
> > > > be listed in the common
> > > > --
> > > > 2.20.1
> > >
> > >
> > > 
> > >

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-01  5:15       ` Michael D Kinney
@ 2020-06-01  7:01         ` Pankaj Bansal
  2020-06-01 15:31           ` Michael D Kinney
  0 siblings, 1 reply; 18+ messages in thread
From: Pankaj Bansal @ 2020-06-01  7:01 UTC (permalink / raw)
  To: Kinney, Michael D, Pankaj Bansal (OSS), Leif Lindholm,
	devel@edk2.groups.io
  Cc: Andrew Fish, Laszlo Ersek

Hi Mike,

But doesn't this violate the edk2 c guidelines ?

https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-2-every-include-file-shall-have-a-unique-name

The case above explicitly forbids the edk2 port to have the same filename that is used by MdePkg / MdeModulePkg.

Which one to follow ??

Regards,
Pankaj Bansal 

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, June 1, 2020 10:45 AM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Leif Lindholm
> <leif@nuviainc.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on
> package ordering
> 
> Pankaj Bansal,
> 
> The override package should be listed before the standard package
> in the [Packages] section of an INF file.
> 
> The internal behavior of the build system is to generate makefiles
> that invoke compilers.  The list of -I include paths passed to
> compilers is generated from [Includes] sections of the package DEC
> files listed in an INF [Packages] section.  The order that the
> -I directives are listed is based is the same order that the
> packages are listed in the INF [Packages] section.
> 
> Mike
> 
> > -----Original Message-----
> > From: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > Sent: Sunday, May 31, 2020 8:39 PM
> > To: Leif Lindholm <leif@nuviainc.com>;
> > devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> > <lersek@redhat.com>; Pankaj Bansal (OSS)
> > <pankaj.bansal@oss.nxp.com>
> > Subject: RE: [edk2-devel] [PATCH edk2-InfSpecification]
> > Drop statement on package ordering
> >
> > Hi Mike,
> >
> > This means any port of edk2, should it so wish to
> > override the include file provided by edk2 packages
> > (MdePkg or MdeModulePkg),
> > must be listed after these dec files (MdePkg.dec or
> > MdeModulePkg.dec) in an inf file?
> >
> > Regards,
> > Pankaj Bansal
> >
> > > -----Original Message-----
> > > From: Leif Lindholm <leif@nuviainc.com>
> > > Sent: Monday, June 1, 2020 4:14 AM
> > > To: devel@edk2.groups.io; michael.d.kinney@intel.com
> > > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> > <lersek@redhat.com>; Pankaj
> > > Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > > Subject: Re: [edk2-devel] [PATCH edk2-
> > InfSpecification] Drop statement on
> > > package ordering
> > >
> > > Hi Mike,
> > >
> > > Ok, I'm happy to hear that.
> > >
> > > I agree that the overriding behaviour is useful, and
> > it would be good
> > > to document it. The problem is that the current
> > wording does not say
> > > that (in a way that is useful to anyone who does not
> > already know what
> > > it means). And the MdePkg/MdeModulePkg example sounds
> > positively
> > > horrific when interpreted in this light.
> > >
> > > Clearly, my proposed modification is not the right
> > thing to do here.
> > >
> > > The problem with the document implying that the order
> > should reflect
> > > some sort of hierarchy *apart from when explicitly
> > overriding* is that
> > > this is asking a human to do the thing that humans
> > are bad at and
> > > computers are good at. It can't scale where humans
> > are reviewing ports
> > > that they understand less well than the people
> > contributing them.
> > >
> > > I think we should find a wording that explains the
> > behaviour instead
> > > of explaining some potential derivative of the
> > behaviour, as well as
> > > providing a realistic example instead of the
> > MdePkg/MdeModulePkg
> > > statament.
> > >
> > > My suggestion is to keep it simple: say something
> > like "where there is
> > > a need to override an include file provided by one
> > package with one
> > > provided by another package, know that the compiler
> > invocation will
> > > list the include directories in the same order as the
> > .dec files are
> > > listed in the .inf".
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > On Sun, May 31, 2020 at 22:19:24 +0000, Michael D
> > Kinney wrote:
> > > > Hi Leif,
> > > >
> > > > The reason for this statement is not for
> > performance.
> > > >
> > > > It is if the same include file exists in the same
> > path
> > > > in more than one package.  Defining this behavior
> > makes
> > > > the build system deterministic.
> > > >
> > > > There is use case where a platform package can
> > provide
> > > > an include override of a common package and the
> > platform
> > > > modules list the platform package before the common
> > > > package in the [Packages] section.
> > > >
> > > > So deterministic build when there are include file
> > > > name collisions and overrides are 2 reasons to keep
> > > > the currently defined behavior.
> > > >
> > > > With this background, perhaps some clarification or
> > > > rewording of the spec is required?  Do you have
> > suggestions?
> > > >
> > > > Thanks,
> > > >
> > > > Mike
> > > >
> > > >   This is not a common use case,
> > > > but one that does apply is a platform module that
> > wants
> > > > to use an override of a standard include file in a
> > platform
> > > > package.
> > > >
> > > > For the build system autogen stage, this statement
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: Leif Lindholm <leif@nuviainc.com>
> > > > > Sent: Friday, May 29, 2020 7:03 AM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Kinney, Michael D
> > <michael.d.kinney@intel.com>;
> > > > > Andrew Fish <afish@apple.com>; Laszlo Ersek
> > > > > <lersek@redhat.com>; Pankaj Bansal
> > > > > <pankaj.bansal@oss.nxp.com>
> > > > > Subject: [PATCH edk2-InfSpecification] Drop
> > statement
> > > > > on package ordering
> > > > >
> > > > > The description of [Packages] sections stated
> > that
> > > > > "Packages must be listed in the order that may be
> > > > > required for specifying
> > > > >  include path statements for a compiler. For
> > example,
> > > > > the
> > > > >  MdePkg/MdePkg.dec file must be listed before the
> > > > >  MdeModulePkg/MdeModulePkg.dec file."
> > > > >
> > > > > Drop it.
> > > > >
> > > > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > > > ---
> > > > >
> > > > > Surely this isn't something we take seriously?
> > > > > If there is a measurable performance impact to
> > the
> > > > > order of -I option
> > > > > on the compiler command line, we should approach
> > this
> > > > > programmatically.
> > > > >
> > > > >
> > 3_edk_ii_inf_file_format/37_[packages]_sections.md | 7
> > > > > ++-----
> > > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git
> > > > >
> > a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > >
> > b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > index 17a8d91..c09112b 100644
> > > > > ---
> > > > >
> > a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > +++
> > > > >
> > b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > @@ -42,11 +42,8 @@ Defines the `[Packages]`
> > section tag
> > > > > that is used in EDK II module INF files.
> > > > >  Each entry in this section contains a directory
> > name,
> > > > > forward slash character
> > > > >  and the name of the DEC file contained in the
> > > > > directory name.
> > > > >
> > > > > -Packages must be listed in the order that may be
> > > > > required for specifying
> > > > > -include path statements for a compiler. For
> > example,
> > > > > the _MdePkg/MdePkg.dec_
> > > > > -file must be listed before the
> > > > > `MdeModulePkg/MdeModulePkg.dec` file. If there
> > > > > -are PCDs listed in the generated "As Built" INF,
> > the
> > > > > packages that declare any
> > > > > -PCDs must be listed in this section.
> > > > > +If there are PCDs listed in the generated "As
> > Built"
> > > > > INF, the packages that
> > > > > +declare any PCDs must be listed in this section.
> > > > >
> > > > >  Each package filename must be listed only once
> > per
> > > > > section. Package filenames
> > > > >  listed in architectural sections are not
> > permitted to
> > > > > be listed in the common
> > > > > --
> > > > > 2.20.1
> > > >
> > > >
> > > > 
> > > >

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-01  7:01         ` Pankaj Bansal
@ 2020-06-01 15:31           ` Michael D Kinney
  0 siblings, 0 replies; 18+ messages in thread
From: Michael D Kinney @ 2020-06-01 15:31 UTC (permalink / raw)
  To: Pankaj Bansal (OSS), Leif Lindholm, devel@edk2.groups.io,
	Kinney, Michael D
  Cc: Andrew Fish, Laszlo Ersek

Pankaj Bansal,

Of course it is better if there is never a name collision,
and for the packages maintained in the open source TianoCore
projects, we can check for these name collisions.

However, there can be name collisions between packages 
not maintained within TianoCore.

This is why a clear rule needs to be followed by the 
build system, so the builds are always deterministic.

I agree that the override use case I described does break
the rule you pointed to in the EDK II C Coding Standard.
There are multiple methods to override an include file
in a common package.  All of these are complex and 
difficult to maintain.  It is always better to avoid
the need to perform these type of overrides.

Deterministic builds across all OS/compiler combinations
when include name collisions can occur between TianoCore
and non-TianoCore packages is the primary reason for 
defining the include order in the EDK II build specs.

Mike


> -----Original Message-----
> From: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Sent: Monday, June 1, 2020 12:01 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Leif
> Lindholm <leif@nuviainc.com>; devel@edk2.groups.io
> Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH edk2-InfSpecification]
> Drop statement on package ordering
> 
> Hi Mike,
> 
> But doesn't this violate the edk2 c guidelines ?
> 
> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-
> specification/5_source_files/53_include_files#5-3-2-
> every-include-file-shall-have-a-unique-name
> 
> The case above explicitly forbids the edk2 port to have
> the same filename that is used by MdePkg /
> MdeModulePkg.
> 
> Which one to follow ??
> 
> Regards,
> Pankaj Bansal
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Monday, June 1, 2020 10:45 AM
> > To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>;
> Leif Lindholm
> > <leif@nuviainc.com>; devel@edk2.groups.io; Kinney,
> Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> <lersek@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH edk2-
> InfSpecification] Drop statement on
> > package ordering
> >
> > Pankaj Bansal,
> >
> > The override package should be listed before the
> standard package
> > in the [Packages] section of an INF file.
> >
> > The internal behavior of the build system is to
> generate makefiles
> > that invoke compilers.  The list of -I include paths
> passed to
> > compilers is generated from [Includes] sections of
> the package DEC
> > files listed in an INF [Packages] section.  The order
> that the
> > -I directives are listed is based is the same order
> that the
> > packages are listed in the INF [Packages] section.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Pankaj Bansal (OSS)
> <pankaj.bansal@oss.nxp.com>
> > > Sent: Sunday, May 31, 2020 8:39 PM
> > > To: Leif Lindholm <leif@nuviainc.com>;
> > > devel@edk2.groups.io; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Pankaj Bansal (OSS)
> > > <pankaj.bansal@oss.nxp.com>
> > > Subject: RE: [edk2-devel] [PATCH edk2-
> InfSpecification]
> > > Drop statement on package ordering
> > >
> > > Hi Mike,
> > >
> > > This means any port of edk2, should it so wish to
> > > override the include file provided by edk2 packages
> > > (MdePkg or MdeModulePkg),
> > > must be listed after these dec files (MdePkg.dec or
> > > MdeModulePkg.dec) in an inf file?
> > >
> > > Regards,
> > > Pankaj Bansal
> > >
> > > > -----Original Message-----
> > > > From: Leif Lindholm <leif@nuviainc.com>
> > > > Sent: Monday, June 1, 2020 4:14 AM
> > > > To: devel@edk2.groups.io;
> michael.d.kinney@intel.com
> > > > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Pankaj
> > > > Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > > > Subject: Re: [edk2-devel] [PATCH edk2-
> > > InfSpecification] Drop statement on
> > > > package ordering
> > > >
> > > > Hi Mike,
> > > >
> > > > Ok, I'm happy to hear that.
> > > >
> > > > I agree that the overriding behaviour is useful,
> and
> > > it would be good
> > > > to document it. The problem is that the current
> > > wording does not say
> > > > that (in a way that is useful to anyone who does
> not
> > > already know what
> > > > it means). And the MdePkg/MdeModulePkg example
> sounds
> > > positively
> > > > horrific when interpreted in this light.
> > > >
> > > > Clearly, my proposed modification is not the
> right
> > > thing to do here.
> > > >
> > > > The problem with the document implying that the
> order
> > > should reflect
> > > > some sort of hierarchy *apart from when
> explicitly
> > > overriding* is that
> > > > this is asking a human to do the thing that
> humans
> > > are bad at and
> > > > computers are good at. It can't scale where
> humans
> > > are reviewing ports
> > > > that they understand less well than the people
> > > contributing them.
> > > >
> > > > I think we should find a wording that explains
> the
> > > behaviour instead
> > > > of explaining some potential derivative of the
> > > behaviour, as well as
> > > > providing a realistic example instead of the
> > > MdePkg/MdeModulePkg
> > > > statament.
> > > >
> > > > My suggestion is to keep it simple: say something
> > > like "where there is
> > > > a need to override an include file provided by
> one
> > > package with one
> > > > provided by another package, know that the
> compiler
> > > invocation will
> > > > list the include directories in the same order as
> the
> > > .dec files are
> > > > listed in the .inf".
> > > >
> > > > Regards,
> > > >
> > > > Leif
> > > >
> > > > On Sun, May 31, 2020 at 22:19:24 +0000, Michael D
> > > Kinney wrote:
> > > > > Hi Leif,
> > > > >
> > > > > The reason for this statement is not for
> > > performance.
> > > > >
> > > > > It is if the same include file exists in the
> same
> > > path
> > > > > in more than one package.  Defining this
> behavior
> > > makes
> > > > > the build system deterministic.
> > > > >
> > > > > There is use case where a platform package can
> > > provide
> > > > > an include override of a common package and the
> > > platform
> > > > > modules list the platform package before the
> common
> > > > > package in the [Packages] section.
> > > > >
> > > > > So deterministic build when there are include
> file
> > > > > name collisions and overrides are 2 reasons to
> keep
> > > > > the currently defined behavior.
> > > > >
> > > > > With this background, perhaps some
> clarification or
> > > > > rewording of the spec is required?  Do you have
> > > suggestions?
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Mike
> > > > >
> > > > >   This is not a common use case,
> > > > > but one that does apply is a platform module
> that
> > > wants
> > > > > to use an override of a standard include file
> in a
> > > platform
> > > > > package.
> > > > >
> > > > > For the build system autogen stage, this
> statement
> > > > >
> > > > > Mike
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Leif Lindholm <leif@nuviainc.com>
> > > > > > Sent: Friday, May 29, 2020 7:03 AM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Kinney, Michael D
> > > <michael.d.kinney@intel.com>;
> > > > > > Andrew Fish <afish@apple.com>; Laszlo Ersek
> > > > > > <lersek@redhat.com>; Pankaj Bansal
> > > > > > <pankaj.bansal@oss.nxp.com>
> > > > > > Subject: [PATCH edk2-InfSpecification] Drop
> > > statement
> > > > > > on package ordering
> > > > > >
> > > > > > The description of [Packages] sections stated
> > > that
> > > > > > "Packages must be listed in the order that
> may be
> > > > > > required for specifying
> > > > > >  include path statements for a compiler. For
> > > example,
> > > > > > the
> > > > > >  MdePkg/MdePkg.dec file must be listed before
> the
> > > > > >  MdeModulePkg/MdeModulePkg.dec file."
> > > > > >
> > > > > > Drop it.
> > > > > >
> > > > > > Signed-off-by: Leif Lindholm
> <leif@nuviainc.com>
> > > > > > ---
> > > > > >
> > > > > > Surely this isn't something we take
> seriously?
> > > > > > If there is a measurable performance impact
> to
> > > the
> > > > > > order of -I option
> > > > > > on the compiler command line, we should
> approach
> > > this
> > > > > > programmatically.
> > > > > >
> > > > > >
> > > 3_edk_ii_inf_file_format/37_[packages]_sections.md
> | 7
> > > > > > ++-----
> > > > > >  1 file changed, 2 insertions(+), 5
> deletions(-)
> > > > > >
> > > > > > diff --git
> > > > > >
> > >
> a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > >
> > >
> b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > > index 17a8d91..c09112b 100644
> > > > > > ---
> > > > > >
> > >
> a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > > +++
> > > > > >
> > >
> b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> > > > > > @@ -42,11 +42,8 @@ Defines the `[Packages]`
> > > section tag
> > > > > > that is used in EDK II module INF files.
> > > > > >  Each entry in this section contains a
> directory
> > > name,
> > > > > > forward slash character
> > > > > >  and the name of the DEC file contained in
> the
> > > > > > directory name.
> > > > > >
> > > > > > -Packages must be listed in the order that
> may be
> > > > > > required for specifying
> > > > > > -include path statements for a compiler. For
> > > example,
> > > > > > the _MdePkg/MdePkg.dec_
> > > > > > -file must be listed before the
> > > > > > `MdeModulePkg/MdeModulePkg.dec` file. If
> there
> > > > > > -are PCDs listed in the generated "As Built"
> INF,
> > > the
> > > > > > packages that declare any
> > > > > > -PCDs must be listed in this section.
> > > > > > +If there are PCDs listed in the generated
> "As
> > > Built"
> > > > > > INF, the packages that
> > > > > > +declare any PCDs must be listed in this
> section.
> > > > > >
> > > > > >  Each package filename must be listed only
> once
> > > per
> > > > > > section. Package filenames
> > > > > >  listed in architectural sections are not
> > > permitted to
> > > > > > be listed in the common
> > > > > > --
> > > > > > 2.20.1
> > > > >
> > > > >
> > > > > 
> > > > >

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-05-31 22:43   ` [edk2-devel] " Leif Lindholm
  2020-06-01  3:39     ` Pankaj Bansal
@ 2020-06-02 13:29     ` Laszlo Ersek
  2020-06-02 13:37       ` Pankaj Bansal
  2020-06-02 14:20       ` Leif Lindholm
  1 sibling, 2 replies; 18+ messages in thread
From: Laszlo Ersek @ 2020-06-02 13:29 UTC (permalink / raw)
  To: Leif Lindholm, devel, michael.d.kinney; +Cc: Andrew Fish, Pankaj Bansal

On 06/01/20 00:43, Leif Lindholm wrote:
> Hi Mike,
> 
> Ok, I'm happy to hear that.
> 
> I agree that the overriding behaviour is useful, and it would be good
> to document it. The problem is that the current wording does not say
> that (in a way that is useful to anyone who does not already know what
> it means). And the MdePkg/MdeModulePkg example sounds positively
> horrific when interpreted in this light.
> 
> Clearly, my proposed modification is not the right thing to do here.
> 
> The problem with the document implying that the order should reflect
> some sort of hierarchy *apart from when explicitly overriding* is that
> this is asking a human to do the thing that humans are bad at and
> computers are good at. It can't scale where humans are reviewing ports
> that they understand less well than the people contributing them.
> 
> I think we should find a wording that explains the behaviour instead
> of explaining some potential derivative of the behaviour, as well as
> providing a realistic example instead of the MdePkg/MdeModulePkg
> statament.
> 
> My suggestion is to keep it simple: say something like "where there is
> a need to override an include file provided by one package with one
> provided by another package, know that the compiler invocation will
> list the include directories in the same order as the .dec files are
> listed in the .inf".

(since I've been copied)

I have not been aware of the header name collision scenario (nor that
the [Packages] ordering was supposed to work around such issues).

I work strictly with edk2 proper, where a name collision like this can
be detected, and so should be prevented. (Insert yet another argument
why keeping platform code outside of edk2 is a bad idea.) In particular,
a collision between MdePkg and MdeModulePkg would be super bad.

Which now seems to turn out consistent with my general review point that
the [Packages] section, like (almost) all other INF file sections,
should be sorted lexicographically.

How about replacing

"""
Packages must be listed in the order that may be required for specifying
include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
"""

with

"""
The order in which packages are listed may be relevant. Said order
specifies in what order include path statements are generated for a
compiler. Normally, header file name collisions are not expected between
packages -- they are forbidden in edk2 proper --, but with a module INF
consuming both edk2-native and out-of-edk2 packages, header file names
may collide. For setting specific include path priorities, the packages
may be listed in matching order in the INF file. Listing a package
earlier will cause a compiler to consider include paths from that
package earlier.
"""

Thanks
Laszlo


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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 13:29     ` Laszlo Ersek
@ 2020-06-02 13:37       ` Pankaj Bansal
  2020-06-02 14:22         ` Leif Lindholm
  2020-06-02 16:11         ` Laszlo Ersek
  2020-06-02 14:20       ` Leif Lindholm
  1 sibling, 2 replies; 18+ messages in thread
From: Pankaj Bansal @ 2020-06-02 13:37 UTC (permalink / raw)
  To: Laszlo Ersek, Leif Lindholm, devel@edk2.groups.io,
	michael.d.kinney@intel.com
  Cc: Andrew Fish



> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, June 2, 2020 7:00 PM
> To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io;
> michael.d.kinney@intel.com
> Cc: Andrew Fish <afish@apple.com>; Pankaj Bansal (OSS)
> <pankaj.bansal@oss.nxp.com>
> Subject: Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on
> package ordering
> 
> On 06/01/20 00:43, Leif Lindholm wrote:
> > Hi Mike,
> >
> > Ok, I'm happy to hear that.
> >
> > I agree that the overriding behaviour is useful, and it would be good
> > to document it. The problem is that the current wording does not say
> > that (in a way that is useful to anyone who does not already know what
> > it means). And the MdePkg/MdeModulePkg example sounds positively
> > horrific when interpreted in this light.
> >
> > Clearly, my proposed modification is not the right thing to do here.
> >
> > The problem with the document implying that the order should reflect
> > some sort of hierarchy *apart from when explicitly overriding* is that
> > this is asking a human to do the thing that humans are bad at and
> > computers are good at. It can't scale where humans are reviewing ports
> > that they understand less well than the people contributing them.
> >
> > I think we should find a wording that explains the behaviour instead
> > of explaining some potential derivative of the behaviour, as well as
> > providing a realistic example instead of the MdePkg/MdeModulePkg
> > statament.
> >
> > My suggestion is to keep it simple: say something like "where there is
> > a need to override an include file provided by one package with one
> > provided by another package, know that the compiler invocation will
> > list the include directories in the same order as the .dec files are
> > listed in the .inf".
> 
> (since I've been copied)
> 
> I have not been aware of the header name collision scenario (nor that
> the [Packages] ordering was supposed to work around such issues).
> 
> I work strictly with edk2 proper, where a name collision like this can
> be detected, and so should be prevented. (Insert yet another argument
> why keeping platform code outside of edk2 is a bad idea.) In particular,
> a collision between MdePkg and MdeModulePkg would be super bad.
> 
> Which now seems to turn out consistent with my general review point that
> the [Packages] section, like (almost) all other INF file sections,
> should be sorted lexicographically.
> 
> How about replacing
> 
> """
> Packages must be listed in the order that may be required for specifying
> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
> """
> 
> with
> 
> """
> The order in which packages are listed may be relevant. Said order
> specifies in what order include path statements are generated for a
> compiler. Normally, header file name collisions are not expected between
> packages -- they are forbidden in edk2 proper --, but with a module INF
> consuming both edk2-native and out-of-edk2 packages, header file names
> may collide. For setting specific include path priorities, the packages
> may be listed in matching order in the INF file. Listing a package
> earlier will cause a compiler to consider include paths from that
> package earlier.
> """

Nicely summed up! it is much clearer now for anyone like me who wants to port edk2 for his platform.
one more suggestion. should this be mentioned along with above explaination:
"whenever possible use lexicographically ascending order"

> 
> Thanks
> Laszlo


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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 13:29     ` Laszlo Ersek
  2020-06-02 13:37       ` Pankaj Bansal
@ 2020-06-02 14:20       ` Leif Lindholm
  2020-06-02 16:20         ` Laszlo Ersek
  1 sibling, 1 reply; 18+ messages in thread
From: Leif Lindholm @ 2020-06-02 14:20 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: devel, michael.d.kinney, Andrew Fish, Pankaj Bansal

On Tue, Jun 02, 2020 at 15:29:55 +0200, Laszlo Ersek wrote:
> I have not been aware of the header name collision scenario (nor that
> the [Packages] ordering was supposed to work around such issues).

Nor had I...

> I work strictly with edk2 proper, where a name collision like this can
> be detected, and so should be prevented. (Insert yet another argument
> why keeping platform code outside of edk2 is a bad idea.) In particular,
> a collision between MdePkg and MdeModulePkg would be super bad.
> 
> Which now seems to turn out consistent with my general review point that
> the [Packages] section, like (almost) all other INF file sections,
> should be sorted lexicographically.
> 
> How about replacing
> 
> """
> Packages must be listed in the order that may be required for specifying
> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
> """
> 
> with
> 
> """
> The order in which packages are listed may be relevant. Said order
> specifies in what order include path statements are generated for a
> compiler. Normally, header file name collisions are not expected between
> packages -- they are forbidden in edk2 proper --, but with a module INF
> consuming both edk2-native and out-of-edk2 packages, header file names
> may collide. For setting specific include path priorities, the packages
> may be listed in matching order in the INF file. Listing a package
> earlier will cause a compiler to consider include paths from that
> package earlier.
> """

Could I suggest striking:
" -- they are forbidden in edk2 proper --, but with a module INF
consuming both edk2-native and out-of-edk2 packages, header file
names may collide"?

This document specifies a file format, not automatically edk2-related.

I think we're reaching a point where a major documentation overhaul is
necessary. I had already been reflecting on how the coding style
document encompasses more than coding style (at one point it explains
how while() loops are different from do{}while() loops). And we
recently had that conversation around struct assignments which some
maintainers claim are banned, but which is not mentioned in that
document.

Not trying to resolve that issue *now*, just reflecting on how some
things have been added to these documents historically to deal with a
specific issue, and ended up confusing things as improved development
practices have made the original problem go away.

So with the edk2 refences removed, I like your new wording.

/
    Leif



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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 13:37       ` Pankaj Bansal
@ 2020-06-02 14:22         ` Leif Lindholm
  2020-06-02 16:11         ` Laszlo Ersek
  1 sibling, 0 replies; 18+ messages in thread
From: Leif Lindholm @ 2020-06-02 14:22 UTC (permalink / raw)
  To: Pankaj Bansal (OSS)
  Cc: Laszlo Ersek, devel@edk2.groups.io, michael.d.kinney@intel.com,
	Andrew Fish

Hi Pankaj,

On Tue, Jun 02, 2020 at 13:37:30 +0000, Pankaj Bansal (OSS) wrote:
> > How about replacing
> > 
> > """
> > Packages must be listed in the order that may be required for specifying
> > include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
> > file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
> > """
> > 
> > with
> > 
> > """
> > The order in which packages are listed may be relevant. Said order
> > specifies in what order include path statements are generated for a
> > compiler. Normally, header file name collisions are not expected between
> > packages -- they are forbidden in edk2 proper --, but with a module INF
> > consuming both edk2-native and out-of-edk2 packages, header file names
> > may collide. For setting specific include path priorities, the packages
> > may be listed in matching order in the INF file. Listing a package
> > earlier will cause a compiler to consider include paths from that
> > package earlier.
> > """
> 
> Nicely summed up! it is much clearer now for anyone like me who
> wants to port edk2 for his platform.
> one more suggestion. should this be mentioned along with above
> explaination:
> "whenever possible use lexicographically ascending order"

My preference would be that we rework the coding style document to a
document that encompasses style of all contributions, and drop some of
the things that are not style related.

(For example, we already know that we're short on documentation on
Python contributions.)

/
    Leif

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 13:37       ` Pankaj Bansal
  2020-06-02 14:22         ` Leif Lindholm
@ 2020-06-02 16:11         ` Laszlo Ersek
  2020-06-03  3:12           ` Pankaj Bansal
  1 sibling, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2020-06-02 16:11 UTC (permalink / raw)
  To: Pankaj Bansal (OSS), Leif Lindholm, devel@edk2.groups.io,
	michael.d.kinney@intel.com
  Cc: Andrew Fish

On 06/02/20 15:37, Pankaj Bansal (OSS) wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, June 2, 2020 7:00 PM
>> To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io;
>> michael.d.kinney@intel.com
>> Cc: Andrew Fish <afish@apple.com>; Pankaj Bansal (OSS)
>> <pankaj.bansal@oss.nxp.com>
>> Subject: Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on
>> package ordering
>>
>> On 06/01/20 00:43, Leif Lindholm wrote:
>>> Hi Mike,
>>>
>>> Ok, I'm happy to hear that.
>>>
>>> I agree that the overriding behaviour is useful, and it would be good
>>> to document it. The problem is that the current wording does not say
>>> that (in a way that is useful to anyone who does not already know what
>>> it means). And the MdePkg/MdeModulePkg example sounds positively
>>> horrific when interpreted in this light.
>>>
>>> Clearly, my proposed modification is not the right thing to do here.
>>>
>>> The problem with the document implying that the order should reflect
>>> some sort of hierarchy *apart from when explicitly overriding* is that
>>> this is asking a human to do the thing that humans are bad at and
>>> computers are good at. It can't scale where humans are reviewing ports
>>> that they understand less well than the people contributing them.
>>>
>>> I think we should find a wording that explains the behaviour instead
>>> of explaining some potential derivative of the behaviour, as well as
>>> providing a realistic example instead of the MdePkg/MdeModulePkg
>>> statament.
>>>
>>> My suggestion is to keep it simple: say something like "where there is
>>> a need to override an include file provided by one package with one
>>> provided by another package, know that the compiler invocation will
>>> list the include directories in the same order as the .dec files are
>>> listed in the .inf".
>>
>> (since I've been copied)
>>
>> I have not been aware of the header name collision scenario (nor that
>> the [Packages] ordering was supposed to work around such issues).
>>
>> I work strictly with edk2 proper, where a name collision like this can
>> be detected, and so should be prevented. (Insert yet another argument
>> why keeping platform code outside of edk2 is a bad idea.) In particular,
>> a collision between MdePkg and MdeModulePkg would be super bad.
>>
>> Which now seems to turn out consistent with my general review point that
>> the [Packages] section, like (almost) all other INF file sections,
>> should be sorted lexicographically.
>>
>> How about replacing
>>
>> """
>> Packages must be listed in the order that may be required for specifying
>> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
>> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
>> """
>>
>> with
>>
>> """
>> The order in which packages are listed may be relevant. Said order
>> specifies in what order include path statements are generated for a
>> compiler. Normally, header file name collisions are not expected between
>> packages -- they are forbidden in edk2 proper --, but with a module INF
>> consuming both edk2-native and out-of-edk2 packages, header file names
>> may collide. For setting specific include path priorities, the packages
>> may be listed in matching order in the INF file. Listing a package
>> earlier will cause a compiler to consider include paths from that
>> package earlier.
>> """
> 
> Nicely summed up! it is much clearer now for anyone like me who wants to port edk2 for his platform.
> one more suggestion. should this be mentioned along with above explaination:
> "whenever possible use lexicographically ascending order"

I'd love that, but it's really just a policy question that I prefer.

If we tried to elevate my preference to official edk2 spec level, it
could run into opposition (like any other proposal -- so that would be
just fine, per se!). I just wouldn't like to delay the more important
clarification with a discussion around my preference.

So minimally, that would take a two-part patch series, and even so the
second patch would likely have to be marked RFC. I think we can simply
postpone the official speccing of the lexicographical sorting idea
(indefinitely, even).

Thanks
Laszlo


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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 14:20       ` Leif Lindholm
@ 2020-06-02 16:20         ` Laszlo Ersek
  2020-06-03 11:44           ` Leif Lindholm
  0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2020-06-02 16:20 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: devel, michael.d.kinney, Andrew Fish, Pankaj Bansal

On 06/02/20 16:20, Leif Lindholm wrote:
> On Tue, Jun 02, 2020 at 15:29:55 +0200, Laszlo Ersek wrote:
>> I have not been aware of the header name collision scenario (nor that
>> the [Packages] ordering was supposed to work around such issues).
> 
> Nor had I...
> 
>> I work strictly with edk2 proper, where a name collision like this can
>> be detected, and so should be prevented. (Insert yet another argument
>> why keeping platform code outside of edk2 is a bad idea.) In particular,
>> a collision between MdePkg and MdeModulePkg would be super bad.
>>
>> Which now seems to turn out consistent with my general review point that
>> the [Packages] section, like (almost) all other INF file sections,
>> should be sorted lexicographically.
>>
>> How about replacing
>>
>> """
>> Packages must be listed in the order that may be required for specifying
>> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
>> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
>> """
>>
>> with
>>
>> """
>> The order in which packages are listed may be relevant. Said order
>> specifies in what order include path statements are generated for a
>> compiler. Normally, header file name collisions are not expected between
>> packages -- they are forbidden in edk2 proper --, but with a module INF
>> consuming both edk2-native and out-of-edk2 packages, header file names
>> may collide. For setting specific include path priorities, the packages
>> may be listed in matching order in the INF file. Listing a package
>> earlier will cause a compiler to consider include paths from that
>> package earlier.
>> """
> 
> Could I suggest striking:
> " -- they are forbidden in edk2 proper --, but with a module INF
> consuming both edk2-native and out-of-edk2 packages, header file
> names may collide"?

I'm sad; that's the part I like the most! ;) That describes the actual
use case (I'm a fan of use case details in commit messages too).

Anyway, I don't insist...

> 
> This document specifies a file format, not automatically edk2-related.

I disagree with this specific statement; the INF spec says "edk2" in the
*name*. It's called "edk2 INF specification".

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications

"This page contains the released versions of the EDK II Specifications
published using Gitbook."

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications#inf

"This document describes the EDK II build information (INF) file format."

The following link doesn't seem to load at the moment:

https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/v/release/1.27/

but checking the source in the git repo
<https://github.com/tianocore-docs/edk2-InfSpecification>, the actual
text seems to say "EDK II Module Information (INF) File Specification".

The whole feature is related to out-of-tree INF files (where header file
name collisions cannot be easily detected).

> 
> I think we're reaching a point where a major documentation overhaul is
> necessary. I had already been reflecting on how the coding style
> document encompasses more than coding style (at one point it explains
> how while() loops are different from do{}while() loops). And we
> recently had that conversation around struct assignments which some
> maintainers claim are banned, but which is not mentioned in that
> document.
> 
> Not trying to resolve that issue *now*, just reflecting on how some
> things have been added to these documents historically to deal with a
> specific issue, and ended up confusing things as improved development
> practices have made the original problem go away.
> 
> So with the edk2 refences removed, I like your new wording.

OK -- I won't let "perfect" get in the way of "good" :)

Thanks!
Laszlo


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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 16:11         ` Laszlo Ersek
@ 2020-06-03  3:12           ` Pankaj Bansal
  0 siblings, 0 replies; 18+ messages in thread
From: Pankaj Bansal @ 2020-06-03  3:12 UTC (permalink / raw)
  To: Laszlo Ersek, Leif Lindholm, devel@edk2.groups.io,
	michael.d.kinney@intel.com
  Cc: Andrew Fish

> >>
> >> (since I've been copied)
> >>
> >> I have not been aware of the header name collision scenario (nor that
> >> the [Packages] ordering was supposed to work around such issues).
> >>
> >> I work strictly with edk2 proper, where a name collision like this can
> >> be detected, and so should be prevented. (Insert yet another argument
> >> why keeping platform code outside of edk2 is a bad idea.) In particular,
> >> a collision between MdePkg and MdeModulePkg would be super bad.
> >>
> >> Which now seems to turn out consistent with my general review point that
> >> the [Packages] section, like (almost) all other INF file sections,
> >> should be sorted lexicographically.
> >>
> >> How about replacing
> >>
> >> """
> >> Packages must be listed in the order that may be required for specifying
> >> include path statements for a compiler. For example, the
> MdePkg/MdePkg.dec_
> >> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
> >> """
> >>
> >> with
> >>
> >> """
> >> The order in which packages are listed may be relevant. Said order
> >> specifies in what order include path statements are generated for a
> >> compiler. Normally, header file name collisions are not expected between
> >> packages -- they are forbidden in edk2 proper --, but with a module INF
> >> consuming both edk2-native and out-of-edk2 packages, header file names
> >> may collide. For setting specific include path priorities, the packages
> >> may be listed in matching order in the INF file. Listing a package
> >> earlier will cause a compiler to consider include paths from that
> >> package earlier.
> >> """
> >
> > Nicely summed up! it is much clearer now for anyone like me who wants to
> port edk2 for his platform.
> > one more suggestion. should this be mentioned along with above explaination:
> > "whenever possible use lexicographically ascending order"
> 
> I'd love that, but it's really just a policy question that I prefer.
> 
> If we tried to elevate my preference to official edk2 spec level, it
> could run into opposition (like any other proposal -- so that would be
> just fine, per se!). I just wouldn't like to delay the more important
> clarification with a discussion around my preference.
> 
> So minimally, that would take a two-part patch series, and even so the
> second patch would likely have to be marked RFC. I think we can simply
> postpone the official speccing of the lexicographical sorting idea
> (indefinitely, even).

The point that I want to make is that these document should guide new
developers, as to how to contribute to edk2 code as well as explaining how
edk2 code has been organized.

To explain a practice historically followed in edk2 (but no longer followed),
is informercial to new developers. but it is of little help for them to contribute to edk2.

perhaps along the edk2 c guidelines, there should be a section in each specification
file, titled Guidelines ?

> 
> Thanks
> Laszlo


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

* Re: [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-05-29 14:02 [PATCH edk2-InfSpecification] Drop statement on package ordering Leif Lindholm
  2020-05-31 22:19 ` Michael D Kinney
@ 2020-06-03  3:33 ` Andrew Fish
       [not found] ` <1614EB3F428C08F5.21938@groups.io>
  2 siblings, 0 replies; 18+ messages in thread
From: Andrew Fish @ 2020-06-03  3:33 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: edk2-devel-groups-io, Mike Kinney, Laszlo Ersek, Pankaj Bansal



> On May 29, 2020, at 7:02 AM, Leif Lindholm <leif@nuviainc.com> wrote:
> 
> The description of [Packages] sections stated that
> "Packages must be listed in the order that may be required for specifying
> include path statements for a compiler. For example, the
> MdePkg/MdePkg.dec file must be listed before the
> MdeModulePkg/MdeModulePkg.dec file."
> 
> Drop it.
> 
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> 
> Surely this isn't something we take seriously?
> If there is a measurable performance impact to the order of -I option
> on the compiler command line, we should approach this programmatically.
> 

I think the intent of this was to deal with duplicates include file names, and it was not really about build performance. 

Thanks,

Andrew Fish

> 3_edk_ii_inf_file_format/37_[packages]_sections.md | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/3_edk_ii_inf_file_format/37_[packages]_sections.md b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> index 17a8d91..c09112b 100644
> --- a/3_edk_ii_inf_file_format/37_[packages]_sections.md
> +++ b/3_edk_ii_inf_file_format/37_[packages]_sections.md
> @@ -42,11 +42,8 @@ Defines the `[Packages]` section tag that is used in EDK II module INF files.
> Each entry in this section contains a directory name, forward slash character
> and the name of the DEC file contained in the directory name.
> 
> -Packages must be listed in the order that may be required for specifying
> -include path statements for a compiler. For example, the _MdePkg/MdePkg.dec_
> -file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file. If there
> -are PCDs listed in the generated "As Built" INF, the packages that declare any
> -PCDs must be listed in this section.
> +If there are PCDs listed in the generated "As Built" INF, the packages that
> +declare any PCDs must be listed in this section.
> 
> Each package filename must be listed only once per section. Package filenames
> listed in architectural sections are not permitted to be listed in the common
> -- 
> 2.20.1
> 


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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
       [not found] ` <1614EB3F428C08F5.21938@groups.io>
@ 2020-06-03  3:41   ` Andrew Fish
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Fish @ 2020-06-03  3:41 UTC (permalink / raw)
  To: edk2-devel-groups-io, Andrew Fish
  Cc: Leif Lindholm, Mike Kinney, Laszlo Ersek, Pankaj Bansal

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



> On Jun 2, 2020, at 8:33 PM, Andrew Fish via groups.io <afish=apple.com@groups.io> wrote:
> 
> 
> 
>> On May 29, 2020, at 7:02 AM, Leif Lindholm <leif@nuviainc.com> wrote:
>> 
>> The description of [Packages] sections stated that
>> "Packages must be listed in the order that may be required for specifying
>> include path statements for a compiler. For example, the
>> MdePkg/MdePkg.dec file must be listed before the
>> MdeModulePkg/MdeModulePkg.dec file."
>> 
>> Drop it.
>> 
>> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
>> ---
>> 
>> Surely this isn't something we take seriously?
>> If there is a measurable performance impact to the order of -I option
>> on the compiler command line, we should approach this programmatically.
>> 
> 
> I think the intent of this was to deal with duplicates include file names, and it was not really about build performance. 
> 

Whoops. Sorry I got a new Mac and for the 1st time in 12 years I did not migrate. So my scroll direction got reversed, and that reversed the order of mails in my email client and my brain has not adjusted yet. 

Thanks,

Andrew Fish

> Thanks,
> 
> Andrew Fish
> 
>> 3_edk_ii_inf_file_format/37_[packages]_sections.md | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>> 
>> diff --git a/3_edk_ii_inf_file_format/37_[packages]_sections.md b/3_edk_ii_inf_file_format/37_[packages]_sections.md
>> index 17a8d91..c09112b 100644
>> --- a/3_edk_ii_inf_file_format/37_[packages]_sections.md
>> +++ b/3_edk_ii_inf_file_format/37_[packages]_sections.md
>> @@ -42,11 +42,8 @@ Defines the `[Packages]` section tag that is used in EDK II module INF files.
>> Each entry in this section contains a directory name, forward slash character
>> and the name of the DEC file contained in the directory name.
>> 
>> -Packages must be listed in the order that may be required for specifying
>> -include path statements for a compiler. For example, the _MdePkg/MdePkg.dec_
>> -file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file. If there
>> -are PCDs listed in the generated "As Built" INF, the packages that declare any
>> -PCDs must be listed in this section.
>> +If there are PCDs listed in the generated "As Built" INF, the packages that
>> +declare any PCDs must be listed in this section.
>> 
>> Each package filename must be listed only once per section. Package filenames
>> listed in architectural sections are not permitted to be listed in the common
>> -- 
>> 2.20.1
>> 
> 
> 
> 


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

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-02 16:20         ` Laszlo Ersek
@ 2020-06-03 11:44           ` Leif Lindholm
  2020-06-03 13:43             ` Laszlo Ersek
  0 siblings, 1 reply; 18+ messages in thread
From: Leif Lindholm @ 2020-06-03 11:44 UTC (permalink / raw)
  To: devel, lersek; +Cc: michael.d.kinney, Andrew Fish, Pankaj Bansal

On Tue, Jun 02, 2020 at 18:20:26 +0200, Laszlo Ersek wrote:
> On 06/02/20 16:20, Leif Lindholm wrote:
> > On Tue, Jun 02, 2020 at 15:29:55 +0200, Laszlo Ersek wrote:
> >> I have not been aware of the header name collision scenario (nor that
> >> the [Packages] ordering was supposed to work around such issues).
> > 
> > Nor had I...
> > 
> >> I work strictly with edk2 proper, where a name collision like this can
> >> be detected, and so should be prevented. (Insert yet another argument
> >> why keeping platform code outside of edk2 is a bad idea.) In particular,
> >> a collision between MdePkg and MdeModulePkg would be super bad.
> >>
> >> Which now seems to turn out consistent with my general review point that
> >> the [Packages] section, like (almost) all other INF file sections,
> >> should be sorted lexicographically.
> >>
> >> How about replacing
> >>
> >> """
> >> Packages must be listed in the order that may be required for specifying
> >> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
> >> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
> >> """
> >>
> >> with
> >>
> >> """
> >> The order in which packages are listed may be relevant. Said order
> >> specifies in what order include path statements are generated for a
> >> compiler. Normally, header file name collisions are not expected between
> >> packages -- they are forbidden in edk2 proper --, but with a module INF
> >> consuming both edk2-native and out-of-edk2 packages, header file names
> >> may collide. For setting specific include path priorities, the packages
> >> may be listed in matching order in the INF file. Listing a package
> >> earlier will cause a compiler to consider include paths from that
> >> package earlier.
> >> """
> > 
> > Could I suggest striking:
> > " -- they are forbidden in edk2 proper --, but with a module INF
> > consuming both edk2-native and out-of-edk2 packages, header file
> > names may collide"?
> 
> I'm sad; that's the part I like the most! ;) That describes the actual
> use case (I'm a fan of use case details in commit messages too).
> 
> Anyway, I don't insist...
> 
> > 
> > This document specifies a file format, not automatically edk2-related.
> 
> I disagree with this specific statement; the INF spec says "edk2" in the
> *name*. It's called "edk2 INF specification".
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications
> 
> "This page contains the released versions of the EDK II Specifications
> published using Gitbook."
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications#inf
> 
> "This document describes the EDK II build information (INF) file format."
> 
> The following link doesn't seem to load at the moment:
> 
> https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/v/release/1.27/
> 
> but checking the source in the git repo
> <https://github.com/tianocore-docs/edk2-InfSpecification>, the actual
> text seems to say "EDK II Module Information (INF) File Specification".
> 
> The whole feature is related to out-of-tree INF files (where header file
> name collisions cannot be easily detected).

My wording "edk2-related" was imprecise to the level of being
incorrect, apologies for adding confusion.

My intended point wast that here is nothing specific about colliding
with headers in the edk2 repository. This aspect relates to *all*
different repos used by a specific platform. (And given how much magic
the EDK2 build system looks to a newcomer anyway...)

> > I think we're reaching a point where a major documentation overhaul is
> > necessary. I had already been reflecting on how the coding style
> > document encompasses more than coding style (at one point it explains
> > how while() loops are different from do{}while() loops). And we
> > recently had that conversation around struct assignments which some
> > maintainers claim are banned, but which is not mentioned in that
> > document.
> > 
> > Not trying to resolve that issue *now*, just reflecting on how some
> > things have been added to these documents historically to deal with a
> > specific issue, and ended up confusing things as improved development
> > practices have made the original problem go away.
> > 
> > So with the edk2 refences removed, I like your new wording.
> 
> OK -- I won't let "perfect" get in the way of "good" :)

So, umm, given that the entire actual content of the patch would now
be written by you - could you submit possibly your own patch, and I'll
abandon this one?

I'd be happy with (or frankly, without :) a Suggested-by.

Regards,

Leif

> Thanks!
> Laszlo
> 
> 
> 
> 

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

* Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering
  2020-06-03 11:44           ` Leif Lindholm
@ 2020-06-03 13:43             ` Laszlo Ersek
  0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2020-06-03 13:43 UTC (permalink / raw)
  To: Leif Lindholm, devel; +Cc: michael.d.kinney, Andrew Fish, Pankaj Bansal

On 06/03/20 13:44, Leif Lindholm wrote:
> On Tue, Jun 02, 2020 at 18:20:26 +0200, Laszlo Ersek wrote:
>> On 06/02/20 16:20, Leif Lindholm wrote:
>>> On Tue, Jun 02, 2020 at 15:29:55 +0200, Laszlo Ersek wrote:
>>>> I have not been aware of the header name collision scenario (nor that
>>>> the [Packages] ordering was supposed to work around such issues).
>>>
>>> Nor had I...
>>>
>>>> I work strictly with edk2 proper, where a name collision like this can
>>>> be detected, and so should be prevented. (Insert yet another argument
>>>> why keeping platform code outside of edk2 is a bad idea.) In particular,
>>>> a collision between MdePkg and MdeModulePkg would be super bad.
>>>>
>>>> Which now seems to turn out consistent with my general review point that
>>>> the [Packages] section, like (almost) all other INF file sections,
>>>> should be sorted lexicographically.
>>>>
>>>> How about replacing
>>>>
>>>> """
>>>> Packages must be listed in the order that may be required for specifying
>>>> include path statements for a compiler. For example, the MdePkg/MdePkg.dec_
>>>> file must be listed before the `MdeModulePkg/MdeModulePkg.dec` file.
>>>> """
>>>>
>>>> with
>>>>
>>>> """
>>>> The order in which packages are listed may be relevant. Said order
>>>> specifies in what order include path statements are generated for a
>>>> compiler. Normally, header file name collisions are not expected between
>>>> packages -- they are forbidden in edk2 proper --, but with a module INF
>>>> consuming both edk2-native and out-of-edk2 packages, header file names
>>>> may collide. For setting specific include path priorities, the packages
>>>> may be listed in matching order in the INF file. Listing a package
>>>> earlier will cause a compiler to consider include paths from that
>>>> package earlier.
>>>> """
>>>
>>> Could I suggest striking:
>>> " -- they are forbidden in edk2 proper --, but with a module INF
>>> consuming both edk2-native and out-of-edk2 packages, header file
>>> names may collide"?
>>
>> I'm sad; that's the part I like the most! ;) That describes the actual
>> use case (I'm a fan of use case details in commit messages too).
>>
>> Anyway, I don't insist...
>>
>>>
>>> This document specifies a file format, not automatically edk2-related.
>>
>> I disagree with this specific statement; the INF spec says "edk2" in the
>> *name*. It's called "edk2 INF specification".
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications
>>
>> "This page contains the released versions of the EDK II Specifications
>> published using Gitbook."
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications#inf
>>
>> "This document describes the EDK II build information (INF) file format."
>>
>> The following link doesn't seem to load at the moment:
>>
>> https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/v/release/1.27/
>>
>> but checking the source in the git repo
>> <https://github.com/tianocore-docs/edk2-InfSpecification>, the actual
>> text seems to say "EDK II Module Information (INF) File Specification".
>>
>> The whole feature is related to out-of-tree INF files (where header file
>> name collisions cannot be easily detected).
> 
> My wording "edk2-related" was imprecise to the level of being
> incorrect, apologies for adding confusion.
> 
> My intended point wast that here is nothing specific about colliding
> with headers in the edk2 repository. This aspect relates to *all*
> different repos used by a specific platform. (And given how much magic
> the EDK2 build system looks to a newcomer anyway...)
> 
>>> I think we're reaching a point where a major documentation overhaul is
>>> necessary. I had already been reflecting on how the coding style
>>> document encompasses more than coding style (at one point it explains
>>> how while() loops are different from do{}while() loops). And we
>>> recently had that conversation around struct assignments which some
>>> maintainers claim are banned, but which is not mentioned in that
>>> document.
>>>
>>> Not trying to resolve that issue *now*, just reflecting on how some
>>> things have been added to these documents historically to deal with a
>>> specific issue, and ended up confusing things as improved development
>>> practices have made the original problem go away.
>>>
>>> So with the edk2 refences removed, I like your new wording.
>>
>> OK -- I won't let "perfect" get in the way of "good" :)
> 
> So, umm, given that the entire actual content of the patch would now
> be written by you - could you submit possibly your own patch, and I'll
> abandon this one?
> 
> I'd be happy with (or frankly, without :) a Suggested-by.

I can add this to my queue, sure. I'll get to it sometime. If it's
urgent, anyone please feel free to post the patch with the updated wording.

Thanks,
Laszlo


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

end of thread, other threads:[~2020-06-03 13:43 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-29 14:02 [PATCH edk2-InfSpecification] Drop statement on package ordering Leif Lindholm
2020-05-31 22:19 ` Michael D Kinney
2020-05-31 22:43   ` [edk2-devel] " Leif Lindholm
2020-06-01  3:39     ` Pankaj Bansal
2020-06-01  5:15       ` Michael D Kinney
2020-06-01  7:01         ` Pankaj Bansal
2020-06-01 15:31           ` Michael D Kinney
2020-06-02 13:29     ` Laszlo Ersek
2020-06-02 13:37       ` Pankaj Bansal
2020-06-02 14:22         ` Leif Lindholm
2020-06-02 16:11         ` Laszlo Ersek
2020-06-03  3:12           ` Pankaj Bansal
2020-06-02 14:20       ` Leif Lindholm
2020-06-02 16:20         ` Laszlo Ersek
2020-06-03 11:44           ` Leif Lindholm
2020-06-03 13:43             ` Laszlo Ersek
2020-06-03  3:33 ` Andrew Fish
     [not found] ` <1614EB3F428C08F5.21938@groups.io>
2020-06-03  3:41   ` [edk2-devel] " Andrew Fish

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