1243165 - Making a change to a test manifest shouldn't cause us to re-download and install artifacts

Status: NEW

(Firefox Build System :: General, defect)

Description

[Dave Townsend [:mossop]]

I did an artifact build this morning to do some work on a test failure. Later I moved the test to a different test manifest file and then ran "mach xpcshell-test" at which point it decided to download new artifacts and install them meaning my check of a single test change took two minutes :(

Comment 1

[Nick Alexander :nalexander [he/him]]

Mossop: there's nothing that would make you download new artifacts based on content.  I'm quite confident you witnessed the following situation.  First, |hg up -r fx-team|.  Discover there are no artifacts for fx-team available (presumably, the builds haven't finished).  Later, |mach build| or similar (install test manifests).  Have |mach artifact install| run and discover there are now artifacts available, and download them.

This is expected, if surprising -- it's part of the race window.  This would be addressed by your suggested command to |hg up| to the latest commit for which artifacts are available.

I will leave this to chmanchester, who may have further comment, or may elect to close.  (We should address this, of course -- probably by implementing the above command.)

Comment 2

[Dave Townsend [:mossop]]

I'm pretty sure the situation was that when I first built it downloaded one set of artifacts and then later newer ones closer to my changeset were available and so they were downloaded and installed, yes.

I would argue that if the only change I've made is to an xpcshell test manifest and I'm only wanting to run xpcshell tests that we shouldn't even be spending the time running artifact install. Even without there being a new artifact available it still adds a few seconds to the run time.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Dave Townsend [:mossop] from comment #2)
> I'm pretty sure the situation was that when I first built it downloaded one
> set of artifacts and then later newer ones closer to my changeset were
> available and so they were downloaded and installed, yes.

Good, we're in agreement.

> I would argue that if the only change I've made is to an xpcshell test
> manifest and I'm only wanting to run xpcshell tests that we shouldn't even
> be spending the time running artifact install. Even without there being a
> new artifact available it still adds a few seconds to the run time.

Possibly.  I made artifact install run with every configure (technically, config.status run) and with every |mach build| run.  I'm guessing the xpcshell test run is using something that does a |mach build install-tests| or similar, and hence triggers artifacts.  I would be happy for someone (not me, at least not right now) to do the legwork figure out what's happening here and make sure we install the tests without triggering the artifact install.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to Dave Townsend [:mossop] from comment #2)
> > I'm pretty sure the situation was that when I first built it downloaded one
> > set of artifacts and then later newer ones closer to my changeset were
> > available and so they were downloaded and installed, yes.
> 
> Good, we're in agreement.
> 
> > I would argue that if the only change I've made is to an xpcshell test
> > manifest and I'm only wanting to run xpcshell tests that we shouldn't even
> > be spending the time running artifact install. Even without there being a
> > new artifact available it still adds a few seconds to the run time.
> 
> Possibly.  I made artifact install run with every configure (technically,
> config.status run) and with every |mach build| run.  I'm guessing the
> xpcshell test run is using something that does a |mach build install-tests|
> or similar, and hence triggers artifacts.  I would be happy for someone (not
> me, at least not right now) to do the legwork figure out what's happening
> here and make sure we install the tests without triggering the artifact
> install.

Right -- this is inherent to the integration between the test manifests, the build system, and mach artifact: installing tests depends on the build backend files, the recursive make build backend depends on the test manifest files, and the target for the backend files executes config.status, which runs |./mach artifact install|. We're re-working the relationship between the build system and test manifests, but there might be decent reasons to have running tests depend on config.status.

It seems like updating to the last revision with artifacts available is almost always the right thing in this situation, so it's worth considering building this in.

Another approach would be to change the integration point for |./mach artifact install| so it runs less often, perhaps only when someone literally types |./mach build| in their terminal. Nick, what would you think of that?

Comment 5

[Chris Manchester (limited bugmail, email directly)]

(In reply to Chris Manchester [:chmanchester] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > (In reply to Dave Townsend [:mossop] from comment #2)
> > > I'm pretty sure the situation was that when I first built it downloaded one
> > > set of artifacts and then later newer ones closer to my changeset were
> > > available and so they were downloaded and installed, yes.
> > 
> > Good, we're in agreement.
> > 
> > > I would argue that if the only change I've made is to an xpcshell test
> > > manifest and I'm only wanting to run xpcshell tests that we shouldn't even
> > > be spending the time running artifact install. Even without there being a
> > > new artifact available it still adds a few seconds to the run time.
> > 
> > Possibly.  I made artifact install run with every configure (technically,
> > config.status run) and with every |mach build| run.  I'm guessing the
> > xpcshell test run is using something that does a |mach build install-tests|
> > or similar, and hence triggers artifacts.  I would be happy for someone (not
> > me, at least not right now) to do the legwork figure out what's happening
> > here and make sure we install the tests without triggering the artifact
> > install.
> 
> Right -- this is inherent to the integration between the test manifests, the
> build system, and mach artifact: installing tests depends on the build
> backend files, the recursive make build backend depends on the test manifest
> files, and the target for the backend files executes config.status, which
> runs |./mach artifact install|. We're re-working the relationship between
> the build system and test manifests, but there might be decent reasons to
> have running tests depend on config.status.
> 
> It seems like updating to the last revision with artifacts available is
> almost always the right thing in this situation, so it's worth considering
> building this in.
> 
> Another approach would be to change the integration point for |./mach
> artifact install| so it runs less often, perhaps only when someone literally
> types |./mach build| in their terminal. Nick, what would you think of that?

I guess you're saying we should do exactly that, but perhaps you can provide more context on the decision to initially put it in config.status.

Comment 6

[Nick Alexander :nalexander [he/him]]

> > Another approach would be to change the integration point for |./mach
> > artifact install| so it runs less often, perhaps only when someone literally
> > types |./mach build| in their terminal. Nick, what would you think of that?
> 
> I guess you're saying we should do exactly that, but perhaps you can provide
> more context on the decision to initially put it in config.status.

Because the build system is weird!  If you don't have a configured tree, the very first |mach build| will run configure and then run the build, but without calling |mach build| again.  There's a delicate interaction with updating the dist/bin install manifest, and it can't handle this.  That is, the |mach build| installs artifacts, but can't update the (non-existent) dist/bin install manifest; then the configure kicks in, creating the dist/bin install manifest; then the build continues, and removes the artifacts we just installed.  Installing artifacts during configure solves this pretty neatly.

Comment 7

[Mike Hommey [:glandium]]

Note that now that artifacts builds are using the hybrid FasterMake+RecursiveMake backend, the dist/bin install manifest is not used anymore. Instead, faster make's various install manifests for dist/bin/* are used, and they don't require the feeding from the list of artifacts.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I noticed the same today when I run |mach build package-tests|. A new artifact build got downloaded and delayed the command.