Closed Bug 1403519 Opened 3 years ago Closed 3 years ago

Only run Sphinx tasks when Sphinx docs change

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla59

People

(Reporter: gps, Assigned: dustin)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

The Sphinx docs are essentially independent from everything else in mozilla-central. They don't impact builds or tests. We should be able to change scheduling to reflect this exclusive nature of the Sphinx tasks.

This may be a good test for SCHEDULES because the Sphinx docs are spread out all over the repo!
Better summary :)
Summary: Don't run builds, tests when Sphinx docs change → Only run Sphinx tasks when Sphinx docs change
Blocks: fastci
Assignee: nobody → dustin
(I can work on this but probably in the "week or two" range.  If it's more urgent, or you've already started, please feel free to reassign)
Comment on attachment 8914447 [details]
Bug 1403519 - only build docs when necessary

https://reviewboard.mozilla.org/r/185768/#review190692

::: browser/moz.build:12
(Diff revision 1)
>  CONFIGURE_SUBST_FILES += ['installer/Makefile']
>  
>  SPHINX_TREES['browser'] = 'docs'
>  
> +with Files('docs/**'):
> +    SCHEDULES.exclusive = ['docs']

I dislike that these need to be kept in sync.  I'm open to better ways of handling this!  I'm still very new to the fancy moz.build frontend technology.
Attachment #8914447 - Flags: review?(ted)
Attachment #8914447 - Flags: review?(ted) → review?(core-build-config-reviews)
Attachment #8914447 - Flags: review?(core-build-config-reviews) → review?(gps)
I'd prefer a WONTFIX over the silent treatment here.......
(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> I'd prefer a WONTFIX over the silent treatment here.......

I don't understand the SCHEDULES stuff so I'm not going to review, but this shouldn't be WONTFIX.  gps, bump this up your queue!
Flags: needinfo?(gps)
I'm behind in my queue due to work week and head cold. I'm working through things this morning...
Flags: needinfo?(gps)
Comment on attachment 8914447 [details]
Bug 1403519 - only build docs when necessary

https://reviewboard.mozilla.org/r/185768/#review204978

This seems reasonable.

I think there is room to mark ``**/*.py`` as inclusive as well. But it isn't strictly necessary.

Also, since we define the directories containing Sphinx docs in moz.build, we could probably change the moz.build layer to automagically emit equivalent Files() SCHEDULES data structures when those are defined. That would help avoid redundancy and ensure everything stays in sync. That's definitely follow-up territory though.
Attachment #8914447 - Flags: review?(gps) → review+
I think this can land, but I want to wait until bug 1426254 and bug 1426255 land, then bug 1403322, then check that I have all of the semantics right.  This is the first component that is used both exclusively (for the .rst) and inclusively (for the .py).
This is a rebase so mozreview will show lots of junk.  I discovered a few more SPHINX_TREES and added corresponding `Files(..): SCHEDULES.exclusive = ['docs']` and otherwise didn't change anything.  So I'll consider the r+ carried forward.

Some checks:

# some random file does not build docs..
(sandbox) dustin@lamport ~/p/m-c (bug1403519) $ ./mach file-info schedules LEGAL 
robocop, geckoview, reftest, web-platform-tests, macosx, talos, cppunittest, awsy, mochitest, web-platform-tests-reftests, web-platform-tests-wdspec, mozmill, telemetry-tests-client, linux, windows, marionette, firefox-ui, gtest, xpcshell, android, xpcshell-coverage

# Python change adds docs to the list of exclusive components..
(sandbox) dustin@lamport ~/p/m-c (bug1403519) $ ./mach file-info schedules python/mach_commands.py
robocop, mozmill, mochitest, marionette, web-platform-tests, docs, macosx, talos, cppunittest, awsy, geckoview, web-platform-tests-reftests, web-platform-tests-wdspec, linux, telemetry-tests-client, firefox-ui, reftest, windows, gtest, xpcshell, android, xpcshell-coverage

# Docs files only schedule docs tasks...
(sandbox) dustin@lamport ~/p/m-c (bug1403519) $ ./mach file-info schedules build/docs/locales.rst 
docs
(sandbox) dustin@lamport ~/p/m-c (bug1403519) $ ./mach file-info schedules taskcluster/docs/kinds.rst 
docs
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d539e4a02bd
only build docs when necessary r=gps
From the Bugzilla lint (which, I guess, traverses all moz.build's?)

[task 2018-01-11T20:20:44.572Z] ValueError: Two Files sections have set SCHEDULES.exclusive to differentvalues; these cannot be combined: [u'android'] and [u'docs']
Flags: needinfo?(dustin)
So the problem is in mobile/android/moz.build:
---
with Files('**'):
    BUG_COMPONENT = ('Firefox for Android', 'Build Config & IDE Support')
    SCHEDULES.exclusive = ['android']
...
with Files('docs/**'):
    SCHEDULES.exclusive = ['docs']
---

I tried re-ordering them and using FINALLY, but it didn't seem to help (I think FINALLY is for inheritance *between* moz.build files?)

Is there a way to say Files('**', except=['docs/**']), without explicitly listing all files and directories in this dir?
Flags: needinfo?(gps)
We don't have a mechanism to declare exclusions, no. What we can do is overwrite values since last write generally wins. Although IIRC SCHEDULES.exclusive might be implemented in a way that makes that difficult?
Flags: needinfo?(gps)
I suppose it's posisble to make that work for SCHEDULES.exclusive, but it feels strange for later declarations of that variable to override earlier, while SCHEDULES.inclusive doesn't work that way.  I could do something like

  SCHEDULES.exclusive.reset()
  SCHEDULES.exclusive = ['docs']

to be more explicit, but that starts to look functional and not like a config file.  I think the least adventurous option is just to list the files in mobile/android..
I changed my mind -- it doesn't feel strange, so I'll take that first option.
Comment on attachment 8943050 [details]
Bug 1403519: reset SCHEDULES.exclusive if set multiple times;

https://reviewboard.mozilla.org/r/213318/#review219076
Attachment #8943050 - Flags: review?(gps) → review+
The 'Bugzilla' task passed, meaning there are no more conflicts of this sort.
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b293bcd3fc7
only build docs when necessary r=gps
https://hg.mozilla.org/integration/autoland/rev/f53f8adcd578
reset SCHEDULES.exclusive if set multiple times; r=gps
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f0395687d36
only build docs when necessary r=gps
https://hg.mozilla.org/integration/autoland/rev/ce4e64aa7ea0
reset SCHEDULES.exclusive if set multiple times; r=gps
https://hg.mozilla.org/mozilla-central/rev/0f0395687d36
https://hg.mozilla.org/mozilla-central/rev/ce4e64aa7ea0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(dustin)
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.