grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: RFC: Grub project management


From: Glenn Washburn
Subject: Re: RFC: Grub project management
Date: Sat, 13 Feb 2021 02:17:51 -0600

On Fri, 12 Feb 2021 17:16:39 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Thu, Feb 11, 2021 at 09:53:37PM -0600, Glenn Washburn wrote:
> > I believe I speak for more than just myself when I say that the
> > current development process leaves much to be desired. GRUB has
> > been in a feature freeze since last March, with the release being
> > pushed rescheduled several times and now looks like its projected
> > to be in March (assuming its not pushed back again). I get the
> > impression that Daniel is overloaded with non-GRUB
> > responsibilities. I also wonder if part of the issue is concerned
> > about making a release which he believes hasn't had sufficient
> > testing.
> > 
> > Daniel, could you please clarify what needs to be done to get the
> > release out the door and what the community can do to help expedite
> > this process?
> 
> What I am understanding is that Daniel is the bottlenck here. That is
> his reviews of patches (and fixes up) are what is holding up the
> acceptance?

Its not clear to me exactly what the nature of the bottleneck is. I
would like some clarity on this. I'm frustrated at what appears to me
to be a lack of transparency. So I'm left guessing at what the needs
are.

> > 
> > In the meantime, patches are being sent to the list which will not
> > be considered for acceptance until the release is out and feature
> > freeze lifted. Does anyone believe that someone is going to go back
> > to last March and check all the submitted patches to see about
> > including them?
> 
> Daniel (from the years I have worked with him) is very detailed
> person who has a nice workflow to make sure they are there.

That's great to hear.

> > I judge the answer to be no. So patches will be continually falling
> > into a blackhole. What we need is an issue/merge management system
> > so
> 
> Do you have an example of a particular patchset that went into a
> blackhole?

I can't really say what went into a blackhole because I don't know
what's on Daniel's list of patches to circle back to once the feature
freeze is lifted. I can give a list of patches that I see as being more
at risk (not that I personally care about all of them, but they seem
in concept worthy of inclusion).

* [PATCH 0/4] UEFI IPv6 and DHCPv6 support
  Apparently this was posted back in 2016 and never merged either,
  since the author works for SuSE. He'll probably come back in another
  4 years if its forgotten about

* [PATCH] minix: avoid mistakenly probing ext2 filesystems
  There was no review of this patch. The author appears to be from a
  company, so perhaps he'll keep pushing.

* [PATCH v2 0/7] support >512b sector disks with old/buggy firmware
  Also not reviewed, not super active author.

* search: Support searching for partition UUID with --part-uuid
  Never replied to. Author does not appear to be backed by a company.

* [PATCH] Cryptomount support for key files and detached header
  I reviewed this patch and recommended against it being accepted as is,
  but having something like GitLab's merge requests where a merge
  request was submitted and closed, still keeps track of this for other
  users that would want to use this patch.

* [PATCH] search: Support searching for GPT partition label with
  --part-label
  Not reviewed

* [PATCH v1] fix kernel cmdline corruption
  Will this get dropped like in 2018?

* v7 for detached headers and key files
  I don't think these should be included as is, but these patches have
  been attempted to be merged in one form or another since 2015. Even
  if they weren't accepted, having a closed out merge request allows
  grub users like myself to search for detached header and find the
  closed MR and potentially use it. The barrier to entry for someone
  to search the mailing list, get the patches, and finding an upstream
  commit that they'll apply against is a lot higher.

* [PATCH 0/5] Testing improvements
  These are my patches from almost 2 months ago. They haven't been
  reviewed.

Ok, now there's a list, so these might now avoid the blackhole.

There's lots of things that I'm finding on the list of potentially
useful, unaccepted patches.

> > things don't get lost, or can easily be found.
> 
> I would assume that folks who care about their patches would repost
> once the release goes out?

Depends on what level of care you're talking about. They obviously care
about their patch or they wouldn't have taken the time to create it and
submit it to the list. What I think what you mean is "they don't care
enough to baby-sit the patch for as long as it takes for the GRUB
project to get to the point where non-critical patches are accepted
again." And how do you know that by the time these patches are gotten
back to that the authors will be in a place where they will be willing
or able to pick the patches back up? I'm concerned that I will be one
of those people. So, no I don't think that is a fair assumption.

I'll speak for myself, and say that I don't want to have to continually
check the list to see if my patches have been responded to. I think the
cryptomount detached header patches are a good example of this. I want
to move on and have closure for my patches, ideally getting them into
an acceptable form and getting them merged. It actually add unnecessary
stress into my life.

And yet, I want to contribute to the project. I don't think its good
for the GRUB project to make things harder for those who aren't
employed by Ubuntu, Oracle, SuSE, IBM, or who ever else is actually
getting paid to work on this project. That's not the spirit of Free
Software as I conceive it.

I have a few unsubmitted patches that I've been waiting for the feature
freeze to lift to send. Even if I sent them now and they got reviewed,
there's a decent chance that they'd need to be revise around the time
that they get merged, which I might need to be around for. I think I'll
start sending them and see if they get picked back up. This is a big
reason for GRUB to have a devel branch, so that patches can be merged
even in a feature freeze.

I'm a bit frustrated, because I don't know where I'll be in 3 months in
terms of being able to pick back up patches. Context switching is
costly time-wise. And I've got other non-developer things I want to be
doing.

> Is this why the GitLab came about - as a way to keep track of patches
> that need review?

No, I came up with the idea of using GitLab for its CI. It has a pretty
decent free-tier. Travis seemed a lot more limited. GitLab was a
more featureful solution. I added the automatic CI for merge requests
because it was low hanging fruit, compared to the CI system in general.
I'm not saying that we need to use GitLab for keeping track of patches,
but I do think we need SOMETHING.

> > 
> > GRUB currently uses the Savannah GNU project hosting site. And
> > while I love GNU philosophy and the role that Savannah has played
> > for free software projects, it is showing its age and not ideal for
> > a modern development process. My two big pain points is that it
> > does not do merge requests and the bug tracker is an unpleasant
> > experience when compared to modern ones. If you've ever tried to
> > search for a bug, you'll know what I mean. So unfortunately, I
> > think its time to say
> 
> It is a feature. We got no bugs :-)
> 
> > goodbye Savannah.
> 
> But the merge requests are not the pain points. It is reviewing the
> patches I would think?

Pain for whom, users or developers? A user who wants to take an
unaccepted patch and use it because it fills a need might answer that
question differently than a developer trying to get the patches
submitted. They are both pain points in my opinion. So yes reviewing
does seem to be a big pain point, which is in part why I created the CI
system.

And we need to extend the testing system too, which is why I created the
patch series titled "[CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for
cryptomount". I've reworked them to not depend on the patches series
adding LUKS keyfile support, and will be submitting those soon.

I also think we should be trying to add tests along-side patch
submissions. Tests for bug fixes for regression testings as well as
tests for new features. For instance, the usb xHCI patches should add
some qemu tests (probably not that hard, just modifying the uhci_test).

> > 
> > Another issue, as I see it, is the testing. The only testing that
> > I'm aware of is the travis CI, which is only a build test, and
> > Adrian's testing for debian, which does do the make check tests but
> > is missing filesystem testing and qemu tests for most
> > architectures. Ideally, the make check tests should be done within
> > travis, but we're not there yet. This process feels closed and
> > non-transparent to me, which I find undesirable in general, but
> > especially so for a free software project which is the corner-stone
> > of the linux community. And I'm not saying that that is intentional
> > on the part of those working on those tests. So, for one, it would
> > be nice to have publicly accessible the status of Travis runs. And
> > may be it is, but why isn't it obvious?
> 
> As you point out the Travis CI is for build tests which is not that
> exciting. I think what you are really saying is that you want better
> tests?

The Travis CI runs one "test", which isn't much of a test. It just sees
if mkimage can build an image. We have a boatload of tests that can be
run by "make check". My CI setup does that, and it can be improved. And
yes, I do want better tests, as mentioned above.

> > 
> > Based on what I've gathered from the list and private conversations
> > (which isn't much), the travis is only really run as a prep for
> > making a release. If that's true, the process fails to use the main
> > point of a continuous integration system. I would like GRUB to get
> > to a place where every commit is run through CI (at least the build
> > test). In the short term, it would be nice to kick off a CI job
> > every time master changes.
> 
> So I agree it is nice, but I am missing how this is going to help
> in getting a release out faster?

It might not help in getting this specific release out faster. Because
of the lack of transparency in what actually needs to be done, I don't
know. I do think that it can in general speed up the patch submission
to merge into master and release timelines in general.

> > 
> > It would be even more nice if we could integrate the merge tracking
> > system with the CI system so that merge requests automatically kick
> > off a CI job to see if the proposed changes pass both the build
> > test and functional make check tests.
> 
> Ah, so it fixes the simple compile issues patches may have!

Nope, it doesn't fix anything, though I'm waiting for Google come out
with an AI that can. It'll make our lives much easier.

Joking aside, yes it will detect simple compile issues, just as the
current travis CI will do (though as far as I know its not used for
that), but its way better. What my GitLab CI config does is run the
"make check" functional tests, many of which use qemu for various
architectures to actually test that a built GRUB image can be booted
from. For instance, the sparc64-ieee1275 target builds fine, but is
failing several functional tests, like gzcompress_test. This may be an
actual bug, or a bug in the test. I don't know until we figure out why
its failing.

And of course, we get to find out more immediately when regressions
happen if we're run the CI on every patch series.

> > 
> > I believe, and my sincere hope, is that by resolving these issues we
> > can speed up the development cycle. I have a lot of changes that
> > I've been waiting months to submit to the list. I don't think it
> > should take a mountain of patience to get changes accepted. The
> > GRUB project and its users are worse off for it.
> > 
> > It is in consideration of all the above that I've setup a system on
> > GitLab that addresses all these concerns and more. I also believe
> > that GitLab was founded as a more ethical company that would be
> > more compatible with a GNU software project than most other project
> > hosting sites. I chose to use GitLab's builtin CI because it has
> > what appears to be unlimited free resources (although resource
> > constrained to an extent) for open source accounts.
> > 
> > I've setup an unofficial GNU GRUB group and project repository at
> > https://gitlab.com/gnu-grub/grub. It is currently configured to
> > mirror the official repository. I have created a few merge requests
> > that are real examples to show what the process would look like.
> > 
> > The most important merge request is the one adding the GitLab CI
> > feature (https://gitlab.com/gnu-grub/grub/-/merge_requests/2). This
> > is a merge request created from a branch in the repo, as opposed to
> > one from a fork. Because it has the .gitlab-ci.yml file configured
> > to run the CI on merge requests, a "Detached merge request
> > pipeline" is kicked off. The results of that can be seen on the
> > merge request page with a link to the pipeline and jobs so that
> > failing jobs can be quickly found and debugged. The last job in the
> > pipeline outputs a summary of architectures for which "make check"
> > tests were run and different kinds of test failures, for a quick
> > overview. Importantly, the GitLab CI config runs "make check" tests
> > successfully for 8 targets: arm-efi, arm64-efi, i386-efi, i386-pc,
> > i386-qemu, powerpc-ieee1275, sparc64-ieee1275, and x86_64-efi.
> 
> That is a nice list of targets!

And I'd like to get the mips and risc ones added. I could use some help
in getting past some blocks. Also I should add that by "successfully",
I mean passing tests excluding certain known failing tests. Those are
fairly clearly defined in the CI config file.

> > 
> > Another merge request is titled "Add basic support for xHCI USB
> > controllers and non root xHCI hubs." As noted in the description,
> > this is a patch series I took off the list authored by Patrick
> > Rudolph. The merge request pipeline fails for the x86_64-efi build,
> > but all the others succeed. I suspect that the gcc being used
> > (10.1.0) is newer than the one developed on and this gcc may be
> > grumpier. These errors can be fixed, the pushed to the same branch
> > and another CI job will be started to test the updated merge
> > request automatically.
> > 
> > Until the GitLab CI feature is merge into master, merge requests
> > should be rebased onto the feature/gitlab-testing branch and merged
> > requested against that branch (not master). This is needed so that
> > the merge request does not show the GitLab CI commits as part of
> > the merge request. Once the GitLab CI feature has been merged into
> > master, merge requests can be off master and against master, as one
> > normally would. One can see that this has been done for all the
> > current merge requests.
> > 
> > For patches that people don't want to disappear on the list, I
> > think a merge request can help mitigate that. Also since the merge
> > request is effectively a whole commit tree, instead of some free
> > floating diffs, we can know precisely which commit its based off of
> > (master? or last stable?).
> > 
> > This is not intended to change the current patch submission
> > requirements for the project. Patches will still need to be sent to
> > the list and will be reviewed on the list. I think for non-trivial
> > patches it should be required to make a merge request as well so
> > that the CI passing is a requirement. I hope that by enforcing a
> > passing CI that maintainer and reviewer time is saved by ensuring
> > that patch series pass the sniff test.
> > 
> > This is currently not officially part of the GRUB project, so
> > creating issues, for instance, should still be done on Savannah.
> > I'll be sending out a patch series to the list soon with the
> > changes used to integrate GRUB into GitLab's CI.
> 
> So the intent here is like the 0day bot on LKML? When someone posts a
> patch it compiles on different platforms and tells folks what and
> where things didn't compile?

I'm not super familiar with the LKML or the 0day bot, but as mentioned
earlier, its more than a build test, which makes it much more useful as
a test.

The main intent is making the CI system better and more public. Perhaps
the Travis is public, but I've seen nothing about how to look at run
logs for instance. I think the merge request integrated with the CI
makes the CI even more useful, but its useful without it. Perhaps, this
makes a better case for a devel branch which is pushed to more often. 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]