public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Re: [edk2-devel] [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI
@ 2020-04-19  8:29 Sean
  2020-04-19  9:35 ` Ard Biesheuvel
  2020-04-19 20:56 ` Rebecca Cran
  0 siblings, 2 replies; 15+ messages in thread
From: Sean @ 2020-04-19  8:29 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: devel@edk2.groups.io, Ard Biesheuvel, Kinney, Michael D

Laszlo,

Regarding your comments about disliking the verbosity of the markdown table/html table for build status both in Core Ci and now these Platform CI readme files.  

As a learning experience I updated the OvmfPkg readme to use reStructuredText instead of markdown.  Not sure if I like RST but it does allow the links to not be in html and supports directives so you can push all that text to end of the file.  

Do you like this enough that I should rework all three readmes and we should discuss if RST should be used instead of MD for the edk2 project? 

RST version: https://github.com/spbrogan/edk2/blob/PlatformAndCoreCIForOvmfArmVirtEmulatorPackages_v8/OvmfPkg/README.rst 
MD version: https://github.com/spbrogan/edk2/blob/PlatformAndCoreCIForOvmfArmVirtEmulatorPackages_v7/OvmfPkg/README-pytools.md

This is really the last remaining issue for the PlatformCI patchset.

Thanks
Sean



-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, April 16, 2020 7:52 AM
To: Sean Brogan <sean.brogan@microsoft.com>
Cc: devel@edk2.groups.io; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI

On 04/15/20 22:38, sean.brogan via [] wrote:
> On Wed, Apr 15, 2020 at 10:18 AM, Laszlo Ersek wrote:
>
>>
>> ArmVirtPkg/ArmVirtPkg.ci.yaml
>> ArmVirtPkg/PlatformCI/Ubuntu-GCC5.yml
>> ArmVirtPkg/PlatformCI/PlatformBuild.py
>> ArmVirtPkg/PlatformCI/README-pytools.md
>> ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml
>
> I am ok with the above except one thought on the readme.  One nice 
> thing about the markdown readme files are the badge shows up in github 
> when you view the package.  This is a quick and easy way to see the 
> current status.

I agree this is very useful.

What I dislike is that, when I open "Readme.md" (e.g. in the project
root) in a normal terminal, I'm greeted by a HTML tag soup under the heading "# Build Status".

Markdown is supposed to be readable as plain text. Embedding the page-ful of build status HTML in "Readme.md" defeats that purpose.
github should either consume a different file (too) for displaying status badges, or else the "Readme.md" file should reference the HTML snippet in question with some kind of link or directive, rather than directly containing it.

Perhaps github already offers this feature -- that would be awesome. I would be happy with the following variant, for example:

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

"README.md" would contain the package description that read nice in a terminal too. Then, "README.md" would either (somehow?) include "README-ci.md" by reference, or else github would render both "README-ci.md" and "README.md".

>
> * Ovmf has a pretty stale readme

Hm, I'd say "somewhat" stale. We do keep it up-to-date with "very important" stuff.

My excuse for not polishing it more -- which I honestly do believe is a
*valid* excuse -- is that users have shown repeatedly that they don't read the README at all. I've explained basic stuff like "how to capture OVMF's debug log" umpteen times on the list, despite it being spelled out in the README. The fact is that effort put towards careful documentation is almost entirely lost effort -- this was also clearly proved by the (non-)reaction that I got to my OVMF white paper that I wrote a few years back (~60 A4 pages, if I recall correctly).

Documentation is just not *worth* polishing, considering the user base as a whole. I for one go to the available documentation *before* starting to use new software, or when questions pop up, but it seems like I belong to a vanishingly small camp with that. People just flock to social media (or, in the least wrong case: they come to this mailing list), and ask questions they could already find the answers to in existent documentation.

This is a  bitter realization for me, especially having written relatively substantial articles for the edk2 wiki:

  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers&amp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd94de877b9e74e06f08208d7e215bca4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637226455298322408&amp;sdata=afDNhjdLt%2B9G4idYYGARh0RiTUnxCBx4fyBA5xT8k9Q%3D&amp;reserved=0
  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FTesting-SMM-with-QEMU%2C-KVM-and-libvirt&amp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd94de877b9e74e06f08208d7e215bca4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637226455298322408&amp;sdata=nUtyttk3BdcGKyTepkUy0OnILT7%2FcBnHCMyt5eAO%2BG8%3D&amp;reserved=0

but it is what I now believe.

> and does not take advantage of markdown.

Correct. That's not intentional; I think the README just predates the usefulness of markdown (i.e. it predates moving the project to github.com). IIRC.

> We could convert it to MD, clean up, and then merge in the content 
> from the pytools.md.  I would need help or a package maintainer to do 
> the cleanup of the readme to make sure it contained the content you 
> desired.

So my problem with this is two-fold. First, regarding just the markdown conversion, I agree it would be nice, but I don't wish to sink any work into it (see my opinion above, about polishing documentation). If someone wanted to spend time on just a structural conversion to markdown (not modifying content), I'd be OK to review that.

Second, merging (i.e., flattening) the tag soup from "README-pytools.md"
into the main package "Readme.md" is something that I'm opposed to, as it interferes with consuming the readme from a plain terminal or text editor.

> * ArmVirtPkg doesn't have a readme and this is definitely a barrier to 
> entry for the package.  I would suggest creating one and then merging 
> in the content from the pytools.md.

Creating a readme in MD format: would be nice if someone contributed that ("patches welcome" :) ).

*Merging* the HTML tag soup: please let's not do that. (I'm totally fine if it is introduced in a separate, appropriately named or located file.)

> * EmulatorPkg has one.  I would just suggest a merge but i am yet to 
> get any feedback from those maintainers.
>
> If that isn't desirable i would at least suggest we change the title 
> to just ReadMe.md so that GitHub shows it by default when the 
> PaltformCI folder is viewed form the web or in editor like vscode.

This sounds 100% viable and great to me. I didn't expect this could work! (I'm generally unaware of the readme filename patterns, and locations, that github.com recognizes; sorry about that.) Having an "unadorned" ReadMe.md file under PlatformCI is just perfect.

So if I understand correctly, we could choose:

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

Do I understand right?

Because, I'd find this great!

Thank you!
Laszlo


^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI
@ 2020-04-09  8:05 Ard Biesheuvel
  2020-04-09 19:57 ` [edk2-devel] " Sean
  0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2020-04-09  8:05 UTC (permalink / raw)
  To: michael.kubacki, devel; +Cc: Laszlo Ersek, Leif Lindholm

On 4/8/20 8:13 PM, michael.kubacki@outlook.com wrote:
> From: Sean Brogan <sean.brogan@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2570
> 
> Add new Azure Pipeline definitions to build and run ArmVirtPkg with:
>    * Ubuntu GCC5
> Add PyTool based build of ArmVirtPkg
> Add extdep for managing the iasl dependency
> Add ArmVirtPkg.ci.yaml for Core CI
> Add README-pytools for details and instructions
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Sean Brogan <sean.brogan@microsoft.com>

> ---
>   ArmVirtPkg/.azurepipelines/Ubuntu-GCC5.yml |  89 +++++++
>   ArmVirtPkg/ArmVirtPkg.ci.yaml              | 103 ++++++++
>   ArmVirtPkg/PlatformBuild.py                | 263 ++++++++++++++++++++
>   ArmVirtPkg/README-pytools.md               | 123 +++++++++
>   ArmVirtPkg/iasl_ext_dep.yaml               |  21 ++
>   5 files changed, 599 insertions(+)
> 

Hello Michael,

I agree with Laszlo's point here: it would be nice if we can keep these 
files organized a bit better, although I suppose there is already 
precent for keep <package>.ci.yaml files at the package's root level, so 
that one should stay where it is.

More comments below.

...

> diff --git a/ArmVirtPkg/README-pytools.md b/ArmVirtPkg/README-pytools.md
> new file mode 100644
> index 000000000000..ea70018e510a
> --- /dev/null
> +++ b/ArmVirtPkg/README-pytools.md
> @@ -0,0 +1,123 @@
> +# ArmVirtPkg
> +
> +This README-pytools.md summarizes the current state of Platform CI for ArmVirtPkg.  It also describes how to _build_ ArmVirtPkg using the Pytools build system.
> +
> +## Platform CI Current Status
> +
> +<table>
> +  <tr>
> +    <th>Config</th>
> +    <th colspan="3">Build & Run</th>
> +    <th>Notes</th>
> +  </tr>
> +  <tr>
> +    <th></th>
> +    <th>DEBUG</th>
> +    <th>RELEASE</th>
> +    <th>NOOPT</th>
> +    <th></th>
> +  </tr>
> +  <tr>
> +    <th colspan="5" align="left">
> +    Ubuntu 18.04 GCC5
> +    </th>
> +  </tr>
> +  <tr>
> +    <td>AARCH64</td>
> +    <td>
> +      <a  href="https://dev.azure.com/tianocore/edk2-ci-play/_build/latest?definitionId=41&branchName=master">
> +      <img src="https://dev.azure.com/tianocore/edk2-ci-play/_apis/build/status/ArmVirtPkg/ArmVirtQemu%20Ubuntu%20GCC5?branchName=master&jobName=Platform_CI&configuration=Platform_CI%20QEMU_AARCH64_DEBUG"/></a>
> +    </td>
> +    <td>
> +      <a  href="https://dev.azure.com/tianocore/edk2-ci-play/_build/latest?definitionId=41&branchName=master">
> +      <img src="https://dev.azure.com/tianocore/edk2-ci-play/_apis/build/status/ArmVirtPkg/ArmVirtQemu%20Ubuntu%20GCC5?branchName=master&jobName=Platform_CI&configuration=Platform_CI%20QEMU_AARCH64_RELEASE"/></a>
> +    </td>
> +    <td>
> +      <a  href="https://dev.azure.com/tianocore/edk2-ci-play/_build/latest?definitionId=41&branchName=master">
> +      <img src="https://dev.azure.com/tianocore/edk2-ci-play/_apis/build/status/ArmVirtPkg/ArmVirtQemu%20Ubuntu%20GCC5?branchName=master&jobName=Platform_CI&configuration=Platform_CI%20QEMU_AARCH64_NOOPT"/></a>
> +    </td>
> +    <td></td>
> +  </tr>
> +  <tr>
> +    <td>ARM</td>
> +    <td>
> +      <a  href="https://dev.azure.com/tianocore/edk2-ci-play/_build/latest?definitionId=41&branchName=master">
> +      <img src="https://dev.azure.com/tianocore/edk2-ci-play/_apis/build/status/ArmVirtPkg/ArmVirtQemu%20Ubuntu%20GCC5?branchName=master&jobName=Platform_CI&configuration=Platform_CI%20QEMU_ARM_DEBUG"/></a>
> +    </td>
> +    <td>
> +      <a  href="https://dev.azure.com/tianocore/edk2-ci-play/_build/latest?definitionId=41&branchName=master">
> +      <img src="https://dev.azure.com/tianocore/edk2-ci-play/_apis/build/status/ArmVirtPkg/ArmVirtQemu%20Ubuntu%20GCC5?branchName=master&jobName=Platform_CI&configuration=Platform_CI%20QEMU_ARM_RELEASE"/></a>
> +    </td>
> +    <td>
> +      <a  href="https://dev.azure.com/tianocore/edk2-ci-play/_build/latest?definitionId=41&branchName=master">
> +      <img src="https://dev.azure.com/tianocore/edk2-ci-play/_apis/build/status/ArmVirtPkg/ArmVirtQemu%20Ubuntu%20GCC5?branchName=master&jobName=Platform_CI&configuration=Platform_CI%20QEMU_ARM_NOOPT"/></a>
> +    </td>
> +    <td></td>
> +  </tr>
> +</table>
> +
> +### Config Details
> +
> +| Config       | Architectures      |Additional Flags |
> +| :----        | :-----             | :----           |
> +| AARCH64      | AARCH64            | None            |
> +| ARM          | ARM                | None            |
> +
> +## Setup
> +
> +### The Usual EDK2 Build Setup
> +
> +- [Python 3.8.x - Download & Install](https://www.python.org/downloads/)
> +- [GIT - Download & Install](https://git-scm.com/download/)
> +- [GIT - Configure for EDK II](https://github.com/tianocore/tianocore.github.io/wiki/Windows-systems#github-help)
> +- [QEMU - Download, Install, and add to your path](https://www.qemu.org/download/)
> +- [EDKII Source - Download/Checkout from Github](https://github.com/tianocore/tianocore.github.io/wiki/Windows-systems#download)
> +  - **NOTE:** Do _not_ follow the EDK II Compile Tools and Build instructions, see below...
> +
> +### Differences from EDK Classic Build Setup
> +
> +- Build BaseTools using "`C:\git\edk2>python BaseTools\Edk2ToolsBuild.py [-t <ToolChainTag>]`"
> +  - This replaces "`edksetup Rebuild`" from the classic build system
> +  - For Windows `<ToolChainTag>` examples, refer to [Windows ToolChain Matrix](https://github.com/tianocore/tianocore.github.io/wiki/Windows-systems-ToolChain-Matrix), defaults to `VS2017` if not specified
> +- **No Action:** Submodule initialization and manual installation/setup of NASM and iASL is **not** required, it is handled by the PyTools build system
> +
> +### Building with Pytools for ArmVirtPkg
> +
> +- Install Pytools
> +  - `pip install --upgrade -r pip-requirements.txt`
> +- Initialize & Update Submodules
> +  - `stuart_setup -c ArmVirt\PlatformBuild.py TOOL_CHAIN_TAG=<TOOL_CHAIN_TAG> -a <TARGET_ARCH>`

Typo here ^^^

Also, could we standardize on forward slashes everywhere (as in the 
examples below)?


> +- Initialize & Update Dependencies (e.g. iASL, NASM & GCC Arm/Aarch64 Compilers)
> +  - `stuart_update -c ArmVirtPkg\PlatformBuild.py TOOL_CHAIN_TAG=<TOOL_CHAIN_TAG> -a <TARGET_ARCH>`
> +- Compile (AARCH64 supported / ARM support coming soon)
> +  - `stuart_build -c ArmVirtPkg\PlatformBuild.py TOOL_CHAIN_TAG=<TOOL_CHAIN_TAG> -a <TARGET_ARCH>`
> +- Running Emulator
> +  - You can add `--FlashRom` to the end of your build command and the emulator will run after the build is complete.
> +  - or use the FlashOnly feature like `stuart_build -c ArmVirtPkg\PlatformBuild.py TOOL_CHAIN_TAG=<TOOL_CHAIN_TAG> -a <TARGET_ARCH> --FlashOnly` to just run the emulator.
> +
> +### Notes
> +
> +1. Including the expected build architecture and toolchain to the _stuart_update_ command is critical. This is because there are extra scopes and tools that will be resolved during the update step that need to match your build step.
> +2. Configuring _ACTIVE_PLATFORM_ and _TARGET_ARCH_ in Conf/target.txt is _not_ required. This environment is set by PlatformBuild.py based upon the "`[-a <TARGET_ARCH>]`" parameter.
> +3. QEMU must be on your path.  On Windows this is a manual process and not part of the QEMU installer.
> +
> +**NOTE:** Logging the execution output will be in the normal stuart log as well as to your console.
> +
> +## Custom Build Options
> +
> +**MAKE_STARTUP_NSH=TRUE** will output a _startup.nsh_ file to the location mapped as fs0. This is used in CI in combination with the --FlashOnly feature to run QEMU to the UEFI shell and then execute the contents of startup.nsh.
> +
> +**QEMU_HEADLESS=TRUE** Since CI servers run headless QEMU must be told to run with no display otherwise an error occurs. Locally you don't need to set this.
> +
> +## Passing Build Defines
> +
> +To pass build defines through _stuart_build_, prepend `BLD_*_`to the define name and pass it on the command-line. _stuart_build_ currently requires values to be assigned, so add an`=1` suffix for bare defines.
> +For example, to enable the TPM2 support, instead of the traditional "-D TPM2_ENABLE=TRUE", the stuart_build command-line would be:
> +
> +`stuart_build -c ArmVirtPkg/PlatformBuild.py BLD_*_TPM2_ENABLE=TRUE`
> +
> +## References
> +
> +- [Installing Pytools](https://github.com/tianocore/edk2-pytool-extensions/blob/master/docs/using.md#installing)
> +- For each workspace, consider creating & using a [Python Virtual Environment](https://docs.python.org/3/library/venv.html). For example <https://microsoft.github.io/mu/CodeDevelopment/prerequisites/#workspace-virtual-environment-setup-process>
> +- [stuart_build command-line parser](https://github.com/tianocore/edk2-pytool-extensions/blob/56f6a7aee09995c2f22da4765e8b0a29c1cbf5de/edk2toolext/edk2_invocable.py#L109)
> diff --git a/ArmVirtPkg/iasl_ext_dep.yaml b/ArmVirtPkg/iasl_ext_dep.yaml
> new file mode 100644
> index 000000000000..8869ed3ecef1
> --- /dev/null
> +++ b/ArmVirtPkg/iasl_ext_dep.yaml
> @@ -0,0 +1,21 @@
> +## @file
> +# Download iasl executable tool from a nuget.org package
> +# - package contains different binaries based on host
> +# Add the folder with the tool to the path
> +#
> +# This is only downloaded for scope armvirt thus
> +# should have no impact on the asl compiler used by any
> +# other platform build
> +#
> +# Copyright (c) Microsoft Corporation.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +{
> +  "id": "iasl-armvirt-1",
> +  "scope": "armvirt",
> +  "type": "nuget",
> +  "name": "iasl",
> +  "source": "https://api.nuget.org/v3/index.json",
> +  "version": "20190215.0.0",
> +  "flags": ["set_path", "host_specific"],
> +}
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-04-24 20:22 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-19  8:29 [edk2-devel] [PATCH v1 2/6] ArmVirtPkg: Add Platform CI and configuration for Core CI Sean
2020-04-19  9:35 ` Ard Biesheuvel
2020-04-20 10:30   ` Laszlo Ersek
2020-04-19 20:56 ` Rebecca Cran
2020-04-20 11:08   ` Laszlo Ersek
2020-04-24 20:22     ` [EXTERNAL] " Bret Barkelew
  -- strict thread matches above, loose matches on Subject: below --
2020-04-09  8:05 Ard Biesheuvel
2020-04-09 19:57 ` [edk2-devel] " Sean
2020-04-15  6:55   ` Sean
2020-04-15 16:57     ` Laszlo Ersek
2020-04-15 19:31       ` Sean
2020-04-16 15:12         ` Laszlo Ersek
2020-04-15 17:18   ` Laszlo Ersek
2020-04-15 17:26     ` Ard Biesheuvel
2020-04-15 20:38     ` Sean
2020-04-16 14:51       ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox