Closed Bug 1275111 Opened 4 years ago Closed 4 years ago

Split out artifact download into separate tier

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: automatedtester, Assigned: mshal)

References

Details

Attachments

(1 file, 1 obsolete file)

My artifact builds have gone up in time but I am not sure if this is down to my internet connection getting worse or if there is a problem in configure.

Currently we lump artifact build download in configure and can't see if configure is getting slower for local builds.
See Also: → 1275673
This also fixes the issue of processing the artifacts twice in some
situations (bug 1275673). The artifact download no longer happens when a
specific target is passed to 'mach build', except in the special case of
'mach build mobile/android', since that is the typical build invocation
for Fennec developers. Otherwise, it only runs once during 'mach build'
after configure.

Review commit: https://reviewboard.mozilla.org/r/57208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57208/
Attachment #8759159 - Flags: review?(mh+mozilla)
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

nalexander, can you confirm this still behaves appropriately for Fennec workflows?
Attachment #8759159 - Flags: feedback?(nalexander)
FYI, I tried putting this in client.mk, but we don't have config.mk in there so MOZ_ARTIFACT_BUILDS is undefined. This seems to be the next best place as far as I can tell.
Assignee: nobody → mshal
(In reply to Michael Shal [:mshal] from comment #2)
> Comment on attachment 8759159 [details]
> MozReview Request: Bug 1275111 - Move artifact download/install into its own
> tier; r?glandium
> 
> nalexander, can you confirm this still behaves appropriately for Fennec
> workflows?

I'm going to redirect to mcomella.  Mike, would you mind testing this patch and ensuring artifact builds still work?  (Feel free to redirect further.)
Flags: needinfo?(michael.l.comella)
Attachment #8759159 - Flags: feedback?(nalexander) → feedback?(michael.l.comella)
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

https://reviewboard.mozilla.org/r/57208/#review54168

::: Makefile.in:248
(Diff revision 1)
>  include $(topsrcdir)/testing/testsuite-targets.mk
>  endif
>  endif
>  
>  default all::
> -	$(call BUILDSTATUS,TIERS $(TIERS) $(if $(MOZ_AUTOMATION),$(MOZ_AUTOMATION_TIERS)))
> +	$(call BUILDSTATUS,TIERS $(if $(MOZ_ARTIFACT_BUILDS),artifact) $(TIERS) $(if $(MOZ_AUTOMATION),$(MOZ_AUTOMATION_TIERS)))

I think it would be better to put artifact in TIERS directly. Then you can add

recurse_artifact:
    $(topsrcdir)/mach --log-no-times artifact install

to Makefile.in (no need for a ifdef, no need for manual TIER_START/TIER_FINISH).

::: mobile/android/Makefile.in:7
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +include $(topsrcdir)/config/config.mk
> +
> +# Since most Fennec developers do 'mach build mobile/android', this skips over

Why do they feel they have to do mach build mobile/android, btw? mach build shouldn't make a difference on artifact builds. I'm reluctant to special case mach build mobile/android when people should just do mach build. So unless there is a valid reason for them to do that, I'd rather not do this.
Attachment #8759159 - Flags: review?(mh+mozilla)
See Also: → 1273054
(In reply to Mike Hommey [:glandium] from comment #5)
> I think it would be better to put artifact in TIERS directly. Then you can
> add
> 
> recurse_artifact:
>     $(topsrcdir)/mach --log-no-times artifact install
> 
> to Makefile.in (no need for a ifdef, no need for manual
> TIER_START/TIER_FINISH).

Good idea - that's way better.

> ::: mobile/android/Makefile.in:7
> (Diff revision 1)
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +include $(topsrcdir)/config/config.mk
> > +
> > +# Since most Fennec developers do 'mach build mobile/android', this skips over
> 
> Why do they feel they have to do mach build mobile/android, btw? mach build
> shouldn't make a difference on artifact builds. I'm reluctant to special
> case mach build mobile/android when people should just do mach build. So
> unless there is a valid reason for them to do that, I'd rather not do this.

I don't know for sure - maybe mcomella or nalexander can chime in here. Perhaps it was a workflow developed when recursing was much slower? There is still an improvement in just doing 'mach build mobile/android' vs a 'mach build' with an artifact build - 4.6s vs 11.5s on my machine.
(In reply to Michael Shal [:mshal] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > I think it would be better to put artifact in TIERS directly. Then you can
> > add
> > 
> > recurse_artifact:
> >     $(topsrcdir)/mach --log-no-times artifact install
> > 
> > to Makefile.in (no need for a ifdef, no need for manual
> > TIER_START/TIER_FINISH).
> 
> Good idea - that's way better.
> 
> > ::: mobile/android/Makefile.in:7
> > (Diff revision 1)
> > >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> > >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > >  
> > > +include $(topsrcdir)/config/config.mk
> > > +
> > > +# Since most Fennec developers do 'mach build mobile/android', this skips over
> > 
> > Why do they feel they have to do mach build mobile/android, btw? mach build
> > shouldn't make a difference on artifact builds. I'm reluctant to special
> > case mach build mobile/android when people should just do mach build. So
> > unless there is a valid reason for them to do that, I'd rather not do this.
> 
> I don't know for sure - maybe mcomella or nalexander can chime in here.
> Perhaps it was a workflow developed when recursing was much slower? There is
> still an improvement in just doing 'mach build mobile/android' vs a 'mach
> build' with an artifact build - 4.6s vs 11.5s on my machine.

I am not currently aware of a reason other than habit (I have long counseled |mach build mobile/android| in preference to |mach build mobile/android/base|) and performance.
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57208/diff/1-2/
Attachment #8759159 - Attachment description: MozReview Request: Bug 1275111 - Move artifact download/install into its own tier; r?glandium → Bug 1275111 - Move artifact download/install into its own tier;
Attachment #8759159 - Flags: feedback?(michael.l.comella) → review?(mh+mozilla)
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

Feedback that this doesn't break android workflows, but note you may run into bug 1277841 until that is fixed.
Attachment #8759159 - Flags: feedback?(michael.l.comella)
Apologies - will get to this on Monday!
Attachment #8760419 - Attachment is obsolete: true
Attachment #8760419 - Flags: review?(s.kaspari)
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

With my work-around fix for bug 1277841 comment 7, I tried:
  1) Rebased this patch onto the current head & clobber build. Works correctly.
  2) `hg up` to the head this changeset was created on from reviewboard & clobber build. Works correctly.

It's worth mentioning that before this patch, the first `mach build` will build successfully and we can run gradle builds in the IDE to avoid calling `mach build` again, which allows us to work around bug 1277841. However, after the changeset in this bug, the first `mach build` no longer works so I'd recommend waiting until bug 1277841 is fixed before we land this patch.

Unrelated, I never use `mach build <path>` so I don't know what the affects of this patch might be on that invocation.
Flags: needinfo?(michael.l.comella)
Attachment #8759159 - Flags: feedback?(michael.l.comella) → feedback+
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

https://reviewboard.mozilla.org/r/57208/#review54970

::: mobile/android/Makefile.in:10
(Diff revision 2)
> +ifdef MOZ_ARTIFACT_BUILDS
> +default::
> +	$(topsrcdir)/mach --log-no-times artifact install
> +endif

Please file a followup to fix things that make people not use plain mach build, and remove this.
Attachment #8759159 - Flags: review?(mh+mozilla)
Comment on attachment 8759159 [details]
Bug 1275111 - Move artifact download/install into its own tier;

https://reviewboard.mozilla.org/r/57208/#review54972

Err, forgot to switch the review state.
Attachment #8759159 - Flags: review?(mh+mozilla)
Attachment #8759159 - Flags: review?(mh+mozilla) → review+
Filed bug 1280423.
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2597c1ed402f
Move artifact download/install into its own tier; r=glandium
https://hg.mozilla.org/mozilla-central/rev/2597c1ed402f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1282420
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.