[Javagedes]

This PR adds an additional check to PR validation that will compare the PR branch size against the target branch size when compiled into patina-dxe-core-qemu. In conceptual terms, this is what happens:

patina

1. Checkout top of tree main for patina-dxe-core-qemu
2. Compile both platforms (debug & release) with PR branch patina patched in
3. Compile both platforms (debug & release) with target branch patina patched in
4. Compare sizes

patina-dxe-core-qemu

1. Checkout patina-dxe-core-qemu
2. Compile both platforms (debug & release) with PR branch of patina-dxe-core
3. Compile both platforms (debug & release) with target branch patina-dxe-core
4. Compare sizes.

In reality, what actually happens is a bit more complicated. The PR branch binaries were already being compiled due to actually patching them into patina-qemu to do a boot. So, a small change was made to produce the 4 compiled binaries as artifacts. Next, a new job was added that would compile the target branch of the PR and produce those 4 compiled binaries as artifacts. Finally, the PatinaQemuPrValidationPost.yml workflow was updated to download all 8 binaries and generate a summary table of sizes comparisons in the PR comment it created / updated.

Example patina run: Javagedes/patina#15
Example patina-dxe-core run: Javagedes/patina-dxe-core-qemu#1

Screenshot example of summary

[os-d]

This is to bring the data presented here: OpenDevicePartnership/patina-dxe-core-qemu#237 (comment) to patina itself? Should we align with that? Is it possible to get at least some of the data from the QEMU validation we already run? It would be nice to avoid duplicating builds in CI.

[Javagedes]

This is to bring the data presented here: OpenDevicePartnership/patina-dxe-core-qemu#237 (comment) to patina itself? Should we align with that? Is it possible to get at least some of the data from the QEMU validation we already run? It would be nice to avoid duplicating builds in CI.

Yes the intent is to show change in size as we actively make changes into patina, not when we integrate patina. I did forgot this existed though.

I'll look into reusability between the two but they are fairly different even though they somewhat do the same thing. The workflow you linked checks out two commits and compiles them and compares sizes. My workflow must checkout a single commit and patch in patina.

[os-d]

This is to bring the data presented here: OpenDevicePartnership/patina-dxe-core-qemu#237 (comment) to patina itself? Should we align with that? Is it possible to get at least some of the data from the QEMU validation we already run? It would be nice to avoid duplicating builds in CI.

Yes the intent is to show change in size as we actively make changes into patina, not when we integrate patina. I did forgot this existed though.

I'll look into reusability between the two but they are fairly different even though they somewhat do the same thing. The workflow you linked checks out two commits and compiles them and compares sizes. My workflow must checkout a single commit and patch in patina.

More that we could drop the other one if we have this. At least until we have other development work in patina-qemu but it really is just taking in patina changes.

But my question on the QEMU running validation, we already are building each flavor of patina-qemu with the changes, can this workflow just build main and compare against artifacts from there?

[Javagedes]

This is to bring the data presented here: OpenDevicePartnership/patina-dxe-core-qemu#237 (comment) to patina itself? Should we align with that? Is it possible to get at least some of the data from the QEMU validation we already run? It would be nice to avoid duplicating builds in CI.

Yes the intent is to show change in size as we actively make changes into patina, not when we integrate patina. I did forgot this existed though.

I'll look into reusability between the two but they are fairly different even though they somewhat do the same thing. The workflow you linked checks out two commits and compiles them and compares sizes. My workflow must checkout a single commit and patch in patina.

More that we could drop the other one if we have this. At least until we have other development work in patina-qemu but it really is just taking in patina changes.

But my question on the QEMU running validation, we already are building each flavor of patina-qemu with the changes, can this workflow just build main and compare against artifacts from there?

Yes I can look into that. Compiling all 8 flavors takes about 4 minutes whereas PR validation takes about 6. I can just compile the 4 I need and get the other 4 from PR validation and do the post when the all 8 are available. It should save a bit on runners and not extend the overall PR CI time.

[Javagedes]

Moving to draft while I integrate this directly into PR validation

[makubacki]

Two things should ideally happen that sound like they are in progress:

1. Consolidate/reuse in patina-dxe-core-qemu so two separate implementations are not doing the same thing. I don't think we should drop the size report there. It is useful to know how changes in that repo impact the binary built there.
2. Reuse existing builds/infrastructure in patina where possible. Simply minimize redundant building of QEMU binaries.

Also, I assume you're doing this, but combine into Patina QEMU PR Validation's display comment to reduce patina PR comment noise.

[Javagedes]

Two things should ideally happen that sound like they are in progress:

1. Consolidate/reuse in patina-dxe-core-qemu so two separate implementations are not doing the same thing. I don't think we should drop the size report there. It is useful to know how changes in that repo impact the binary built there.
2. Reuse existing builds/infrastructure in patina where possible. Simply minimize redundant building of QEMU binaries.

Also, I assume you're doing this, but combine into Patina QEMU PR Validation's display comment to reduce patina PR comment noise.

I'm not sure how realistic (1) is because while they both do a size comparison, they do it in ways that have basically no overlap

patina-dxe-core-qemu workflow

1. Checkout patina-dxe-core-qemu PR target sha
2. Compile patina-dxe-core-qemu
3. Check out patina-dxe-core-qemu PR HEAD sha
4. Compile patina-dxe-core-qemu

patina workflow

1. Checkout patina-dxe-core-qemu default branch top of tree sha
2. Checkout patina PR target sha
3. Patch patina-dxe-core-qemu with patina and compile
4. Checkout patina PR HEAD sha
5. Patch patina-dxe-core-qemu with patina and compile

I will definitely investigate the possibility, but it may be simpler to leave them as two separate PRs then attempt to support both in a single workflow.

I agree with your other comments though.

[makubacki]

Two things should ideally happen that sound like they are in progress:

1. Consolidate/reuse in patina-dxe-core-qemu so two separate implementations are not doing the same thing. I don't think we should drop the size report there. It is useful to know how changes in that repo impact the binary built there.
2. Reuse existing builds/infrastructure in patina where possible. Simply minimize redundant building of QEMU binaries.

Also, I assume you're doing this, but combine into Patina QEMU PR Validation's display comment to reduce patina PR comment noise.

I'm not sure how realistic (1) is because while they both do a size comparison, they do it in ways that have basically no overlap

I just mean they be organized together in whatever meaningful way as the "size reporting workflow(s)" with common/unique code accounted for in patina-devops for visibility (since that's a goal for patina-devops). Even if that means a large portion of branch management logic differs for each repo. At a high-level, both check out a patina-dxe-core-qemu rev, compile patina-dxe-core-qemu binaries, with an optional patina rev patch, so there might be room for consolidation, but it doesn't need to be forced.

[Javagedes]

@os-d @makubacki PR has been updated to integrate changes directly into existing PR Validation jobs. It works for both patina and patina-dxe-core-qemu which means we can remove the existing workflow from patina-dxe-core-qemu