From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.19354.1586971607410619572 for ; Wed, 15 Apr 2020 10:26:47 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C65141FB; Wed, 15 Apr 2020 10:26:45 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F349A3F68F; Wed, 15 Apr 2020 10:26:44 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI To: Laszlo Ersek , devel@edk2.groups.io, sean.brogan@microsoft.com References: <3dcff902-cd2a-2189-245d-84548b2c7da0@arm.com> <12347.1586462270854537334@groups.io> <3e0cfe33-c972-9bdd-5db5-25c52a720823@redhat.com> From: "Ard Biesheuvel" Message-ID: <422ba439-cb7c-a6d0-b8ab-84b4a63900c8@arm.com> Date: Wed, 15 Apr 2020 19:26:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <3e0cfe33-c972-9bdd-5db5-25c52a720823@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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.=C2=A0 The Core CI will only look for th= is file at the root as of now. >=20 > 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. >=20 > Thankfully, this file does have ".ci." in the name. >=20 >> 2.=C2=A0 The PlatformBuild.py file is exactly like a build.bat or buil= d.sh file found in many platform packages.=C2=A0 This file is relevant fo= r local dev as it will build the package and should support the feature s= et of the platform.=C2=A0 I would hope that the package maintainers would= continue to update this file to match the features of their package othe= rwise CI will become less and less relevant overtime.=C2=A0 Thus I think = it belongs in the same place as build.sh. >=20 > Thank you, this explanation is very helpful! >=20 > ArmVirtPkg does not have a "build.sh" of the kind that you mention -- > and I strongly prefer it that way. >=20 Agreed. I never added one because I never felt the need for it. 'build'=20 has a standard interface across platforms and works in a uniform way.=20 build.sh are bespoke helpers, and so I don't think they should have been=20 added in the first place. > OvmfPkg happens to have a "build.sh" -- and if it were up to me, I woul= d > remove it. I have stated this before, perhaps multiple times. I strongl= y > 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). >=20 > 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 ArmVir= tPkg. >=20 > I *do* agree that "PlatformBuild.py" is necessary for CI, and keeping i= t > up-to-date is also necessary for good coverage by CI. I'm just > requesting to move it out of the package root dir. >=20 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.=C2=A0 But again unless you strongly disagree I would prefer the= first. >=20 > The project root already has a .pytool subdir, so (I think?) there woul= d > be a precedent. >=20 > 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. >=20 >> 3. The readme file...You tell me where?=C2=A0 Or it could be combined = with the existing readme.md file? >> 4. The iasl ext_dep file.=C2=A0 I could see this moving deeper into th= e package.=C2=A0 Where would you like it? This file is an external depend= ency to control the version of iasl used on this platform.=C2=A0 It shoul= d be kept current with what the platform expects for iasl usage. >> >> Anyway, I need more actionable feedback if you don't agree with the ab= ove. >=20 > How do you feel about the following directory structure: >=20 > ArmVirtPkg/ArmVirtPkg.ci.yaml > ArmVirtPkg/PlatformCI/Ubuntu-GCC5.yml > ArmVirtPkg/PlatformCI/PlatformBuild.py > ArmVirtPkg/PlatformCI/README-pytools.md > ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml >=20 This looks good to me, and could be used for other packages as well, afai= ct. --=20 Ard.