Re: [External] Re: New test for patches in openbmc/openbmc

[Ed Tanous <edtanous@google.com>]

On Fri, Oct 8, 2021 at 7:18 PM Lei Yu <yulei.sh@bytedance.com> wrote:
>
> On Sat, Oct 9, 2021 at 1:35 AM Ed Tanous <edtanous@google.com> wrote:
> >
> > On Fri, Oct 8, 2021 at 1:31 AM Lei YU <mine260309@gmail.com> wrote:
> > >
> > > It's noticed that the `repotest` is enabled in CI and we got CI
> > > failure due to node-manager's patch:
> > > https://gerrit.openbmc-project.xyz/c/openbmc/openbmc/+/47673
> > >
> > > I know the right way is to ask Intel to upstream the node-manager and
> > > fix the issues we met.
> > > But in reality it's not easy and it takes time for Intel to upstream a
> > > repo (and it depends on Intel to decide whether or not to upstream)
> >
> > If this is something you need, there's no need to wait for Intel, as
> > that application already has an Apache 2 license.  You are free to
> > upstream it and maintain it yourself if you don't want to wait for
> > intel.
> >
> > >
> > >
> > > @Ed Do we really want to reject such patches?
> >
> > I don't want to reject patches, I want to see them on master in a way
> > that things can be changed as needs evolve.  This patch is a perfect
> > example of something that, had we taken the small amount of time to
> > upstream this small daemon, wouldn't have even been an issue now as
> > sdbusplus needs to make a very minor change.  As-is, we're effectively
>
> Totally agree. We have already asked Intel to upstream the
> node-manager, let's wait for the feedback.'

Let's not wait any longer.  The code is licensed appropriately, is
already open source, and in total, is smaller than a number of single
patchsets I've seen in recent history (it would probably classify as a
medium patchset).  Just open a review to add the code to an existing
repository, or request a new repository along with a design doc.  If
you want this to live in dbus-sensors, I'm fine with that, just make
sure we have maintainers that can test on their systems;  It seems
like an ok fit given it's another entity-manager enabled sensor app,
although I don't have a strong opinion between the two options.


>
> > 2 levels of fork deep (openbmc upstream -> intel-bmc -> openbmc
> > upstream only for bytedance systems, which is the source of the
> > problem, not this patch itself.
>
> True. But as an Intel x86 platform, the repo is needed and in the
> current state, the patch has to be added. Otherwise the g220a build is
> broken.
> Is it OK to ignore the repotest CI failure and just merge the patch in
> meta-bytedance layer?
> (Be noted that it's not trying to make a bad example, but only trying
> to fix the broken build)