Re: [edk2-devel] [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI

["Laszlo Ersek" <lersek@redhat.com>]

On 04/09/20 21:57, Sean via groups.io wrote:
> Ard/Laszlo,
> 
> On the topic of Package files.
> 1. The *.ci.yaml is designed to be at the root of the package and this follows all other Core ci usage.  The Core CI will only look for this file at the root as of now.

OK. This makes sense to me. Even if we wanted to put the files away
under a subdirectory, we'd have to inform the Core CI system somehow --
and that would likely take the form of having to name the subdirectory
in a particular way. Which is not different from having a fixed name
regular file in the package root dir.

Thankfully, this file does have ".ci." in the name.

> 2.  The PlatformBuild.py file is exactly like a build.bat or build.sh file found in many platform packages.  This file is relevant for local dev as it will build the package and should support the feature set of the platform.  I would hope that the package maintainers would continue to update this file to match the features of their package otherwise CI will become less and less relevant overtime.  Thus I think it belongs in the same place as build.sh.

Thank you, this explanation is very helpful!

ArmVirtPkg does not have a "build.sh" of the kind that you mention --
and I strongly prefer it that way.

OvmfPkg happens to have a "build.sh" -- and if it were up to me, I would
remove it. I have stated this before, perhaps multiple times. I strongly
prefer people invoking "build" manually, or writing their *own* scripts
for wrapping the "build" utility. I never ever use "OvmfPkg/build.sh"
myself -- it doesn't do what I want (and what I want, in this aspect,
should not be in the upstream edk2 tree).

But, again, OVMF's "build.sh" was added in commit 66325870afaf
("OvmfPkg/build.sh: Add features and replace build32/64.sh",
2011-01-09), and by the time I "arrived" and had grown a distaste for
it, other users had become dependent on it. So I can not propose its
removal. I can certainly argue against spreading the practice to ArmVirtPkg.

I *do* agree that "PlatformBuild.py" is necessary for CI, and keeping it
up-to-date is also necessary for good coverage by CI. I'm just
requesting to move it out of the package root dir.

> The only other pattern we could follow is a ".pytools" folder like for Core CI.  But again unless you strongly disagree I would prefer the first.

The project root already has a .pytool subdir, so (I think?) there would
be a precedent.

But, the leading dot (for "hiding" the subdirectory) is totally not my
point. I don't wish to hide the CI metafiles to *that* extent. I'd just
like to clearly associate them with the CI purpose -- it's fine if they
are visible when the directory is listed with default switches, but the
names should be specific.

> 3. The readme file...You tell me where?  Or it could be combined with the existing readme.md file?
> 4. The iasl ext_dep file.  I could see this moving deeper into the package.  Where would you like it? This file is an external dependency to control the version of iasl used on this platform.  It should be kept current with what the platform expects for iasl usage.
> 
> Anyway, I need more actionable feedback if you don't agree with the above.

How do you feel about the following directory structure:

  ArmVirtPkg/ArmVirtPkg.ci.yaml
  ArmVirtPkg/PlatformCI/Ubuntu-GCC5.yml
  ArmVirtPkg/PlatformCI/PlatformBuild.py
  ArmVirtPkg/PlatformCI/README-pytools.md
  ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml

(Note that I also removed the ".azurepipelines/" prefix from
"Ubuntu-GCC5.yml", in addition to pushing it under "PlatformCI/". If
".azurepipelines/" is required, then I'm fine with it, as long as we can
also keep it under "ArmVirtPkg/PlatformCI/".)

The result would be that someone running "ls" or "dir" in ArmVirtPkg
would see two CI-related entries: the "ArmVirtPkg.ci.yaml" control file,
and the "PlatformCI" directory. Both entries clearly state "CI" in the
name, and separate CI objects from the rest of the package.

Thanks!
Laszlo