Overhaul |mach try| preset mechanism

RESOLVED FIXED in Firefox 67

Status

enhancement
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks 2 bugs)

Trunk
mozilla67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

5 months ago

With many |mach try| subcommands, you can save and load presets. E.g:

$ ./mach try fuzzy --save foobar -q "'some 'query"
$ ./mach try fuzzy --preset foobar

However, there are a lot of problems with this system. Most of them stem from the fact that the storage backend isn't adequate enough. It's a key/value store in an ini file (with subcommand names as the section headers). This makes it hard to save things that require more than one value.

Instead, I propose we use a format as follows (using yaml in the example):

foo:
    subcommand: fuzzy
    args:
        queries:
            - 'some 'query
            - 'someotherquery
        artifact: false
        env:
            CUSTOM_ENV: value
bar:
    subcommand: syntax
    args:
        syntax: -b o -p linux -u mochitest
        ...

This simply stores the subcommand to run, and the arguments to pass into that subcommand. This makes it flexible enough to store as much (or as little) information along with the preset as desired.

Another benefit is that subcommands don't have to worry about loading presets at all. We could even restrict the ability to use --preset unless passed to the root |mach try| command. We could possibly even handle --save outside of the subcommands (though I'm on the fence about this, because it would mean no --save with fuzzy interactive mode).

Completing this task will allow us to fix several outstanding preset bugs, as well as provide a lot of new goodness.

Assignee

Updated

5 months ago
Assignee

Updated

5 months ago
Severity: normal → enhancement
Assignee

Updated

5 months ago
Priority: -- → P2
Assignee

Updated

4 months ago
Blocks: 1524949
Assignee

Updated

4 months ago
Assignee: nobody → ahal
Priority: P2 → P1
Assignee

Updated

4 months ago
Status: NEW → ASSIGNED
Assignee

Updated

4 months ago
Duplicate of this bug: 1507710
Assignee

Comment 2

4 months ago

Status update: this is mostly done. Just need to update some tests/documentation and get it reviewed. Hopefully landed in the next week or two.

Assignee

Updated

4 months ago
Duplicate of this bug: 1518542
Assignee

Updated

4 months ago
No longer blocks: 1507710, 1518542
Assignee

Updated

4 months ago
Blocks: 1529047
Assignee

Comment 4

4 months ago

For mach commands that have 'pass_context=True', we should implicitly add the
handler instance to the context. This will give mach command implementations an
easy way to access things like the command/subcommand names, the parser, argv
list, etc.

Assignee

Comment 5

4 months ago

I forgot to remove this after re-implementing without this dependency.

Depends on D20521

Comment 8

4 months ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9afd71742756
[mach] Stuff the 'handler' instance into the context when applicable r=firefox-build-system-reviewers,mshal
https://hg.mozilla.org/integration/autoland/rev/d7a45b84e063
[tryselect] Remove unused 'flask-wtf' dependency from |mach try chooser| r=gbrown
https://hg.mozilla.org/integration/autoland/rev/1b5a4da48de3
[tryselect] Store all arguments when saving a preset r=gbrown
https://hg.mozilla.org/integration/autoland/rev/24105ffcf6f5
[tryselect] Migrate old preset files to the new format r=gbrown
Assignee

Comment 10

4 months ago

Sadly there was a bug in the preset migration despite the test. If you are here because of warnings about presets not being migrated, please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1530775#c3

Depends on: 1530775
You need to log in before you can comment on or make changes to this bug.