Closed Bug 1408352 Opened 3 years ago Closed 3 years ago

./mach try no longer run linting tasks

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla58

People

(Reporter: emk, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Do I have to wait until landing the integration tree and backout to find linting errors? It is really inefficient...
We're trying to cut down on stuff that runs "automatically" in try, but it's still fine to run it if it's useful to you.

You can run the linter locally, or select the appropriate lint task in your try push.  I have an hg alias setup to run mozlint before pushing for review.  If you prefer to see it in try, you can use `--save` with `mach try fuzzy` to set up a saved task set that runs the linter as well as whatever other tests you'd like.
(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> You can run the linter locally,

$ mach eslint
eslint-plugin-react v7.1.0 needs to be installed locally.
eslint-plugin-no-unsanitized v2.0.1 needs to be installed locally.
eslint-plugin-spidermonkey-js vfile:tools/lint/eslint/eslint-plugin-spidermonkey-js needs to be installed locally.
sax v1.2.4 needs to be installed locally.
eslint-plugin-mozilla vfile:tools/lint/eslint/eslint-plugin-mozilla needs to be installed locally.
ini-parser v0.0.2 needs to be installed locally.
Installing eslint for mach using "d:\mozilla-build\node-v8.1.4-win-x64\npm.cmd install --loglevel=error"...
npm ERR! code EINVAL
npm ERR! EINVAL: invalid argument, read

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\kimu\AppData\Roaming\npm-cache\_logs\2017-10-13T12_49_21_912Z-debug.log

Error installing eslint, aborting.
Could not find eslint!  We looked at the --binary option, at the ESLINT
environment variable, and then at your local node_modules path. Please Install
eslint and needed plugins with:

mach eslint --setup

and try again.
A failure occured in the eslint linter.
? 1 problem (0 errors, 0 warnings, 1 failure)

:(

> or select the appropriate lint task in your
> try push.

Apparently it did not work for me.

> If you prefer to see it in try, you can use `--save` with `mach try
> fuzzy` to set up a saved task set that runs the linter as well as whatever
> other tests you'd like.

$ mach try fuzzy
Could not detect fzf, install it now? [y/n]: y
Git not found.
Failed to install fzf.

Please install fzf manually following the appropriate instructions for your
platform:

    https://github.com/junegunn/fzf#installation

Only the binary is required, if you do not wish to install the shell and
editor integrations, download the appropriate binary and put it on your $PATH:

    https://github.com/junegunn/fzf-bin/releases

:( :( :( (I already installed Git for Windows.)

on Windows, develpment is really uncomfortable these days...
Yuck.. Andrew, ideas?
Flags: needinfo?(ahalberstadt)
If you just want to prevent being backed out over lint errors though, I recommend using the vcs hooks instead of relying on try:
https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook

Of course to do that, you'll need to figure out the eslint setup errors you were getting. I'm not really sure what's happening there :/

For the git problem, you might have git installed in native Windows but not in the mozilla-build environment (or vice versa). But you don't really need git, for |mach try fuzzy| you just need the 'fzf' binary on your path somewhere. It might be easiest to just install it manually using the instructions linked in the error message rather than fighting with git.
Flags: needinfo?(ahalberstadt)
Thank you, installing fzf binary worked.

I filed bug 1408392 about the eslint problem.
(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> We're trying to cut down on stuff that runs "automatically" in try, but it's
> still fine to run it if it's useful to you.

I think it is moderately user hostile to not run lint tasks on Try. They /should/ be low load. So why not run them automatically?
note: I agree that the status quo is bad UX, so this is just me laying out a few arguments against doing this.

One reason boils down to implementation details (which is admittedly not a great reason). But there is so much complexity around taskgraph optimization. These X tasks need to run on these Y branches, but only when pushing with this Z mechanism on a full moon. Dustin has been doing some great work simplifying everything and making the process easier to understand and extend. 

There's also a slippery slope argument (also not a great reason). If we run lint tasks automatically, do we also run python unittests (I'd think yes)? Maybe also reftest sanity? Maybe any SCHEDULES task that takes less than 5 minutes?

I think a better approach to running the lint tasks automatically on try would involve:

1. Pushing the hooks harder, e.g add |mach bootstrap| support (bug 1295833)
2. Integrate |mach lint| with Jan's mozreview/phabricator static-analysis bot (bug ?)

If the lints are running as a hook or as part of review, then running them on try is likely just a waste of resources. If we can't fix these two things relatively quickly though, then maybe we should consider a hack to get them running automatically on try (temporarily).
I think this is actually one of the bigger issues that hobbled try syntax over the years: the conflict between "try should do what I (the committer) tell it to" and "try should do the things that make sense regardless of what I tell it".

This led us to the "try this" (what I tell it to) vs. "just try it" (things that make sense) approach, but we've since added "try nothing" (an empty try push allowing tasks to be added in treeherder).  "Just try it" is not yet ready (it runs too much) and at this point there is no way to do a try push with that mode, as a push without syntax is treated as "try nothing".

So things are still a little muddy, but that's the direction we want to go.
That all makes sense. Thanks for the context!

I also agree that moving linting to before Try is a noble goal: it is good UX to inform people of a problem ASAP. Just as long as hooks don't actively prevent commits from being completing: it is super annoying when you just want to hack on something and a hook prevents you from doing that! It's also annoying when hooks slow down VCS operations like rebase. But well-designed hooks can only run during specific operations :)
Yeah, the hook won't block a commit (but will block a push). Feel free to take a quick glance to double check it's doing things properly:
https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook
https://dxr.mozilla.org/mozilla-central/source/tools/lint/hooks.py

I've been pushing people who complain about lack on linters on try towards it, and so far haven't heard any complaints (aside from the eslint problem :emk has in this bug).
A lot of people have been complaining about this on irc. I think we need to come up with some paradigm to figure this out, even with the vcs hooks.

Previously, we automatically ran lint jobs because the try_syntax_parser simply always scheduled all "job" tasks. This was bad because it also included things other than lint, like toolchains, python unittests, etc. I think we need some kind of per-task configuration, maybe an "ignore-filters: true" config that makes these tasks bypass the target task filtering. I'll take a closer look at this.
I think I found a halfway decent solution. For any task that has the `ignore_filters` attribute set to True, the following would happen on try (bear in mind, on try 'not filtered out' == 'explicitly specified by user'):

not filtered out, not optimized away => task gets run
not filtered out, optimized away => task gets run
filtered out, not optimized away => task gets run
filtered out, optimized away => task does not get run

In other words, if I explicitly specify a lint task on try, it gets run no matter what. But if I don't specify it, it will still be considered for optimization and run if it needs to. This is despite the fact that `optimize_target_tasks` == False on try. This seems to behave the way we want, and gives us the ability to enable this behaviour on a per-task basis.

Here's a |mach try empty| run for this change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ee82b70b1c6ca741d7834756020cb08d37fc6df

Notice how the lint and python-unittest tasks were all scheduled even though the try_task_config.json is empty (they were all scheduled for this push because taskgraph/** was modified). The test in the patch also does a good job illustrating how this works.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
I'm worried we're making the same mistake I mentioned in comment 9.  Especially, the exception to optimize_target_tasks=False.

"Just try it" is, I think, still the ultimate goal we should be striving toward -- but I realize we're not close yet.

In the interim, could we do something like include a few tasks in every set of tasks generated by `mach try`?  If we run a few more lints than necessary, that's not the end of the world.
(In reply to Dustin J. Mitchell [:dustin] from comment #16)
> I'm worried we're making the same mistake I mentioned in comment 9. 
> Especially, the exception to optimize_target_tasks=False.

I think this will be a different scenario from the one we previously found ourselves in comment 9, mostly because we can apply this to hand picked tasks. Whereas before all "-j" tasks ran by default. The other difference is that this doesn't complicate any of the 'try syntax' or 'target tasks' logic at all. It lives outside of all that so is a lot easier to understand.

As for `optimize_target_tasks=False`, it isn't really an exception. If the `target_tasks_method` returns one of these tasks, they'll honour `optimize_target_tasks=False`. It's only if the `target_tasks_method` *doesn't* return one of these tasks that they get added with optimization enabled. When this happens they are added outside of the `target_tasks` process altogether (similar to image tasks), so it makes sense that they don't obey `optimize_target_tasks=False` in this case.


> "Just try it" is, I think, still the ultimate goal we should be striving
> toward -- but I realize we're not close yet.

I'd say this gets us a step closer.


> In the interim, could we do something like include a few tasks in every set
> of tasks generated by `mach try`?  If we run a few more lints than
> necessary, that's not the end of the world.

Only 10% of try pushes are using `try_task_config.json` (and 45% aren't even using |mach try| at all), so fixing this on the |mach try| side of things won't help many people. This needs to be solved in taskgraph (and I'd push for that regardless of usage).
Also meant to mention, 'ignore_filters' might be a bad name. I'm open to other suggestions!
Duplicate of this bug: 1413199
Comment on attachment 8923476 [details]
Bug 1408352 - [taskgraph] Implement 'always_target' attribute,

https://reviewboard.mozilla.org/r/194620/#review200694

::: taskcluster/docs/attributes.rst:178
(Diff revision 1)
>  toolchain-alias
>  ===============
>  For toolchain jobs, this optionally gives an alias that can be used instead of the
>  real toolchain job name in the toolchains list for build jobs.
> +
> +ignore_filters

What do you think about `always_target` as a name?

It might help if the description here talked about the intended use: tasks for which optimization is very effective and the benefits of always running (modulo optimization) outweigh the costs.

::: taskcluster/taskgraph/generator.py:262
(Diff revision 1)
>          # include all docker-image build tasks here, in case they are needed for a graph morph
>          docker_image_tasks = set(t.label for t in full_task_graph.tasks.itervalues()
>                                   if t.attributes['kind'] == 'docker-image')
> -        target_graph = full_task_graph.graph.transitive_closure(target_tasks | docker_image_tasks)
> +        # include all tasks with `ignore_filters` set
> +        ignore_filter_tasks = set(t.label for t in full_task_graph.tasks.itervalues()
> +                                  if t.attributes.get('ignore_filters'))

Maybe (in followup) we should set this for all docker image tasks :)

::: taskcluster/taskgraph/test/test_generator.py:95
(Diff revision 1)
>  
>          def target_tasks_method(full_task_graph, parameters):
>              return self.target_tasks
>  
>          target_tasks_mod._target_task_methods['test_method'] = target_tasks_method
> +        optimize_mod._make_default_strategies = make_fake_strategies

this should probably use mock.patch otherwise it will remain patched, right?
Attachment #8923476 - Flags: review?(dustin) → review+
Comment on attachment 8923477 [details]
Bug 1408352 - Add 'always_target' to lint and python unittest tasks,

https://reviewboard.mozilla.org/r/194622/#review200696
Attachment #8923477 - Flags: review?(dustin) → review+
Blocks: 1413936
Comment on attachment 8923476 [details]
Bug 1408352 - [taskgraph] Implement 'always_target' attribute,

https://reviewboard.mozilla.org/r/194620/#review200694

> What do you think about `always_target` as a name?
> 
> It might help if the description here talked about the intended use: tasks for which optimization is very effective and the benefits of always running (modulo optimization) outweigh the costs.

Sure, `always_target` works for me. Though to be pedantic, `target_tasks_method` is one of several possible filters this could be bypassing. I guess we don't really have any other kinds of filters though.

I'll make the docs a bit better too.

> Maybe (in followup) we should set this for all docker image tasks :)

Filed bug 1413936
I have approval to land through the soft code freeze on the firefox-ci list. I've also tested |mach taskgraph optimized| with a variety of parameters.yml and pushed to try with a variety of jobs.

Here is a try run showing a green 'tg' task:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=136a401bce2821d88a4b46aaa81d61673504ed88
Latest push was just because I forgot to update the commit message to reflect `ignore_filters` -> `always_target`.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d233eb8b619
[taskgraph] Implement 'always_target' attribute, r=dustin
https://hg.mozilla.org/integration/autoland/rev/6fa4468dd31e
Add 'always_target' to lint and python unittest tasks, r=dustin
https://hg.mozilla.org/mozilla-central/rev/4d233eb8b619
https://hg.mozilla.org/mozilla-central/rev/6fa4468dd31e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.