Re: [PATCH] Features/Intel/Readme.md: Document meaning of "Complete" for features

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Thursday, March 19, 2020 7:57 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Chan, Amy <amy.chan@intel.com>;
> Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: RE: [PATCH] Features/Intel/Readme.md: Document meaning of
> "Complete" for features
> 
> I agree that I cannot think of a good reason that the interface would be in
> feature packages and the only use and implementation be in board packages.

Thanks for acknowledging the change.
I want to have this clearly documented because I did see such try from developers.

I will post a formal V2 patch.

> 
> With respect to fine grain binary modularity, I don't have strong data or a
> strong intuition as to why attempts at driver level modularity have not
> worked well.  My intuitions say that it is something like:  we haven't found
> the right use cases, binary re-use of stable code isn't valuable enough, and if
> features are too small it is too complicated to use effectively.
> I think that we have emerging use cases around build time, partial updates,
> and firmware scaling.  By scaling I mean that firmware continues to grow and
> to control the impacts of growth, it is often nice to break things into smaller
> pieces that evolve more independently.  To be clear, in this context I mean
> breaking the monolithic thing into smaller pieces.  My focus is on useful FV
> full of related features.  I hope we can reduce visible interdependencies, get
> build time benefits, and eventually validation and update benefits.  It
> remains to be proven though.

I totally agree with you.

> 
> With respect to packages vs directories, I concur that packaging has some
> advantages.  I am just skeptical that the cost is justified without realizing
> more developer value for the change.
> 
> With respect to AdvancedFeaturePackage abstracting future change.  My
> request is obtain wide adoption before impacting existing consumers.

Thanks for the comments.
We will balance between consolidating common code and customer impact.

> 
> Regards,
> Isaac
> 
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, March 12, 2020 8:21 PM
> To: Oram, Isaac W <isaac.w.oram@intel.com>; Ray Ni
> <niruiyu@users.noreply.github.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Chan, Amy <amy.chan@intel.com>;
> Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: RE: [PATCH] Features/Intel/Readme.md: Document meaning of
> "Complete" for features
> 
> Isaac,
> Thanks for the comments. Reply in below.
> 
> > -----Original Message-----
> > From: Oram, Isaac W <isaac.w.oram@intel.com>
> > Sent: Thursday, March 12, 2020 11:29 PM
> > To: Ray Ni <niruiyu@users.noreply.github.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>
> > Subject: RE: [PATCH] Features/Intel/Readme.md: Document meaning of
> > "Complete" for features
> >
> > Ray,
> >
> > I don't think that this is a desirable rule.
> >
> > I want to create feature packages that bundle frequently used together
> > existing capabilities.  See the NetworkFeaturePkg for an example.  I
> > also want to make feature packages for the USB stack, debug capabilities,
> and the like that are often aggregations of existing modules.
> 
> Thanks for reminding me the NetworkFeaturePkg case. NetworkFeaturePkg
> is a valid case.
> I want to add this rule to avoid creating a feature package that only contains
> header files, but the implementations are in each Board package. Do you
> agree this should be avoided?
> How about:
> "A feature package must not contain only interfaces which are implemented
> by board source code packages."
> 
> >
> > The Minimum Platform Architecture spec targets advanced features that
> > are easy to enable for relatively inexperienced developers.  One way
> > of doing that is to leverage the UEFI PI arch and its binary component
> support features.  The Minimum Platform Architecture aims to use this to
> enable a use case leveraging Firmware Volumes that looks like:
> > 1:  Build NetworkFeaturePkg (this produces an FV, customized via PCD
> > and/or static libraries as needed)
> > 2:  Load FV (from shell, by injecting into an existing image using
> > FMMT, Fiano, etc)
> > 3:  Use network features and functionality
> >
> > The model where the only way people extend a UEFI firmware image is by
> > rebuilding a complete solution needs to end.  It is a misuse of the
> > architecture in my estimation.  We have not had much success with fine
> > granularity component binary use, i.e. individual PEIM and drivers.
> > Perhaps there is too much expertise needed.  Minimum Platform
> Architecture and Advanced Features aim to improve this by enabling larger
> granularity binary components that require less UEFI knowledge to use
> effectively.
> 
> Is your concern that binary modularity may be not always practical today? If
> that's it, I agree with your concerns.
> I do find that /Features/Intel/Debugging/Usb3DebugFeaturePkg only
> contains library. I think the goal is binary modularity. Before that, source
> modularity is the bottom-line requirement for each feature package.
> 
> >
> > I recognize that there is a competing vision that wants to make many
> > small feature packages that are easy to build in or out based on
> > simple PCD feature flags.  As that may improve developer's experience,
> > it is not something I am strongly contesting.  However, I just don't see it as
> any different than MdeModulePkg.  It is the same strategy, just using
> packages to organize instead of directories.
> 
> The key difference I can see between package and module is that package
> groups the module and the accordingly public interfaces together. While if
> putting lots of modules inside a combo package, all the public interfaces (like
> header files) are together and it's hard to tell which interfaces are used by
> which modules.
> 
> >
> > The other consideration should include that we have a lot of existing
> > users.  I don't want to move existing code around to make usable
> > features.  If we move existing code to create the feature in the first place,
> we affect all the existing users, often for no immediate benefit.  If features
> become successful and widely used, then is a good time to refactor the code.
> > The difference is that at that time, the change is essentially behind
> > an abstraction and so the change doesn't cause as much pointless work.
> 
> AdvancedFeaturePkg is the abstraction layer that aims to hide the future
> changes.
> 
> >
> > Regards,
> > Isaac
> >
> >
> > -----Original Message-----
> > From: Ray Ni <niruiyu@users.noreply.github.com>
> > Sent: Thursday, March 12, 2020 5:41 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Oram, Isaac W
> > <isaac.w.oram@intel.com>
> > Subject: [PATCH] Features/Intel/Readme.md: Document meaning of
> > "Complete" for features
> >
> > Today's document doesn't forbidden creation of a feature package with
> > only interfaces and no code to implement the interfaces. Such feature
> package is useless.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Amy Chan <amy.chan@intel.com>
> > Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> > Cc: Isaac W Oram <isaac.w.oram@intel.com>
> > ---
> >  Features/Intel/Readme.md | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Features/Intel/Readme.md b/Features/Intel/Readme.md index
> > 9729f90a41..f0923e3d56 100644
> > --- a/Features/Intel/Readme.md
> > +++ b/Features/Intel/Readme.md
> > @@ -18,7 +18,7 @@ document as needed.
> >  Advanced features should be:
> >  * _Cohesive_, the feature should not contain any functionality unrelated
> to the feature.
> >  * _Complete_, the feature must have a complete design that minimizes
> > dependencies. A feature package cannot directly
> > -  depend on another feature package.
> > +  depend on another feature package. A feature package must contain
> module(s) to implement the feature interfaces.
> >  * _Easy to Integrate_, the feature should expose well-defined software
> interfaces to use and configure the feature.
> >    * It should also present a set of simple and well-documented standard
> EDK II configuration options such as PCDs to
> >    configure the feature.
> > --
> > 2.21.0.windows.1