Re: 回复: [edk2-devel] A proposal to reduce incompatible case

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

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

Hi Zhiguang,

I do not think generating these lists of lib mapping only from evaluation of the edk2 and edk2-platforms DSC usage is correct.

The only way to know for sure that there is a default mapping is if the design and implementation of the library class intended only a single mapping.
This would have to be documented in the lib class and the lib instance.

An example where this will have the incorrect behavior is a platform specific library class where a only a template of that lib class is provided
as a *Null instance.  The expectation is that a platform that needs this feature would be required to provide their own instance of that
library class in their platform DSC.  There may be features that are not used by the platforms in edk2-platforms.  In that case your
tool would identify the *Null instance as the only one.  All platforms would build, but would not have the expected behavior.  It
would be better for platforms to break at build time with a missing library mapping in this case.

There are a few categories of libraries

  1.  *Null library instances that are a template for a platform specific lib.  Also used for package verification builds.
  2.  Library class where different library instances are expected to be used in a single platform.
  3.  Library class with a single lib instance and all platforms are expected to use that one library instance.

I like the idea that packages providing a DSC INC file with default mappings/settings, but we need to make sure it is always correct
and should only be applied to category (3).  This may require manual analysis in each package or extra information added to
lib class/lib instance.

Thanks,

Mike

From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Sunday, November 29, 2020 4:32 AM
To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot <hot.tian@intel.com>; 'Bret Barkelew' <bret@corthon.com>
Cc: Bi, Dandan <dandan.bi@intel.com>; 'Chao Zhang' <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; 'Liming Gao' <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; 'Andrew Fish' <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; 'Bret Barkelew' <brbarkel@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>; debtech@gmail.com; awarkentin@vmware.com; michael.kubacki@outlook.com
Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case


Hi all,



I write a python script to do the work, including classify all the library instance, generating the including lib file.

In the attachment file, the first sheet includes all "Single-instance", which means in edk2 and edk2-platforms repo, only one inf files has the specified library name.
The second sheet includes all "Multi-Single-instance", which means in edk2 and edk2-platforms repo, for a specified module type and arch, this inf is a "Single-instance".

I think the library instance in the first two sheet can be extracted as a XXXLibs.dsc.inc



Here is the draft code

https://github.com/LiuZhiguang001/edk2/commits/extract_lib



Here is the source code of this tool

https://github.com/LiuZhiguang001/MyTool/tree/extract_lib



Please review the excel and the draft code.

If no comments, I will send out a formal patch next week.



Thanks

Zhiguang



> -----Original Message-----

> From: Liu, Zhiguang

> Sent: Saturday, November 21, 2020 2:08 PM

> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Ard Biesheuvel

> <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>;

> devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Feng, Bob C

> <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>; 'Bret Barkelew'

> <bret@corthon.com<mailto:bret@corthon.com>>

> Cc: Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang'

> <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A

> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L

> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray

> <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,

> Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; debtech@gmail.com<mailto:debtech@gmail.com>;

> awarkentin@vmware.com<mailto:awarkentin@vmware.com>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>

> Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case

>

> Hi all,

> Thanks all for the comments.

>

> Hi Jiewen:

> I think we are thinking from the different aspects.

> I want the XXPkgLib.dsc.inc to specify the default library instance that the

> package will consumes.

> You may want it to specify the default library instance that the package will

> produce.

> I now think your way makes more sense, and also align with the implement in

> NetworkPkg.

> I will follow your way when working on the demo code.

>

> Hi Bret:

> I see you point about that platform should be like platform and should only

> change in the stable tag.

> I partly agree with you, but in fact there are some projects need to keep the

> Edk2 updated frequently.

> And my proposal should also be an optional way to add the library instance.

> Platform dsc can also choose the original way. I think it is at least harmless if

> some platforms choose not to use this way.

>

> Hi Lazlo:

> I agree with you that this proposal should only extract "Single-instance".

> After all, this proposal is meant to reduce incompatible case like this

> VaribalePolicyLib.

> I want to the platform to be "Seamless update" in such case.

> However, if this lib has two instances, it is a platform's choice and platform

> owner should be aware the change.

> Therefore, "Single-instance" and "Multiple-instance" should be totally different

> case, and we should only enable this proposal to "Single-instance".

>

> Hi Liming and Ard,

> Thanks for liking this idea.

> I plan to work on the demo code next week and send it here for more

> comments.

>

> BTW, about the file name, I want to it to follow the existing rule of NetworkPkg

> and ArmVirtPkg.

> I think for MdeModulePkg, "MdeModuleLibs.dsc.inc" will be better.

>

> Thanks

> Zhiguang

>

> > -----Original Message-----

> > From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>

> > Sent: Friday, November 20, 2020 7:18 PM

> > To: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming

> > <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen

> > <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;

> > michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>; awarkentin@vmware.com<mailto:awarkentin@vmware.com>;

> debtech@gmail.com<mailto:debtech@gmail.com>;

> > Feng, Bob C <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>

> > Cc: 'Bret Barkelew' <bret@corthon.com<mailto:bret@corthon.com>>; Bi, Dandan

> > <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang' <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang,

> > Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;

> > 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L

> > <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray

> > <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,

> > Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>

> > Subject: Re: 回复: [edk2-devel] A proposal to reduce incompatible case

> >

> > On 11/20/20 10:02, Ard Biesheuvel wrote:

> > > On 11/20/20 8:27 AM, gaoliming wrote:

> > >> Zhiguang:

> > >>    This proposal can reduce the potential library class dependency.

> > >> Each package has its xxxPkgLib.dsc.inc file that includes the

> > >> library instances from this package.

> > >>    Platform DSC can include the required package lib.dsc.inc file

> > >> for the library instances. Can you work out the code changes to

> > >> demonstrate this idea?

> > >>

> > >

> > > +1 for this idea. This would allow us to remove a *lot* of

> > > +boilerplate

> > > in the .DSC files, and focus on the libraries that actually matter

> > > for the platform at hand.

> >

> > I feel like I'm in *cautious* agreement with this idea.

> >

> > In particular, I'd *only* like those lib class -> lib instance

> > resolutions to be extracted, to DSC include files, that *cannot*

> > involve platform choice. To put it differently, single-instance lib classes.

> >

> > ("Single-instance" can be interpreted per module type / per

> > architecture of course -- so if a lib class has exactly 1 instance for

> > PEIMs and exactly 1

> > (different) instance for DXE_DRIVERs, then those resolutions qualify

> > for

> > extraction.)

> >

> > If a library class genuinely has multiple instances in core edk2, then

> > extracting a "default" resolution would be *wrong*, in my opinion.

> > Every platform needs to make the choice explicitly, in such cases.

> > This applies even if a lib class only has two instances, a functional

> > one, and a Null one. The functional one should not be assumed default.

> >

> > Example: extracting the UefiLib resolution is valid. Extracting a

> > SerialPortLib class resolution is invalid.

> >

> > Basically, I'd like to avoid *overrides*. Overrides are terribly hard

> > to reason about.

> >

> > ... If someone wants to work on this, they'll have to implement this

> > with *super

> > small* patches, where every patch extracts resolutions for just one

> > lib class. I would reject any patch that required me to review the

> > extraction of two or more lib classes, from an OVMF or ArmVirt DSC file, at

> the same time.

> >

> > There is big potential in this proposal for making platform DSC files

> > *permanently* more difficult to understand & analyze. I don't

> > immediately see this proposal as a good solution for avoiding the

> > current kind of platform DSC breakage. Platform CI would be much

> > better, in my opinion. The proposal does seem workable, but the

> implementation needs to be surgical, IMO.

> >

> > OK, I'll get off my soap box now.

> >

> > Thanks

> > Laszlo



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