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 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 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 >> 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 >> >