Re: [Xen-devel] [PATCH v7 1/2] OSSTEST: introduce a raisin build test

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2015-06-22 at 16:16 +0100, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

In general I'm very suspicious of a 200 line patch with _no_ commit
message at all.

What about the details of the stuff around overriding the versions iff
tree and revision are set etc and why that matters and how the two
projects therefore interact wrt selecting what to build? I remember
discussing this at length either IRL or in older versions of the thread
and we both know it wasn't trivial to arrive at how this should be done,
but I _already_ can't remember how we reached this point or why and that
is information which really needs to be recorded so it can be referred
to later when we wonder why it does this.

The scope is also missing, i.e. this is currently just a straight raisin
build test, and the results are not consumed by anything. This is
important because of all the stuff about splitting up the dist dir and
incremental builds which we discussed means that as it stands this test
step is not producing build artefacts suitable as an input for actual
tests.

I expect there's also stuff in the intra-version changelog which
actually belongs in the main changelog, such as why you are refactoring
certain things into BuildSupport.pm, which deserves some sort of brief
mention IMHO.

[...]
> Changes in v3:
> - use $raisindir throughout ts-raisin-build
> - do not specify ENABLED_COMPONENTS so that empty REVISION variables can
> be used to disable building a raisin component

I see we do again in the code below, which I suspect was deliberate and
based on some discussion, but it's not mentioned in the Changes in v4+
and since the changelog doesn't explain anything I can't even begin to
guess why or how we arrived at this point.

[...]
> +sub build () {
> +    target_cmd_root($ho, <<END);
> +        cd $raisindir
> +        ./raise -y install-builddep

If two of these happen to run in parallel (build machines can be shared)
then one or the other risks timing out on the underlying dpkg lock
waiting for the other, since the wait might be arbitrarily long
depending on what is going on.

It also risks other builds happening in an environment which differs
from one build to the next (if this happened to run in the middle) or
even things changing while a build is happening.

This is why all build dependencies are installed from ts-xen-build-prep,
that step is run once for each build host as it is installed. This does
unfortunately mean that I think we can't take advantage of raisin's
tracking of necessary build dependencies, but at least it will be
checking things for us.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel