Re: [edk2-devel] [PATCH edk2-InfSpecification] Drop statement on package ordering

["Michael D Kinney" <michael.d.kinney@intel.com>]

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