[edk2-devel] Commits Spanning Packages

[edk2-devel] Commits Spanning Packages

[Michael Kubacki]

Some commits have been spanning packages. For a monolithic upstream repo 
like edk2, this makes ingesting commits downstream more tedious if 
project requirements do not necessitate all packages to be maintained.

Below are examples of commits spanning packages. Many of these are more 
problematic because "core" packages like MdePkg, MdeModule, SecurityPkg, 
UefiCpuPkg, etc. are involved. OvmfPkg and ArmVirtPkg seem to be a 
common pairing. To focus the examples below, I excluded commits with 
that pair.

The request is to limit crossing package boundaries as much as possible 
and for maintainers to assess package impact when merging changes.

Perhaps something like PatchCheck could be updated to help identify this.

Here's some examples:

- **ArmPkg:ArmPlatformPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/103fa647d159e3d76be2634d2653c2d215dd0d46
- **ArmVirtPkg:ArmPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/34969dd260b4ecaba8d4a58d32fc07bd0fee57f9
   - Commit: 
https://github.com/tianocore/edk2/commit/7d78a86ecfc6d86244a552b1bad6fb346439116b
- **ArmVirtPkg:OvmfPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/f8d0501ded2ad426cc5c6c1237ec83fc978f8373
   - Commit: 
https://github.com/tianocore/edk2/commit/d881c6ddf556dfb5032db04733dfa86aa891e7f8
   - Commit: 
https://github.com/tianocore/edk2/commit/77e9b3a7c68cea4eeb64af85e4f695f278172314
- **ArmVirtPkg:ArmPkg:OvmfPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/9a7509e465d648314824733200566190c8814ec6
- **ArmVirtPkg:EmbeddedPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/e40fefafa90fb3a9aca77adc697c97cf6a4dd673
- **BaseTools:ArmPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/f484427d10a5ff6c2437c2f7c671e9e552ad6766
- **DynamicTablesPkg:MdePkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/4c55f6394fafe0494ec24e7c05cb68c938d7852d
- **IntelFsp2WrapperPkg:IntelFsp2Pkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/fa78edc57eb000f7820d5896f61053069b61eba1
- **MdeModulePkg:MdePkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/5443c2dc310d2c8eb15fb8eefd5057342e78cd0d
   - Commit: 
https://github.com/tianocore/edk2/commit/53eb26b238541799134aa5846530485c915735da
   - Commit: 
https://github.com/tianocore/edk2/commit/502c01c5028038e4e6b4512e9c66be0ec4d11492
- **MdeModulePkg:OvmfPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/fd306d1dbc36d10f744306e92366d73273c82c8f
- **MdeModulePkg:UefiCpuPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/e7abb94d1fb8a0e7725b983bbf5ab1334afe7ed1
   - Commit: 
https://github.com/tianocore/edk2/commit/2a09527ebcb459b40bc3661d85aa11c3905526dc
- **MdeModulePkg:MdePkg:SecurityPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/65b5dd828ef2ea5056031b239a4e7a6642f771a3
- **MdeModulePkg:UefiCpuPkg:ArmPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/0f7bccf584d93b2642c0a413a47fc821d1f5dbfd
- 
**MdeModulePkg:ArmVirtPkg:SignedCapsulePkg:EmulatorPkg:OvmfPkg:NetworkPkg:SecurityPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/2f981bddcbd6adde5f682caf0d3812ba92bc0f73
- **MdePkg:NetworkPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/671b0cea510ad6de02ee9d6dbdf8f9bbb881f35d
- **MdePkg:UefiCpuPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/8079d4dc4f8ac23e02b02996e326cac4099b3e49
   - Commit: 
https://github.com/tianocore/edk2/commit/ea55bd8f66eeca5f4e80c3679bcf1b1007286b8a
- **MdePkg:UefiPayloadPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/39f3c26e8c40e092baeb0ec4d0396498506e0a9e
- **OvmfPkg:BaseTools**
   - Commit: 
https://github.com/tianocore/edk2/commit/f0b97e165e0af79ac9eb3c6ac7697f9183afc7cb
   - Commit: 
https://github.com/tianocore/edk2/commit/ff36b2550f94dc5fac838cf298ae5a23cfddf204
- **OvmfPkg:UefiCpuPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/f220dcbba86bfc1222180c61bbd31dd6023433db
   - Commit: 
https://github.com/tianocore/edk2/commit/765ba5bf050022de8c44d93e467639d7f5fa237c
- **OvmfPkg:UefiCpuPkg::UefiPayloadPkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/a89f558d3c56d9c16e5f5b7d395c1aa36ccd38f2
- **RedfishPkg:MdePkg**
   - Commit: 
https://github.com/tianocore/edk2/commit/7275993dc64481b1c21d7d70af434bfaafe86e81

Thanks,
Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114915): https://edk2.groups.io/g/devel/message/114915
Mute This Topic: https://groups.io/mt/104081468/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Commits Spanning Packages

[Michael D Kinney]

Hi Michael,

I think a CI check for this is the best way to prevent patches with
changes to 2 or more packages.

* Use git command to retrieve list of files modified by a patch.
* Look through parent directories and collect all *.dec files
* If number of *.dec files > 1 then FAIL
* If number of *.dec files is 0 or 1, then PASS


I have also noticed that some of the Author names in patches
are devel@edk2.groups.io.  PatchCheck should be updated to
look for that Author email address and FAIL.

Best regards,

Mike

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Wednesday, January 31, 2024 10:58 AM
> To: devel@edk2.groups.io
> Cc: Andrew Fish <afish@apple.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Pierre Gondois <pierre.gondois@arm.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Abner Chang
> <abner.chang@amd.com>; Xu, Min M <min.m.xu@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Tuan Phan <tphan@ventanamicro.com>; Yu Pu
> <yu.pu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Commits Spanning Packages
> 
> Some commits have been spanning packages. For a monolithic upstream repo
> like edk2, this makes ingesting commits downstream more tedious if
> project requirements do not necessitate all packages to be maintained.
> 
> Below are examples of commits spanning packages. Many of these are more
> problematic because "core" packages like MdePkg, MdeModule, SecurityPkg,
> UefiCpuPkg, etc. are involved. OvmfPkg and ArmVirtPkg seem to be a
> common pairing. To focus the examples below, I excluded commits with
> that pair.
> 
> The request is to limit crossing package boundaries as much as possible
> and for maintainers to assess package impact when merging changes.
> 
> Perhaps something like PatchCheck could be updated to help identify
> this.
> 
> Here's some examples:
> 
> - **ArmPkg:ArmPlatformPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/103fa647d159e3d76be2634d2653c2d
> 215dd0d46
> - **ArmVirtPkg:ArmPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/34969dd260b4ecaba8d4a58d32fc07b
> d0fee57f9
>    - Commit:
> https://github.com/tianocore/edk2/commit/7d78a86ecfc6d86244a552b1bad6fb3
> 46439116b
> - **ArmVirtPkg:OvmfPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/f8d0501ded2ad426cc5c6c1237ec83f
> c978f8373
>    - Commit:
> https://github.com/tianocore/edk2/commit/d881c6ddf556dfb5032db04733dfa86
> aa891e7f8
>    - Commit:
> https://github.com/tianocore/edk2/commit/77e9b3a7c68cea4eeb64af85e4f695f
> 278172314
> - **ArmVirtPkg:ArmPkg:OvmfPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/9a7509e465d64831482473320056619
> 0c8814ec6
> - **ArmVirtPkg:EmbeddedPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/e40fefafa90fb3a9aca77adc697c97c
> f6a4dd673
> - **BaseTools:ArmPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/f484427d10a5ff6c2437c2f7c671e9e
> 552ad6766
> - **DynamicTablesPkg:MdePkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/4c55f6394fafe0494ec24e7c05cb68c
> 938d7852d
> - **IntelFsp2WrapperPkg:IntelFsp2Pkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/fa78edc57eb000f7820d5896f610530
> 69b61eba1
> - **MdeModulePkg:MdePkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/5443c2dc310d2c8eb15fb8eefd50573
> 42e78cd0d
>    - Commit:
> https://github.com/tianocore/edk2/commit/53eb26b238541799134aa5846530485
> c915735da
>    - Commit:
> https://github.com/tianocore/edk2/commit/502c01c5028038e4e6b4512e9c66be0
> ec4d11492
> - **MdeModulePkg:OvmfPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/fd306d1dbc36d10f744306e92366d73
> 273c82c8f
> - **MdeModulePkg:UefiCpuPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/e7abb94d1fb8a0e7725b983bbf5ab13
> 34afe7ed1
>    - Commit:
> https://github.com/tianocore/edk2/commit/2a09527ebcb459b40bc3661d85aa11c
> 3905526dc
> - **MdeModulePkg:MdePkg:SecurityPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/65b5dd828ef2ea5056031b239a4e7a6
> 642f771a3
> - **MdeModulePkg:UefiCpuPkg:ArmPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/0f7bccf584d93b2642c0a413a47fc82
> 1d1f5dbfd
> -
> **MdeModulePkg:ArmVirtPkg:SignedCapsulePkg:EmulatorPkg:OvmfPkg:NetworkPk
> g:SecurityPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/2f981bddcbd6adde5f682caf0d3812b
> a92bc0f73
> - **MdePkg:NetworkPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/671b0cea510ad6de02ee9d6dbdf8f9b
> bb881f35d
> - **MdePkg:UefiCpuPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/8079d4dc4f8ac23e02b02996e326cac
> 4099b3e49
>    - Commit:
> https://github.com/tianocore/edk2/commit/ea55bd8f66eeca5f4e80c3679bcf1b1
> 007286b8a
> - **MdePkg:UefiPayloadPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/39f3c26e8c40e092baeb0ec4d039649
> 8506e0a9e
> - **OvmfPkg:BaseTools**
>    - Commit:
> https://github.com/tianocore/edk2/commit/f0b97e165e0af79ac9eb3c6ac7697f9
> 183afc7cb
>    - Commit:
> https://github.com/tianocore/edk2/commit/ff36b2550f94dc5fac838cf298ae5a2
> 3cfddf204
> - **OvmfPkg:UefiCpuPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/f220dcbba86bfc1222180c61bbd31dd
> 6023433db
>    - Commit:
> https://github.com/tianocore/edk2/commit/765ba5bf050022de8c44d93e467639d
> 7f5fa237c
> - **OvmfPkg:UefiCpuPkg::UefiPayloadPkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/a89f558d3c56d9c16e5f5b7d395c1aa
> 36ccd38f2
> - **RedfishPkg:MdePkg**
>    - Commit:
> https://github.com/tianocore/edk2/commit/7275993dc64481b1c21d7d70af434bf
> aafe86e81
> 
> Thanks,
> Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114918): https://edk2.groups.io/g/devel/message/114918
Mute This Topic: https://groups.io/mt/104081468/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Commits Spanning Packages

[Pedro Falcato]

On Wed, Jan 31, 2024 at 6:58 PM Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> Some commits have been spanning packages. For a monolithic upstream repo
> like edk2, this makes ingesting commits downstream more tedious if
> project requirements do not necessitate all packages to be maintained.
>
> Below are examples of commits spanning packages. Many of these are more
> problematic because "core" packages like MdePkg, MdeModule, SecurityPkg,
> UefiCpuPkg, etc. are involved. OvmfPkg and ArmVirtPkg seem to be a
> common pairing. To focus the examples below, I excluded commits with
> that pair.
>
> The request is to limit crossing package boundaries as much as possible
> and for maintainers to assess package impact when merging changes.

Personally I'm all for making downstreams' lives easier, but I don't
see how one could pull off many of the changes you linked, in 2+
commits, without breaking bisection.

Pulling off 103fa647d159e3d76be2634d2653c2d215dd0d46 would require you
to duplicate the struct, change a bunch of usages of the old struct to
the new struct, then change the old one back. This is hugely annoying
and complicated, and we should avoid this kind of rigamarole if
possible.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114920): https://edk2.groups.io/g/devel/message/114920
Mute This Topic: https://groups.io/mt/104081468/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Commits Spanning Packages

[Michael Kubacki]

On 1/31/2024 3:41 PM, Pedro Falcato wrote:
> On Wed, Jan 31, 2024 at 6:58 PM Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
>>
>> Some commits have been spanning packages. For a monolithic upstream repo
>> like edk2, this makes ingesting commits downstream more tedious if
>> project requirements do not necessitate all packages to be maintained.
>>
>> Below are examples of commits spanning packages. Many of these are more
>> problematic because "core" packages like MdePkg, MdeModule, SecurityPkg,
>> UefiCpuPkg, etc. are involved. OvmfPkg and ArmVirtPkg seem to be a
>> common pairing. To focus the examples below, I excluded commits with
>> that pair.
>>
>> The request is to limit crossing package boundaries as much as possible
>> and for maintainers to assess package impact when merging changes.
> 
> Personally I'm all for making downstreams' lives easier, but I don't
> see how one could pull off many of the changes you linked, in 2+
> commits, without breaking bisection.
> 
> Pulling off 103fa647d159e3d76be2634d2653c2d215dd0d46 would require you
> to duplicate the struct, change a bunch of usages of the old struct to
> the new struct, then change the old one back. This is hugely annoying
> and complicated, and we should avoid this kind of rigamarole if
> possible.
> 

That is why the request is:

"... to limit crossing package boundaries as much as possible
and for maintainers to assess package impact when merging changes."

Not an absolute rule. However, the rules currently seem ambiguous and 
susceptible to convenience.

To provide a counter example of a single change that could easily be 
split up and couples MdePkg (highly used, mandatory, fundamental) with 
RedfishPkg (much less frequently used, specialized) - 
https://github.com/tianocore/edk2/commit/7275993dc64481b1c21d7d70af434bf.

There are also dependency relationships between packages. The example 
you cited is between ArmPkg and ArmPlatformPkg. Which has better 
cohesion than something like the example I provided between MdePkg and 
RedfishPkg. Or, all the packages involved in 
https://github.com/tianocore/edk2/commit/2f981bddcbd6adde5f682caf0d3812b 
(which could also easily been split up).

Those should have been split up and could have with minimal effort.

Not looking for anything more than to request devs understand impact, 
package relationships/scope, and consider tooling to make this more 
consistent and raise awareness when it relatively rarely comes up.

- Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114921): https://edk2.groups.io/g/devel/message/114921
Mute This Topic: https://groups.io/mt/104081468/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-