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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 4/15/20 7:18 PM, Laszlo Ersek wrote:
> 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.
> 

Agreed. I never added one because I never felt the need for it. 'build' 
has a standard interface across platforms and works in a uniform way. 
build.sh are bespoke helpers, and so I don't think they should have been 
added in the first place.

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

Agreed. And if it has a uniform interface across packages, even better.

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

This looks good to me, and could be used for other packages as well, afaict.

-- 
Ard.