Closed Bug 1255450 Opened 8 years ago Closed 8 years ago

Runtime config for mach

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

This has been talked about before, but I can't find any existing bugs.

It would be great to have a runtime config (maybe ~/.machrc ?) that core mach (and by extension individual commands) could read from.

Just the ability for commands to have access to this is already huge, but two other immediate use cases I'd like are aliases and default arguments like mercurial has.
There is a alpha quality ini-based config mechanism in mach. See mach/config.py. It is disabled. Feel free to reincarnate it or ditch it completely.

I initially implemented "strongly typed" configs to help prevent cruft. This was inspired by the cargo culting in mozconfigs, which I hoped to avoid in mach config files.

I would very much like to see mach config files. We've had to put way too many hacks (e.g. undocumented environment variables) into mach commands for my liking.
I'm sort of leaning towards ripping out the existing implementation and using python/configobj instead. It seems configobj has some typing capabilities as well.

But I admit I don't fully understand how the existing system (SettingsProvider, etc) is supposed to work. What advantages would I be losing? How come the existing system isn't currently being used?
Flags: needinfo?(gps)
An advantage of the existing system is that all config options were self-documenting and strongly typed.

We never used the existing config system because we didn't have a good use case for it when mach was deployed many years ago. We also were stumped by questions like where to put the config files. I had also wanted the mach config files to be a replacement for mozconfigs (as opposed to say a generic config file for mach behavior).
Flags: needinfo?(gps)
Ok, I'll take another look at the existing system.

As for config locations, I'd vote $MOZBUILD_STATE_PATH/mach.ini and topsrcdir/mach.ini with the latter overriding the former (with additional configs passed via env or cmdline). Multiple config support would be required.
Regarding:
https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/main.py#498

I'm not sure I understand this comment, but I believe it means that after reading configuration in, it is no longer possible to tell which sections/options belong to which files should the user wish to modify configuration and write it back out.

If I understand that comment properly, then I disagree with it. Being able to read from multiple files has value, even if writing them back out is not yet implemented. Besides the ability for a user to write out configuration is purely theoretical at this point (until |mach settings section.option value| or similar exists). I propose we punt on the writing back out problem until there is actually an API exposed to the end user for doing so. In the meantime, editing config files manually is still a huge improvement on the status quo.
I made some good progress, but just realized that the current implementation is too well-defined. I want to implement things like:

[alias]
mochitest-plain = mochitest -f plain

This means config.py needs to allow arbitrary user-defined option names for some sections. I'll come up with something that makes this opt-out. So option names are well-defined by default, but a SettingsProvider can specify a particular section as allowing any option name. Likely by passing in a wildcard or similar.
Configuration files have been implemented for awhile but not enabled. In
addition to enabling configuration, this makes two major changes to the
initial implementation.

1) ConfigProvider classes were removed. Instead, the @SettingsProvider
makes sure that the class has a 'config_settings' attribute, and the
ConfigSettings class knows how to take it from there. I believe this
provides a simpler and more intuitive way of defining settings than
the previous method.

2) Adds support for wildcard options, e.g 'foobar.*'. This allows the
user to add arbitrary option names to the 'foobar' section. A side
effect of this change is that the ConfigSection class now represents
actual settings, not potential settings. E.g iterating through a
section now iterates through the options of the underlying configparser
instead of the settings definitions.

Review commit: https://reviewboard.mozilla.org/r/42097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42097/
Attachment #8734136 - Flags: review?(gps)
These config options can be defined in ~/.mozbuild/mach.ini or topsrcdir/.machrc.
Aliases and defaults work similar to the identically named options in an hgrc.

For example:

[alias]
browser-test = mochitest -f browser

[defaults]
build = faster

Review commit: https://reviewboard.mozilla.org/r/42101/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42101/
Attachment #8734138 - Flags: review?(gps)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

https://reviewboard.mozilla.org/r/42097/#review38937

I didn't look at this in too much detail. But it is on the right track.

I'd really appreciate if this were split into smaller patches to make review easier.

::: python/mach/docs/settings.rst:28
(Diff revision 1)
> +.. parsed-literal::
> +
> +    [alias]
> +    browser-test = mochitest -f browser
> +
> +* defaults - Specify default arguments to apply when running a command. This is useful if you always want a particular argument specified no matter what. For example:

An alias and a default are basically the same thing. Plus, when you have a [defaults] section, there is ambiguity as to whether defaults are active when an alias invokes said command.

Can we limit this to [alias]?

::: python/mach/mach/config.py:164
(Diff revision 1)
> -
> -    Classes implementing this interface expose configurable settings. Settings
> +    'abs_path': AbsolutePathType,
> +    'rel_path': RelativePathType,

We may not need these. When I first wrote the config code, I was planning to replace mozconfigs and that meant a better mechanism to define some paths. If you don't have a use for them, let's kill them. We can always add them later.

::: python/mach/mach/config.py:234
(Diff revision 1)
> +        def get_meta(self, option):
> +            if option in self._settings:
> +                return self._settings[option]
> +            if self._wildcard:
> +                return self._settings['*']
> +            raise KeyError('Option not registered with provider: %s' % option)

I know this was implemented before (by me). This behavior is wrong because if someone updates to an old revision, their mach config may fail to load due to lack of support for a config option.

I think we should use `warnings.warn()` instead.

::: python/mach/mach/main.py:522
(Diff revision 1)
> -        for provider in Registrar.settings_providers:
> -            provider.register_settings()
> -            self.settings.register_provider(provider)
> +            mach.ini, .mach.ini, machrc, .machrc
> +        """
> +        if isinstance(paths, basestring):
> +            paths = [paths]
>  
> -        p = os.path.join(self.cwd, 'mach.ini')
> +        valid_names = ('mach.ini', '.mach.ini', 'machrc', '.machrc')

This feels too complicated. Can we limit the files we look at?

I kinda like universally using `machrc` (or maybe `.machrc` in special cases).
Attachment #8734136 - Flags: review?(gps)
Attachment #8734137 - Flags: review?(gps)
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

https://reviewboard.mozilla.org/r/42099/#review38941

I didn't look at this in too much detail. My only question is: do you think supporting multiple locales is over engineering it? Practically none of our tools have translation support. While I'd like mach to go above and beyond, it feels like something we wouldn't use. But maybe there is demand for the tools to have limited support for non-English. It's at least worth a ping to mhoye.
Attachment #8734138 - Flags: review?(gps)
Comment on attachment 8734138 [details]
MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps

https://reviewboard.mozilla.org/r/42101/#review38943

I left a comment in the first patch that I think [defaults] is not necessary and can be expressed as an [alias].

I'll defer a full review until the first patches in the series are ready.

I do like having an [alias] section though!
(In reply to Gregory Szorc [:gps] from comment #10)
> An alias and a default are basically the same thing. Plus, when you have a
> [defaults] section, there is ambiguity as to whether defaults are active
> when an alias invokes said command.
> 
> Can we limit this to [alias]?

Sure, I kind of like [defaults] but don't feel strongly about it.

> I didn't look at this in too much detail. My only question is: do you think
> supporting multiple locales is over engineering it? Practically none of our
> tools have translation support. While I'd like mach to go above and beyond,
> it feels like something we wouldn't use. But maybe there is demand for the
> tools to have limited support for non-English. It's at least worth a ping to
> mhoye.

I think it's likely over-engineering, but it also might be the start of us taking l10n a bit more seriously. I guess it's already implemented so I'd be inclined to leave it for now and revisit removing it later if we want. Fwiw, the 'locale-gen' command works by prompting for missing translations for --locale. Leaving it blank finds all missing translations, or you can specify a particular option |mach settings locale-gen foo.bar|, so it makes adding new translations quite easy.

I'm not sure how to handle the mandatory en_US locale requirement though. Currently, not including an en_US translation will only break |mach settings| and nothing else. This means options without en_US won't be backed out, and |mach settings| will be perpetually broken. Alternatively we could check for en_US at dispatch time, but then all commands would break, including |mach settings locale-gen|. The only two viable options seem to be:
1. Drop the mandatory en_US locale requirement (and use some placeholder text instead)
2. Have a test that ensures the en_US locale requirement
https://reviewboard.mozilla.org/r/42097/#review38937

> I know this was implemented before (by me). This behavior is wrong because if someone updates to an old revision, their mach config may fail to load due to lack of support for a config option.
> 
> I think we should use `warnings.warn()` instead.

This doesn't raise at config load time (see load_fps and \_finalize), only while trying to get/set the option at runtime. So this shouldn't be a problem.
Some sections should support user-defined options. For example, in an [alias] section, the option names
are not well-defined, rather specified by the user. This patch allows user-defined option names for any
section that has a 'section.*' option defined. Even with 'section.*', option types are still well-defined.

Review commit: https://reviewboard.mozilla.org/r/42843/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42843/
Attachment #8734136 - Attachment description: MozReview Request: Bug 1255450 - Enable runtime configuration files for mach, r?gps → MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps
Attachment #8734137 - Attachment description: MozReview Request: Bug 1255450 - Add a |mach settings locale-gen| subcommand for generating l10n files for settings, r?gps → MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps
Attachment #8734138 - Attachment description: MozReview Request: Bug 1255450 - Create mach settings for defining aliases and defaults, r?gps → MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps
Attachment #8735553 - Flags: review?(gps)
Attachment #8735554 - Flags: review?(gps)
Attachment #8734136 - Flags: review?(gps)
Attachment #8734137 - Flags: review?(gps)
Attachment #8734138 - Flags: review?(gps)
Runtime configs have been implemented for awhile, but disabled. This patch
enables configuration. Config files will be loaded in the following order
(later files override earlier ones):

1. $MOZBUILD_STATE_PATH/machrc
2. topsrcdir/machrc
3. $MACH_SETTINGS_FILE
4. CLI via --settings

Review commit: https://reviewboard.mozilla.org/r/42845/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42845/
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42097/diff/1-2/
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42099/diff/1-2/
Comment on attachment 8734138 [details]
MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42101/diff/1-2/
https://reviewboard.mozilla.org/r/42845/#review39299

::: python/mach/mach/main.py:217
(Diff revision 1)
>  
>          self.cwd = cwd
>          self.log_manager = LoggingManager()
>          self.logger = logging.getLogger(__name__)
>          self.settings = ConfigSettings()
> +        self.settings_paths = []

Should this default to $HOME so the home directory always gets searched for '.machrc' too? I could lean either way.
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

https://reviewboard.mozilla.org/r/42097/#review39307

::: python/mach/docs/settings.rst:44
(Diff revision 2)
> +
> +Each tuple is of the form:
> +
> +.. code-block:: python
> +
> +    ('<section>.<option>', '<type>', '<default>', '<choices>')

Making this a tuple is a bit dangerous because I could see us wanting to add more things. Let's make the name, type, and default part of the tuple and an optional dict for everything else, which is just "choices" right now.
Attachment #8734136 - Flags: review?(gps)
Comment on attachment 8735553 [details]
MozReview Request: Bug 1255450 - [mach] Implement 'wildcard' settings for enabling sections with user-defined options, r?gps

https://reviewboard.mozilla.org/r/42843/#review39309
Attachment #8735553 - Flags: review?(gps) → review+
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

https://reviewboard.mozilla.org/r/42099/#review39311

::: python/mach/mach/dispatcher.py:143
(Diff revision 2)
> -        if handler.subcommand_handlers:
> -            if not args:
> +        if handler.subcommand_handlers and args:
> +            if len(args) == 1 and args[0] in ('help', '--help'):
> -                self._handle_subcommand_main_help(parser, handler)
> -                sys.exit(0)
> -            elif len(args) == 1 and args[0] in ('help', '--help'):

Why did this change?
Attachment #8734137 - Flags: review?(gps)
Comment on attachment 8735554 [details]
MozReview Request: Bug 1255450 - [mach] Enable runtime configuration files, r?gps

https://reviewboard.mozilla.org/r/42845/#review39313

::: .hgignore:27
(Diff revision 1)
>  ^configure$
>  ^old-configure$
>  ^config\.cache$
>  ^config\.log$
>  ^\.clang_complete
> -^mach.ini$
> +^\.?machrc$

We'll need to update .gitignore too.

::: python/mach/mach/main.py:373
(Diff revision 1)
> +        if 'MACH_SETTINGS_FILE' in os.environ:
> +            self.load_settings(os.environ['MACH_SETTINGS_FILE'])

Let's make this MACHRC for brevity.

::: python/mach/mach/main.py:522
(Diff revision 1)
> -        for provider in Registrar.settings_providers:
> -            provider.register_settings()
> -            self.settings.register_provider(provider)
> +            machrc, .machrc
> +        """
> +        if isinstance(paths, basestring):
> +            paths = [paths]
>  
> -        p = os.path.join(self.cwd, 'mach.ini')
> +        valid_names = ('machrc', '.machrc')

A few people have problems with hidden files. So let's limit this to `machrc`.
Attachment #8735554 - Flags: review?(gps)
Comment on attachment 8734138 [details]
MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps

https://reviewboard.mozilla.org/r/42101/#review39323

Nice.
Attachment #8734138 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/42099/#review39311

> Why did this change?

This is an edge case that should have been part of my patch in bug 1255470 but got missed. I can fix this in a new bug if you prefer.

This is what this patch is trying to do:

1) |mach settings| -> Print full config (this used to be |mach settings-create|)
2) |mach settings --list| -> Print condensed list of settings (this used to be |mach settings|)
3) |mach settings locale-gen| -> Generate localized documentation

Without this change, 1) and 3) are not possible together as if you run |mach settings| the subcommand help will be printed instead.
https://reviewboard.mozilla.org/r/42097/#review39307

> Making this a tuple is a bit dangerous because I could see us wanting to add more things. Let's make the name, type, and default part of the tuple and an optional dict for everything else, which is just "choices" right now.

I just noticed that 'choices' wasn't even implemented anywhere yet. And then I noticed values defined at config load time never get any validation. This is sort of ok right now because there's some implicit validation from things like `configparser.getboolean`, but there's nowhere to perform our own validation for stuff like 'choices'. We don't want to do this extra validation at config load time for the 'old commit' problem you mentioned in another comment, but it should at least happen on `__getitem__`.
https://reviewboard.mozilla.org/r/42845/#review39313

> A few people have problems with hidden files. So let's limit this to `machrc`.

Personally I like to keep all my configuration hidden. As long as we don't have automated processes creating these files I don't see the harm in leaving it as is. People who dislike hidden files are free to not create hidden files.

But if you insist, I guess I can use the environment variable to keep my machrc hidden.
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42097/diff/2-3/
Attachment #8734136 - Flags: review?(gps)
Attachment #8734137 - Flags: review?(gps)
Attachment #8735554 - Flags: review?(gps)
Comment on attachment 8735553 [details]
MozReview Request: Bug 1255450 - [mach] Implement 'wildcard' settings for enabling sections with user-defined options, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42843/diff/1-2/
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42099/diff/2-3/
Comment on attachment 8735554 [details]
MozReview Request: Bug 1255450 - [mach] Enable runtime configuration files, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42845/diff/1-2/
Comment on attachment 8734138 [details]
MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42101/diff/2-3/
Attachment #8734136 - Flags: review?(gps)
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

https://reviewboard.mozilla.org/r/42097/#review40993

::: python/mach/mach/config.py:257
(Diff revision 3)
>              # Prune empty sections.
>              if not len(self._config.options(self._name)):
>                  self._config.remove_section(self._name)
>  
>          def __getattr__(self, k):
>              return self.__getitem__(k)

As implemented, this will raise KeyError for unknown options. __getattr__ should raise AttributeError. That's a bug as it can confuse some Python internals that expect attribute lookups to raise AttributeError.

You may want to create a custom exception type to represent an unknown option and re-raise a KeyError or AttributeError as appropriate. Same goes for __setattr__.

::: python/mach/mach/test/test_config.py:100
(Diff revision 3)
>          self.assertNotIn('foo', s)
>  
> +    def test_duplicate_option(self):
> +        s = ConfigSettings()
> +
> +        with self.assertRaises(Exception):

Doesn't this raise a more specific error?

::: python/mach/mach/test/test_config.py:204
(Diff revision 3)
> +        with self.assertRaises(ValueError):
> +            foo.xyz

It feels weird that accessing raises a ValueError. But I suppose the important thing is an exception is raised. meh.
https://reviewboard.mozilla.org/r/42099/#review39311

> This is an edge case that should have been part of my patch in bug 1255470 but got missed. I can fix this in a new bug if you prefer.
> 
> This is what this patch is trying to do:
> 
> 1) |mach settings| -> Print full config (this used to be |mach settings-create|)
> 2) |mach settings --list| -> Print condensed list of settings (this used to be |mach settings|)
> 3) |mach settings locale-gen| -> Generate localized documentation
> 
> Without this change, 1) and 3) are not possible together as if you run |mach settings| the subcommand help will be printed instead.

Please separate this into another bug and/or patch.
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

https://reviewboard.mozilla.org/r/42099/#review41011

::: python/mach/mach/commands/settings.py:17
(Diff revision 3)
>      CommandProvider,
>      Command,
> +    SubCommand,
>  )
>  
> +from mozbuild.base import MachCommandBase

This breaks an abstraction layer.

mach is supposed to be a standalone package with only minimal 3rd party dependencies. mozbuild comes from mozilla-central and doesn't have a stable API nor regular PyPI releases. So, you can't import it.

Sadly, this means the virtualenv foo you are using can't be used :/ You'll probably have to catch ImportError for polib and print a message telling people to `pip install` it :/

::: python/mach/mach/commands/settings.py:32
(Diff revision 3)
> -    @Command('settings-list', category='devenv',
> +    @Command('settings', category='devenv',
> -        description='Show available config settings.')
> +             description='Show available config settings.')
> -    def list_settings(self):
> -        """List available settings in a concise list."""
> +    @CommandArgument('-l', '--list', dest='short', action='store_true',
> +                     help='Show settings in a concise list')

I'm not a fan of commands with subcommands having their own default behavior. I know it is more typing, but I'd rather do `mach settings list` to keep the interface consistent. Otherwise, it introduces confusion as to whether a command is a normal command or has subcommands.

If we do this pattern elsewhere, I consider it a bug and a UI issue.
Attachment #8734137 - Flags: review?(gps)
Comment on attachment 8735554 [details]
MozReview Request: Bug 1255450 - [mach] Enable runtime configuration files, r?gps

https://reviewboard.mozilla.org/r/42845/#review41017

::: python/mach/mach/main.py:371
(Diff revision 2)
> +        self.load_settings(self.settings_paths)
> +
> +        if 'MACHRC' in os.environ:
> +            self.load_settings(os.environ['MACHRC'])

One potential issue I can see here is the inability to disable a settings file.

The way Mercurial works is that the system, user, and local paths are all loaded by default (in that order). However, if HGRCPATH is set, it overrides the defaults and loads the specified file plus the repo local file. This is convenient because sometimes you wish to run without global settings enabled.

I could see us disabling the global machrc if MACHRC is set. However, I'm on the fence about what to do about the topsrcdir machrc. On one hand, it is local and should always apply. On the other, people can and do have multiple objdirs per source directory. So, you could imagine they may want to disable the topsrcdir config for a particular objdir.

Speaking of multiple objdirs per source directory, we'll likely need to come up with a mechanism for doing that. Perhaps we can sneak something into mozconfigs and write out a pointer to the machrc in mozinfo.json or something. But that can be a follow-up bug.
Attachment #8735554 - Flags: review?(gps)
Depends on: 1262495
https://reviewboard.mozilla.org/r/42099/#review41011

> I'm not a fan of commands with subcommands having their own default behavior. I know it is more typing, but I'd rather do `mach settings list` to keep the interface consistent. Otherwise, it introduces confusion as to whether a command is a normal command or has subcommands.
> 
> If we do this pattern elsewhere, I consider it a bug and a UI issue.

The way I think of it is `locale-gen` is meant for mach command authors and should be hard to find for regular users. So to me `mach settings` is the primary (and only) interface for end users. While `mach settings locale-gen` is a tucked away convenience command that is only useful to a handful of people. I decided to use a subcommand for it so that it is obvious it only applies to settings, and so that is kept out of the main help.

If you still aren't convinced, I'd prefer to implement 'locale-gen' as a new command, and leave |mach settings| as is.
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42097/diff/3-4/
Attachment #8734136 - Flags: review?(gps)
Attachment #8734137 - Flags: review?(gps)
Attachment #8735554 - Flags: review?(gps)
Comment on attachment 8735553 [details]
MozReview Request: Bug 1255450 - [mach] Implement 'wildcard' settings for enabling sections with user-defined options, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42843/diff/2-3/
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42099/diff/3-4/
Comment on attachment 8735554 [details]
MozReview Request: Bug 1255450 - [mach] Enable runtime configuration files, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42845/diff/2-3/
Comment on attachment 8734138 [details]
MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42101/diff/3-4/
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

https://reviewboard.mozilla.org/r/42097/#review42017
Attachment #8734136 - Flags: review?(gps) → review+
Attachment #8734137 - Flags: review?(gps) → review+
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

https://reviewboard.mozilla.org/r/42099/#review42021

::: python/mach/mach/commands/settings.py:40
(Diff revision 4)
>  
> -    @Command('settings-list', category='devenv',
> +    @Command('settings', category='devenv',
> -        description='Show available config settings.')
> +             description='Show available config settings.')
> -    def list_settings(self):
> -        """List available settings in a concise list."""
> -        for section in sorted(self.settings):
> +    @CommandArgument('-l', '--list', dest='short', action='store_true',
> +                     help='Show settings in a concise list')
> +    def manipulate_settings(self, short=None):

"manipulate_settings" feels like the wrong name for this (not that it matters that much).
Comment on attachment 8735554 [details]
MozReview Request: Bug 1255450 - [mach] Enable runtime configuration files, r?gps

https://reviewboard.mozilla.org/r/42845/#review42023

Yay!
Attachment #8735554 - Flags: review?(gps) → review+
Comment on attachment 8734136 [details]
MozReview Request: Bug 1255450 - [mach] Replace ConfigProvider class with config_settings attribute, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42097/diff/4-5/
Comment on attachment 8735553 [details]
MozReview Request: Bug 1255450 - [mach] Implement 'wildcard' settings for enabling sections with user-defined options, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42843/diff/3-4/
Comment on attachment 8734137 [details]
MozReview Request: Bug 1255450 - [mach] Simplify managing of locale documentation for settings, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42099/diff/4-5/
Comment on attachment 8735554 [details]
MozReview Request: Bug 1255450 - [mach] Enable runtime configuration files, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42845/diff/3-4/
Comment on attachment 8734138 [details]
MozReview Request: Bug 1255450 - [mach] Create setting for defining command aliases, r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42101/diff/4-5/
Blocks: 1263936
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.