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

[Ian Campbell <ian.campbell@citrix.com>]

Ian -- various questions for you here.

On Mon, 2015-07-20 at 13:56 +0100, Stefano Stabellini wrote:
> On Thu, 9 Jul 2015, Ian Campbell wrote:
> > On Mon, 2015-06-22 at 16:16 +0100, Stefano Stabellini wrote:
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > [...]
> 
> I didn't do a good job with the commit message. I'll try to track down
> the old discussion and write some more details.

Thanks!

> > > 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.
> 
> It is fine for OSSTest not use install-builddep. I think that the best
> way for OSSTest to use Raisin would be to call raise in
> ts-xen-build-prep to list the dependencies and install them from there.

Unfortunately ts-xen-build-prep will not have access to the repo, since
it won't be cloned. Also when reusing a host (which happens for builds)
I think this step is skipped (Ian J: can you confirm or deny that?)

> But here we are testing Raisin itself so I think that in this context it
> is actually important to run install-builddep. I don't think it would
> cause any troubles to other builds happening in parallel because the
> build dependencies of those builds would be installed by
> ts-xen-build-prep beforehand, that I was told is run before any build
> jobs?  If I am wrong, then we have a problem.

I think there are two problems, one is that install-builddep will take
the apt/dpkg lock, which will be shared by any other invocations --
making the timeout hard to decide upon (since it will pipeline things).

Second if install-builddep did install something extra due to some
change in raisin.git, for example, then some jobs would see new build
deps arrive half way through, or subsequent tests of older branches
might see something installed which wouldn't have been before.

Is there a check-builddep? That's the thing I would suggest we run here,
so we get an explicit failure.

> Otherwise is there a way to guarantee that only one raisin build happen
> simultaneously?

Ian?

I suspect we don't want this, 
> 
> Given that the series is already at v7, I would prefer to remove the
> call to install-builddep, even though I think it is important, rather
> than make substantial modifications. However, if I do remove the call to
> install-builddep from ts-raisin-build, I would have to install any
> missing build dependencies somewhere else. Where would that be? In
> ts-xen-build-prep? Is that script even called for non-xen build jobs?

In ts-xen-build-prep, I think.

Ian.