From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.19168.1586971106978483975 for ; Wed, 15 Apr 2020 10:18:27 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=h8kbnUeQ; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586971106; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gq/Z15R9DehPWvLt8OC4m9b1aeaevYxmHwTl+PlZrio=; b=h8kbnUeQ5MYQml8BERvl+/kyk0M0202Ruxrnlzz4YppKDXd2XtYvIouE4qOBHy/Z6kOYS5 4rOwnF8taFaoGXOvHkrzgMGjGmjlQP2izcl0HoK2Wi4yF9SDNTctl1idhG/+So2GnzZEsW 2HaxxBjg2pnRwGG/T0S+TRlSWKwPs0k= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-227-Rcv67zpuPpexN2BDcMFXyA-1; Wed, 15 Apr 2020 13:18:19 -0400 X-MC-Unique: Rcv67zpuPpexN2BDcMFXyA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3EC2A8018A5; Wed, 15 Apr 2020 17:18:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-155.ams2.redhat.com [10.36.112.155]) by smtp.corp.redhat.com (Postfix) with ESMTP id 27E58D7664; Wed, 15 Apr 2020 17:18:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI To: devel@edk2.groups.io, sean.brogan@microsoft.com, Ard Biesheuvel References: <3dcff902-cd2a-2189-245d-84548b2c7da0@arm.com> <12347.1586462270854537334@groups.io> From: "Laszlo Ersek" Message-ID: <3e0cfe33-c972-9bdd-5db5-25c52a720823@redhat.com> Date: Wed, 15 Apr 2020 19:18:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <12347.1586462270854537334@groups.io> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/09/20 21:57, Sean via groups.io wrote: > Ard/Laszlo, >=20 > On the topic of Package files. > 1. The *.ci.yaml is designed to be at the root of the package and this fo= llows all other Core ci usage.=C2=A0 The Core CI will only look for this fi= le 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.=C2=A0 The PlatformBuild.py file is exactly like a build.bat or build.s= h file found in many platform packages.=C2=A0 This file is relevant for loc= al dev as it will build the package and should support the feature set of t= he platform.=C2=A0 I would hope that the package maintainers would continue= to update this file to match the features of their package otherwise CI wi= ll become less and less relevant overtime.=C2=A0 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 Co= re CI.=C2=A0 But again unless you strongly disagree I would prefer the firs= t. 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?=C2=A0 Or it could be combined wit= h the existing readme.md file? > 4. The iasl ext_dep file.=C2=A0 I could see this moving deeper into the p= ackage.=C2=A0 Where would you like it? This file is an external dependency = to control the version of iasl used on this platform.=C2=A0 It should be ke= pt current with what the platform expects for iasl usage. >=20 > 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