New test for patches in openbmc/openbmc

[Benjamin Fair]

This policy isn't new, just this automatic enforcement. It was reviewed in
February: https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/40294

There's also a detailed explanation of the rationale for each guideline in
that document.

I'd like to point out that some of the patches listed in the original email
are against non-OpenBMC projects, which is allowed in some situations by my
understanding of the guidelines.

On Wed, 22 Sept 2021 at 19:50, John Broadbent <jebr at google.com> wrote:

> I am concerned this change will encourage both patches in private layers,
> and forks of the entire project.
>
> Oskar is right, patches should be temporary fixes, but I have worked
> around, and some organizations never clean up their "temporary fixes".
> Their engineers move from one fire to the next. I suppose, I would
> prefer to see .patch files in openbmc meta layers rather than have the same
> .patch file pushed to a private layer, or worse a fork of openbmc.
>
> Where can I get some more context on why .patch files are disallowed from
> open bmc meta layers?
>
> I genuinely appreciate all their effort and hard work
> the maintainer put in. They have always guided the community in the
> right direction, but some more context for this decision might be helpful
> for new people, such as myself.
>
> Thank you
> John Broadbent
>
> On Wed, Sep 22, 2021 at 4:36 PM Oskar Senft <osk at google.com> wrote:
>
>> Hi Alexander
>>
>> While I can understand your position, I think there's a bigger picture
>> to consider. In my understanding Open Source works by individual /
>> independent contributors providing their use cases, knowledge and
>> experience by means of designs and source code to the world. Since
>> there are many individuals trying to do different things and some
>> people (maintainers) being the gatekeepers for what can be submitted,
>> it of course often gets to a point where not everyone agrees.
>>
>> Trust me, I've been there. I had many occasions where I needed a new
>> feature or a fix to satisfy project requirements and timelines and was
>> not able to upstream it in the given time. I sometimes gave up, often
>> found a different, "better" solution and many times worked with the
>> community to find a solution that would be accepted upstream.
>>
>> While I agree that deadlines and requirements do not always allow to
>> go the "everything upstream immediately" route, my experience has
>> shown me that forks or patches are ultimately costing more than using
>> clean upstream code, in particular if a device is to be supported for
>> years through new versions of the upstream code.
>>
>> As an example, we've been using an i2c sensor chip that needs to be
>> configured at runtime. Upstream support for that was (still is)
>> missing. The patch to do that specifically for us was 1 line -
>> literally. However, it's incredibly difficult to discover and
>> understand this one line years later. Together with hwmon maintainers
>> I've spent the last 2 weeks designing and implementing various
>> versions of a generic solution that we hope can be used for other
>> hwmon drivers. I understand that I'm in a fortunate position so I can
>> spend that time. But I still need to justify to my manager and myself
>> why it's worth it, which I believe I can.
>>
>> In my experience, having patches checked in is just that - a temporary
>> patch - not a solution. From Oxford's dictionary: "to patch: treat
>> someone's injuries or repair the damage to something, especially
>> hastily" (I know there's also a definition of the noun in the realm of
>> computing).
>>
>> So while I agree that not allowing patches is actually making things
>> harder for some in the short term, I truly believe that it's going to
>> make things better for everyone in the long term.
>>
>> Oskar.
>>
>> On Wed, Sep 22, 2021 at 5:03 AM Alexander Amelkin <a.amelkin at yadro.com>
>> wrote:
>> >
>> > Hi Ed!
>> >
>> > Most patches you listed (at least those for YADRO) are
>> > platform specific and no repository will accept them for
>> > a general audience.
>> >
>> > No vendor, I'm confident, is willing to spend endless time
>> > persuading maintainers to include vendor-specific or
>> > platform-specific patches into their repositories.
>> >
>> > For instance,
>> >
>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0002-Add-support-for-boot-initiator-mailbox.patch
>> > is there because our customers demand this feature and we failed
>> > proving to openbmc maintainers that this is a needed feature
>> > and not a "security threat" or something. We honestly tried for months.
>> >
>> > On the other hand,
>> >
>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0004-aspeed-add-bmc-position-support.patch
>> > is strictly hardware-specific and is not needed as is for other
>> > vendors or platforms, and we don't have time to make it a
>> > generic solution. If we ever do have that time, we will surely
>> > push the developed generic solution to the appropriate
>> > repository.
>> >
>> > What you propose now will force vendors to move farther away
>> > from upstream and create their own forks of openbmc where
>> > they will not even try to upstream their changes and will just drift
>> > farther and farther away.
>> >
>> > Is that what you really pursue or did I get your idea wrong?
>> > So far it looks to me like a destructive decision.
>> >
>> > WBR, Alexander.
>> >
>> > 22.09.2021 01:35, Ed Tanous пишет:
>> > > A few new features have been merged into CI that will now disallow
>> > > .patch files within most meta layers.  This is due to a significant
>> > > number of them popping up in both reviews and in the repo itself,
>> > > despite having documented rules to the contrary.  The hope here is to
>> > > better codify our rules, and give very quick response to submitters
>> > > about the right procedure so we can encourage getting patches in
>> > > faster, and keep machines buildable against master.  As the patches
>> > > state, meta-phosphor is still allowed to contain patch files as an
>> > > escape hatch, if the community decides it's required.
>> > >
>> > > The patchsets in question are here:
>> > > https://gerrit.openbmc-project.xyz/q/repotest
>> > >
>> > > And add some ability for us to make more of these expectations for
>> > > meta layers codified in the future.
>> > >
>> > > The script itself is here:
>> > >
>> https://github.com/openbmc/openbmc/blob/master/meta-phosphor/scripts/run-repotest.sh
>> > > and is runnable on any tree prior to submitting to CI.  We currently
>> > > have the following patches in meta layers.
>> > >
>> > >
>> meta-amd/meta-ethanolx/recipes-x86/chassis/x86-power-control/0001-Amd-power-control-modifications-for-EthanolX.patch
>> > >
>> meta-ampere/meta-common/recipes-devtools/mtd/mtd-utils/0001-flashcp-support-offset-option.patch
>> > >
>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0001-aspeed-scu-Switch-PWM-pin-to-GPIO-input-mode.patch
>> > >
>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0002-aspeed-Disable-internal-PD-resistors-for-GPIOs.patch
>> > >
>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0003-aspeed-support-passing-system-reset-status-to-kernel.patch
>> > >
>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0004-aspeed-add-gpio-support.patch
>> > >
>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0005-aspeed-Enable-SPI-master-mode.patch
>> > >
>> meta-ampere/meta-jade/recipes-bsp/u-boot/u-boot-aspeed/0006-aspeed-support-Mt.Jade-platform-init.patch
>> > > meta-aspeed/recipes-bsp/u-boot/files/default-gcc.patch
>> > >
>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0001-bytedance-g220a-Enable-ipmb.patch
>> > >
>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0003-misc-aspeed-Add-Aspeed-UART-routing-control-driver.patch
>> > >
>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0004-ARM-dts-aspeed-Add-uart-routing-node.patch
>> > >
>> meta-bytedance/meta-g220a/recipes-kernel/linux/linux-aspeed/0005-ARM-dts-aspeed-Enable-g220a-uart-route.patch
>> > >
>> meta-bytedance/meta-g220a/recipes-phosphor/ipmi/phosphor-node-manager-proxy/0001-Remove-Total_Power-sensor.patch
>> > >
>> meta-facebook/meta-bletchley/recipes-bsp/u-boot/u-boot-aspeed-sdk/0001-u-boot-ast2600-57600-baudrate-for-bletchley.patch
>> > >
>> meta-facebook/meta-tiogapass/recipes-bsp/u-boot/u-boot-aspeed/0001-configs-ast-common-use-57600-baud-rate-to-match-Tiog.patch
>> > >
>> meta-facebook/meta-yosemitev2/recipes-bsp/u-boot/u-boot-aspeed/0001-board-aspeed-Add-Mux-for-yosemitev2.patch
>> > >
>> meta-facebook/meta-yosemitev2/recipes-bsp/u-boot/u-boot-aspeed/0002-spl-host-console-handle.patch
>> > >
>> meta-google/dynamic-layers/nuvoton-layer/recipes-bsp/images/npcm7xx-igps/0001-Set-FIU0_DRD_CFG-and-FIU_Clk_divider-for-gbmc-hoth.patch
>> > >
>> meta-google/recipes-extended/libconfig/files/0001-conf2struct-Use-the-right-perl.patch
>> > >
>> meta-google/recipes-extended/libconfig/files/0001-makefile-Add-missing-LDFLAGS.patch
>> > >
>> meta-google/recipes-phosphor/initrdscripts/obmc-phosphor-initfs/rwfs-clean-dev.patch
>> > >
>> meta-ingrasys/meta-zaius/recipes-bsp/u-boot/u-boot-aspeed/0001-board-aspeed-Add-reset_phy-for-Zaius.patch
>> > >
>> meta-nuvoton/recipes-bsp/images/npcm7xx-igps/0001-Adjust-paths-for-use-with-Bitbake.patch
>> > >
>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0001-Add-system-reset-status-support.patch
>> > >
>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0002-config-ast-common-set-fieldmode-to-true.patch
>> > >
>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0003-aspeed-add-gpio-support.patch
>> > >
>> meta-yadro/meta-nicole/recipes-bsp/u-boot/files/0004-aspeed-add-bmc-position-support.patch
>> > >
>> meta-yadro/meta-nicole/recipes-kernel/linux/linux-aspeed/0001-Add-NCSI-channel-selector.patch
>> > >
>> meta-yadro/meta-nicole/recipes-phosphor/host/op-proc-control/0001-Stop-and-send-SRESET-for-one-thread-only.patch
>> > >
>> meta-yadro/recipes-phosphor/dbus/phosphor-dbus-interfaces/0001-Add-boot-initiator-mailbox-interface.patch
>> > >
>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0001-Add-support-for-persistent-only-settings.patch
>> > >
>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0002-Add-support-for-boot-initiator-mailbox.patch
>> > >
>> meta-yadro/recipes-phosphor/ipmi/phosphor-ipmi-host/0003-Fix-version-parsing-update-AUX-revision-info.patch
>> > >
>> > > If you are a maintainer of these meta layers, please work to get these
>> > > patches submitted to the correct repositories using their prefered
>> > > review (email for linux/u-boot, gerrit for phosphor repos).
>> > >
>> > > Thanks,
>> > >
>> > > -Ed
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20210923/e3aa25ce/attachment-0001.htm>