Hi all,
I have been tied up a lot these days, the project is under a lot of
load, and as a result the pressure on myself and individual
contributors alike is mounting, which is not helping, so lets try to
alleviate some of this pressure.
My goal with this email is to improve the efficiency of this project,
in terms of safely landing new features.
Debunking the magic bullet fallacy
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A lot of people around here want to land a lot of new *features*, and I
want to be very clear that we are not talking about patches, we are
talking about features, most of which have a great impact on core
architecture and design.
Because of a number of circumstances, not least of which is an increase
an overall load, review cycles are becoming less efficient.
This has led some people to believe that:
a.) We dont have enough people with the power to review patches.
I agree with this, which is why I have delegated a lot of feature
branch reviews to Jürg, while only discussing things between us
at a high level in some (not all) circumstances.
I want to find another lieutenant who can review feature
additions, but for this to happen people need to understand the
codebase as a whole, care about how the codebase evolves as a
whole, and beyond this they have to earn my trust such that I am
assured we are united in a common vision.
Unfortunately this takes time, and I find myself lacking in
suitable candidates, but believe me that I am hungry to find one.
I think we can be doing something to make it easier for new members of the
project to find their way in the codebase. HACKING.rst does explain rules
around submissions, the coding style, etc, but it doesn't refer to an overall
architecture.
I think we can make it more feasible for people to understand the whole
codebase by providing an architecture overview with references to the
codebase.
One perspective is to also follow the flow of the `bst` commandline client,
and explaining what happens where. In light of the BuildStream as a
library thread, other perspectives will follow further down the line.
I'll try to contribute something in this space, but will solicit some
help :)
Lastly, I am not sure I agree with the need to understand the codebase
as a whole for everyone with the commit-bit set. As the project
progresses and becomes more complex (we hopefully are able to
guard against over-complexity), specialism is bound to happen. Either
out of interest or for other reasons. One example that springs to mind
is documentation. I could also see this happen for other areas (e.g.
Sandbox). Which is a segway into:
b.) A core developer should be able to land patches without review,
and fix later if a problem arises.
In my experience I have often worked this way, and we currently
do have a short list of committers in BuildStream. It works well
as long as said core developers are personally responsible and
accountable for the patches they are sure about.
For a contributor to retain committer status, I have to know that
the "fix it later" part has an immediate turnaround and takes
precedence over all other ongoing work, otherwise this is a
violation of trust, and it means that "being sure about the
commit" is in fact a lie.
I wouldn't quite put it so negative. Or rather it depends on your
definition of being sure. Mistakes happen, and should be addressed.
If another helpful soul addresses the issue before the original
committer turns around a fix, that is totally fine. Timezones, life,
are reasons why someone could not _immediately_ turn something
around. And we can mitigate negativity here by a) putting a
reasonable turnaround time on a fix, and b) being able to revert
the change and that being ok to do in absence of an immediate
fix.
Also, this cannot mean unreviewed landing of feature additions,
which necessarily impact API stable interfaces or make
significant changes to core architecture and design, which may
not need detailed code review, but needs discussion an approval.
I think that is fair. Agreeing upon a design, upon an API, and potentially
an approach beforehand will prevent surprises. Ideally this also identifies
others that are interested in the feature, which should help with the
code review process.
We are not suffering from an influx of easily reviewable bug
fixes which are not landing, which happens sometimes in larger
and more mature software projects: We are suffering from an
influx of many large and complicated feature branches that all
want to land now, so it should be noted that having more
committers in this scenario does not fix the current problem.
There's also small and less complicated features, which do take away
review time from the more complex ones.
And the burden of actually landing the changes, where it's not as simple
as hitting the Merge button on the GitLab UI...
One of my big concerns here is that Jurg and you, due to the burden that
comes with being a maintainer, are spending time that would otherwise
bring more value to the community in the form of other contributions.
c.) The bar is too high for landing patches.
This is just patently false. The last two weeks I've spent fixing
regressions, and untangling structural weakness in our codebase
is proof enough to me that the bar needs to be raised, not
lowered.
Just some thoughts... is the current model mentally enabling
non-maintainers to offload the responsibility of overall codebase quality
to the maintainers? Sense of ownership and pride and joy are to an
extent enabled by having control and responsibility. It also allows
the load to be spread more evenly (or at least less disproportionally).
In summary here, there is no magic bullet which will allow your
features to land faster, even if we had more core maintainers, large
core features could not safely land any faster, and relaxing the review
process here can only result in a complete disaster.
I have a less apocalyptic view :). I understand your perspective though.
What can we do to make the project more efficient ?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Asides from the fact that you just cannot land a lot of features at the
same time safely, there are a few things we can do to greatly improve
the efficiency in how we land features.
On my part, I'm going to try to do a better job in roadmapping, I had
this working much better last year but in recent times I've been too
tied up with specific goals to paint a better picture here.
There were also a lot less people involved :). Even if we do roadmap,
does that preclude others from working on things not on the roadmap?
If someone has a hypothetical itch to scratch, I assume we still welcome
those contributions?
In order for Jürg and I to handle review of your features more
efficiently, I'm going to lay out a list of things you can do to make
all of our lives easier.
Round tripping is more expensive than actual review
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The life of a maintainer is wrought with micro interruptions and
context switches, as such it is important to state all of the facts
at once and avoid round tripping.
Unless it is clearly in context in a mailing list thread or issue
report thread: Never assume we know what you are talking about, state
all of the facts and provide links.
When writing an email to the list, or a comment on an issue report,
provide _all_ of the details at once. Dont attempt to entice me into
a long discourse, instead strive to provide a clearly thought out
problem statement with the goal of having it solvable in a single
reply.
You may find that half of the time, going through the exercise of
clearly stating a problem in an email results in your having
understood the problem, and sending the email becomes unnecessary
(while the activity of writing it was valuable).
There are cases where still posting it with a TL;DR disclaiming it is a self answering message, is adding value for the community.
Only submit patches that you believe will land
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
We've been very relaxed in the last year about how we do reviews,
patch review has been a place for mentoring and discussion about
how to land the patch.
Remove this from our workflow by only submitting patches for review
that you are personally near certain will land.
This is impossible for a first time submitter, and we understand
this. Don't be shy to submit your first patch, and if you have team
mates who regularly submit patches to BuildStream, get them to review
it first before submitting it for review in our tracking system.
I guarantee that the patches you think are ready to land, will
most often not be ready. But I also guarantee that this will
*greatly* reduce the round tripping which is bogging us down and
clogging up the review pipeline.
An alternative/addition is to do non-maintainer round tripping. That is,
work with others and get someone to give it a thumbs up before landing
it in the maintainer queue.
Don't take this out of view into private repos, because other community
members (and potentially maintainers) may still be interested in how you
are making the feature sausage.
Submitting with a WIP prefix should help this process, right?
Try to leave the code in a better state than how you found it
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In many cases, there are multiple ways to implement a feature,
usually these consist of a quick and dirty way, and a way which makes
sense.
Choose the way which makes sense, even though it almost certainly
costs a bit more legwork in the short term.
The quick and dirty fix is almost always going to be percieved as a
threat to the longevity of the project at large, and it is our
position as maintainers to defend the project against such threats.
Dont make us feel threatened by your patches, instead try to share
in the responsibility of making the whole codebase better all the
time.
The above, intentional or not, reads really combative. And for me
at least introduces an "us vs them" storyline :/. Again, I'd like us to
use more positive messaging, which can achieve the same goal.
Try your best to suggest a solution, or more than one solution
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
When you have a feature to implement and you see the current
architecture does not "fit" with what you intend to achieve,
this means that we need to reconsider the architectural design
to afford for the feature you want to bring to our users.
In these cases, try to be creative and think of a solution which
best fits the architectural design of the current codebase, and
propose that solution.
This goes back to the ability to easily verify against the architectural
design of the current codebase.
In other words, try to make it easier for the core maintainers to do
their jobs: it's much easier for us to look at a proposed solution
and identify problems with that, and possibly point out problems with
the proposal for you to work on, than to do legwork and come up with
a plan ourselves to solve the problem you want to have solved.
Of course. That doesn't mean feature proposals without solutions are
invalid and should not be filed, correct? Maybe set the expectation that
if it's not on the roadmap, maintainers won't contribute to features that
have no solution proposals.
Solve the mentoring problem outside of the maintainership
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Last year I spent a great deal of time coaching younger developers in
the ways of writing maintainable software.
I actually love teaching, so I am particularly at risk of falling
prey to an invitation to mentor you on how to write your patch.
:)
In the present climate however, it is unreasonable for me to be
spending my time in such activities - when I happen to find time, I
will indulge myself.
If you have juniors on your team, then have your senior developers
mentor them as they develop features for BuildStream, help them write
proposals to the mailing list before hand to ensure that we are first
in agreement on landing the feature and that the architectural
changes involved are going to be accepted.
Mentor them to be good at submitting changes to FOSS projects.
Respect the style in place and be consistent
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It is of great importance that the codebase remain consistent such
that it can be read and understood as a whole to any newcomers.
Linters are not sufficient to address this.
Not all of our coding practices are the best, if you do not agree
with a practice, propose that we change it, and if we agree to change
it, we will expect a patch which changes the practice *unilaterally*
across the codebase.
It is not acceptable to leave the codebase in a state where things
are done in one way here, and in another way there.
This should really go without saying: Don't let your personal taste
in coding style get in the way of having a consistent code base, this
can only be a constant source of friction in the review process which
will waste time and leave everyone dissatisfied.
Summary
~~~~~~~
In closing, there is a lot we can do to improve the flow of reviews for
the many features that all want to land - increasing the volume and
pressuring for more reviews more often from the maintainership is not
going to magically make the feature branches safe to land sooner.
Let's be more professional and productive, share the responsibility
more evenly, and while we wont have any magic bullet, we will certainly
improve the situation.
And by all means, lets reserve some measure of time for horsing around,
poking fun at one another, and laughing a bit from time to time.
Keep it light hearted and fun, and we'll all have a great time while
accomplishing great things.
I'm glad you closed out with these positive notes :).
Cheers,
-Tristan
Cheers,
Sander
_______________________________________________
Buildstream-list mailing list
Buildstream-list gnome org
https://mail.gnome.org/mailman/listinfo/buildstream-list
