Re: [BuildStream] Some bad mistakes, lets understand them, and make this work better

[Tristan Van Berkom <tristan vanberkom codethink co uk>]

Hi Sander,

On Wed, 2018-09-12 at 15:40 +0100, Sander Striker wrote:
> Hi,
[...]
> > I agree that it took quite some time, but in this particular case, I
> > really feel that Tristan Maat was out of his depth and this required a
> > lot of design phase before implementation. So I helped a lot with
> > designing the changes which needed to happen to the scheduler.
>
> Did we actually bring this to the list?  Because changes to the
> scheduler are changes to the core, and that is something that
> everyone should be aware of.  It might have drawn some additional
> eyeballs to the design and intent.

We didn't bring it to the list, but we certainly should have done that.

Frankly, at the time I felt there was a lot of pressure for velocity,
and that an email thread in that timing would postpone this further (I
might be wrong here, though). I would certainly much prefer to do this
sort of thing on the mailing list.


[...]
> > I very strongly agree with you and would prefer a tighter circle of
> > committers myself.
> >
> > I'm not sure how we can really describe that in terms of policy,
> > though, and it was raised to me that we probably need to be acting on
> > policy.
> >
> > I can see that side of the coin, even though I personally have greater
> > confidence in a completely ad hoc approach where people use critical
> > thinking to decide to hand out commit access on a case by case basis
> > (it wouldn't be perceived as "fair", but I'm not sure it needs to be).
>
> The process can be subjective.  I'll refer back to 
> http://subversion.apache.org/docs/community-guide/roles.html#committers as an example of something that 
> works.

This looks very good to me.

If there are no strong objections, I would very much like to implement
this model for BuildStream.

I do feel that we might be adding too much process for a project which
is not really quite that huge in scale, that said; we already have more
people working on the project than I ever anticipated.

Perhaps I should raise this as a separate proposal to the list.

[...]
> > But more accurately I feel that we are not making changes in the right
> > ways in the first place, and would rather emphasize on that, rather
> > than the timing.
>
> Making changes in the right ways comes way ahead of review, so we
> should indeed put emphasis there.

You are entirely right that we should focus on making coding practices
and overall architecture more clear.

[...]
> > As I've also mentioned in the same email; I know that Tiago
> > specifically has demonstrated that he cares enough to submit patches
> > which refactor some of the things I've called out in this email.
> >
> > That said, I find myself rather alone on this front which is sad, and
> > more seriously: I am not enough people to refactor the code base at the
> > rate which others are landing features.
>
> Nor should you.  But clearly the understanding of the codebase and
> the intents are not aligned.  While that is the case you are setting
> yourself up for an impossible mission and a world of frustration. 
> For yourself, but also for others.

Yes, I am starting to see things differently.

You have raised the need of having clear architectural diagrams and
more guidelines on several occasions, I have remained skeptical as to
whether the maintenance cost of keeping all of this updated is worth
the benefits.

I think this boils down to scale again, it's generally simple enough to
ensure everyone is on the same page when everyone can be counted on
your fingers, even better with one hand.

As we have this many developers at this point, we need more materials
such as guidelines in place to ensure people can get the right answers
when humans are not available to guide them - otherwise, all humans
involved are in for a world of frustration, as you accurately point
out.

On the flip side, I strongly believe that the health of the project
will still be contingent on having "go to" humans which scale at the
rate of added developers, however, we have already investigated the
possibility of cloning Jürg and the outcome was grim. As we don't
really have control of that aspect, we must do what we can :)

[...]
> > Also, "consideration" is a better word here than "caring".
> > Consideration of the side effects your patch has to others who will
> > have to contribute to the same code base, takes a conscious effort, and
> > we need more of that effort.
> >
> > I don't believe that "people don't care", but I believe that where
> > there is lack of consideration, it is mostly a lack of experience.
>
> I think that Jim's response shows this to be not quite the case.
> Clearer guidance and expectations make up for a lot of lack of
> experience.  And lack of experience also means not having to un-learn 
> certain things :).

Half agree.

Jim's response shows that different sets of experiences lead to
different measures of consideration, I certainly agree with this.

> > We just need to communicate better, why we make the technical decisions
> > we make in review (which I'm trying to illustrate with my examples of
> > what not to do), so that others who end up being reviewers, make the
> > same considerations we do.
>
> Right, and that's the gist of it.  We need to make this front and
> center.  And more approachable than reading this thread, or rather,
> your original post :).

Nod.

[...]
> Do we want to do something at the MR level that provides guidance like:
> http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
>
> Or do we want to do that on a per commit basis?  Or not at all?

I think we want it on a per commit basis.

And I think that the subversion example in this case is overkill, but
agree that we could better than:

  http://buildstream.gitlab.io/buildstream/HACKING.html#commit-messages

In the text above the linked section, we do mention:

 "If the branch fixes an issue or is related to any issues, these
  issues must be mentioned in the merge request or preferably the
  commit messages themselves."

But we should just be less ambiguous and more strict about that, just
always mention the issue in the commit message is better than leaving
wiggle room.

[...]
> > Of course, it's significant churn and costs some effort, feature
> > branches would conflict a lot; but the refactor would be rather
> > mechanical and straight forward to carry out (albeit still a lot of
> > work).
> >
> > Perhaps this change is worth it for the added linting securities it can
> > provide.
>
> It feels like an extreme measure, and potentially unnecessary if
> people know what the expectations/rules are?  

I'm actually unsure if this is </sarcasm> :)

I do personally think it is an extreme measure, but on the other hand
accesses to private data are normally expected to raise a compile time
error, or are simply impossible to express in other languages.

It could be very valuable to have this, although it would be better
without such a big change to the code base.

Ideally we could inform the linter to ignore the error on the
declaration side of the private member, rather than having to inform
the linker on the invocation side, then we might be able to get away
with something much simpler, and still get the desired linting.

In fact, it seems more logical to implement this ourselves in the
linter itself, rather than to shift our codebase to adapt to how the
linter works, even.

Cheers,
    -Tristan