Hi Jiewen, thank you for looking into this. I'm replying below quite at length; please bear with me. At the end I'm going to propose a new alternative to the PCD:
On 09/16/17 01:58, Yao, Jiewen wrote: > Hi Laszlo/Ruiyu/Star > I went through the whole conversation and have some thought: > > For staging, I quote the description from > https://github.com/tianocore/edk2-staging > > I think UDF matches all below criteria. "edk2 required quality criteria" is > the key. > > We have lots of features there - such as StandaloneSmm, CustomizedSecureBoot, > RiscV, ResetSystem, StructurePcd. > > All those features need more validation. > We are following the rule and I think UDF feature can follow the same rule. > > If there is quality concern and the quality concern cannot be resolved in a > short period of time, the staging tree is a better choice. > (NOTE: "Fix it immediately" is not an alternative; we can't do that.) Moving the UDF feature, including the PartitionDxe changes, to edk2-staging would require the following: (1) A maintainer that owns and maintains the UDF branch. - I definitely cannot be this person, first because I have no UDF knowledge, and second because I have no time for any more maintainer responsibilities than I currently have. Again, my interest in UDF is *only* as a user. - I assume Ray will also have no time for maintaining a UDF feature branch in edk2-staging. - If Paulo is willing to become a maintainer for the UDF branch in edk2-staging, that's good. Assuming he is *allowed* to become such a maintainer. (According to the edk2-staging description, periodic rebases to edk2/master are required, hence Paulo would be required to dedicate time to maintaining "edk2-staging/udf" on an on-going basis.) (2) The UDF-related commits would have to be reverted from edk2, and applied to edk2-staging/udf. The first part (the reverts) would be bad. The commits that have gone into UDF support thus far -- namely the build error fixes from everyone, and there's also a pending style improvement series from Hao Wu -- have already improved UDF a lot, and would have never happened in the staging repository. Furthermore, let me explain how most end-users (upstream users) consume OVMF: - edk2 maintainers push patches to edk2/master, - various people (Ard and others from Linaro, my colleague Gerd Hoffmann on his own time, probably others) operate build bots / CI environments that fetch edk2 master every day, and build RPM, DEB etc. packages for OVMF - end-users set up YUM or APT repositories on their machines, pointing to these packages, and periodically update their OVMF packages. This means we have a significant user base. If we break something in edk2/master, the build bots report build failures, and the users report functional failures. This is good for edk2/master -- not just for OvmfPkg, but for many other modules built into the OVMF binary. If we move UDF to edk2-staging/udf, then UDF support will never reach these end-users. The people that operate the CI environments will not repeatedly merge edk2-staging/udf into edk2/master on their end, just so they can build UDF support into OVMF. Such merges can even fail, if rebasing the edk2-staging/udf branch to the current edk2/master branch requires *manual* conflict resolution, and the edk2-staging/udf maintainer is busy. --*-- I understand that you guys absolutely don't want a new FeaturePCD. How about the following alternative. When the edk2-staging repository was initially discussed, I mentioned to Mike that Staging or Experimental directories would also be a possibility, inside edk2/master. So the below is a variant of that idea: (a) In the "MdeModulePkg/Universal/Disk/PartitionDxe" directory, create another directory called "Experimental". (b) Move the "Udf.c" source file to: MdeModulePkg/Universal/Disk/PartitionDxe/Experimental/Udf.c (c) Create a file called "Stubs.c": MdeModulePkg/Universal/Disk/PartitionDxe/Stubs.c This file would contain an implementation of PartitionInstallUdfChildHandles() that returned EFI_NOT_FOUND *unconditionally*, and did nothing else. (d) In the "PartitionDxe.inf" file, replace "Udf.c" with "Stubs.c", under [Sources]. (e) Create "PartitionExperimentalDxe.inf" in the same directory where "PartitionDxe.inf" is: MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf MdeModulePkg/Universal/Disk/PartitionDxe/PartitionExperimentalDxe.inf The second INF file would be identical to "PartitionDxe.inf", except it would include "Experimental/Udf.c", rather than "Stubs.c". The end result is that the PartitionDxe driver is built with just a UDF stub function that does nothing, and the PartitionExperimentalDxe driver is built with UDF support. The current regression would be fixed for all platforms immediately that use MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf No new PCD for MdeModulePkg, just a new INF file, and a stub file. In turn, OvmfPkg, ArmVirtPkg, and Nt32Pkg could accept -D UDF_ENABLE, and select the proper INF file, based on UDF_ENABLE. The -D UDF_ENABLE flag would also be very easy to add, for the people that operate the CI environments / build bots. If this were acceptable, I'd be glad to send patches for it. If it's not acceptable, then I think we should simply revert the UDF patches from edk2, and *not* bother about creating an edk2-staging/udf branch. I don't believe that CI operators and end-users would care about edk2-staging/udf, even if Paulo spent the time and rebased that branch to edk2/master very frequently. (Above, you mentioned the staging branches StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem, StructurePcd -- and those are branches that *I* don't know about.) So, can we do "PartitionExperimentalDxe.inf"? Thanks! Laszlo > ======================================================== > This repository is used by EDK II as a staging location for new > features that are not yet ready for inclusion in EDK II. > > Introduction > ================= > Need place on tianocore.org where new features that are not ready for product > integration can be checked in for evaluation by the EDK II community prior to > adding to the edk2 trunk. This serves several purposes: > > * Encourage source code to be shared earlier in the development process > * Allow source code to be shared that does not yet meet all edk2 required > quality criteria > * Allow source code to be shared so the EDK II community can help finish and > validate new features > * Provide a location to hold new features until they are deemed ready for > product integration > * Provide a location to hold new features until there is a natural point in > edk2 release cycle to fully validate the new feature. > > > > From: edk2-devel [mailto:[email protected]] On Behalf Of Yao, > Jiewen > Sent: Saturday, September 16, 2017 7:39 AM > To: Laszlo Ersek <[email protected]>; Ni, Ruiyu <[email protected]>; Paulo > Alcantara <[email protected]> > Cc: Wu, Hao A <[email protected]>; [email protected]; Zeng, Star > <[email protected]> > Subject: Re: [edk2] Functionality issues in UDF support > > Hi Laszlo and Ruiyu > I can think 1 possible alternative, for your consideration only. > > 1) Move the feature to OvmfPkg. > > As such, it won't block us at this moment. > > Once the UDF solution has good quality, we can move it back to MdeModulePkg. > > Thank you > Yao Jiewen > > From: Laszlo Ersek [mailto:[email protected]] > Sent: Saturday, September 16, 2017 12:51 AM > To: Ni, Ruiyu <[email protected]<mailto:[email protected]>>; Paulo > Alcantara <[email protected]<mailto:[email protected]>> > Cc: Yao, Jiewen <[email protected]<mailto:[email protected]>>; Wu, Hao > A <[email protected]<mailto:[email protected]>>; > [email protected]<mailto:[email protected]>; Zeng, Star > <[email protected]<mailto:[email protected]>> > Subject: Re: [edk2] Functionality issues in UDF support > > On 09/15/17 18:40, Ni, Ruiyu wrote: >> Laszlo, >> Please do not add a PCD for this. Too many PCDs are no good to the project. > > I understand that new MdeModulePkg PCDs are not liked, but what do you > propose instead? If we simply revert the PartitionDxe changes, then > people that want to experiment with general UDF support under OVMF won't > be able to do that at all. > > I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and > Nt32Pkg, which would control both the FeaturePCD and the inclusion of > UdfDxe in the build. If you disagree with the FeaturePCD, I can stop > working on this, but I don't know what the alternative is. "Fix it > immediately" is not an alternative; we can't do that. If you want to > revert the change, it's your prerogative, but that will prevent > everybody from testing gradual UDF improvements. (No 3rd parties build > OVMF from any staging branches, so if the feature is only available on a > staging branch, it might as well not exist, for the outside world.) > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > [email protected]<mailto:[email protected]> > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
