Prevent option shadowing.

Review Request #3035 — Created Oct. 24, 2015 and submitted

benjyw
pants
4f296a4...
pants-reviews
nhoward_tw, stuhood, zundel

It was unused (at least deliberately, see below...), confusing, and
required some complicated logic to support.

Highlights:

  • Simplifies OptionValueContainer. We still need some magic to support
    ranked values, but we no longer need option forwarding magic, which
    was a big part of the complication in that file. Now, instead of its
    own logic being largely (and complicatedly) implemented in terms of
    hasattr/setattr/getattr, it simply wraps a key->rankedvalue dict,
    and there's a small wrapper of __getattr__/__setattr__ magic, just to
    support direct attribute access.

  • Also removes the ability to access an option's value by any of its registered
    aliases. This wasn't used in practice, and complicated the code needlessly.
    So for example, in the rare case where we register an option with two names, say
    -f, --foo, we can now only access the value as opts.foo, not opts.f. But
    we have almost no such options anyway, so this is hardly important. Now you
    must access the option by its 'dest' name, as determined by argparse, namely
    the first name in the registration list with len(name) > 1.

  • We now detect attempts to shadow options, and error out on them.
    Unfortunately, we had quite a few unintentional shadowed options in
    the codebase (and some of these were quite confusing, so it's a good
    thing we got rid of them). Each such case was fixed by one of the
    following means:

    • Renaming the option.

    • Deferring to the global option of the same name (in some cases
      making that option recursive). This meant expanding the meaning of
      the global option, but in a sensible way. E.g., previously the
      global --fail-fast meant "fail fast on parsing", but now it means
      "fail fast in whatever way makes sense to the code using the option".

    • Removing the inner option. E.g., the filter task had a --tag option that
      wasn't necessary: the global --tag option already filters targets
      by tags, and the task had a more general --tag-regex option already.

    • Removing the outer option. Specifically, since so many tasks register
      a --version option, it made no sense for the global scope to hog that
      option name for itself. So now only -V and --pants-version work for
      getting the version of pants itself. Seems reasonable, especially since
      you already had to use pants_version in config, due to some special-casing.

    • Changing task registration names. E.g., the JvmBundle task was
      registered as bundle under goal bundle, meaning that its
      options scope was bundle.bundle, elided to just bundle.
      Which meant that any other task in the bundle goal could shadow its options.
      Renaming the task to bundle.jvm fixes this issue, and is also
      a good idea anyway. There's no reason to privilege JVM in this way.

Note that unfortunately it was not feasible to do a deprecation cycle
on these option changes, as they interact with the code in subtle ways,
and getting deprecation right would have been a lot of work.
However they were mostly config-only options, and migrate_config.py should
deal with them. When this rolls out, repo owners are going to have
to pay extra attention to potential issues in their repos, but that
was going to be true anyway, no matter how this change shaped up.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/87215317

ST
  1. Great cleanup!

  2. build-support/bin/ci.sh (Diff revision 1)
     
     

    Allowing tasks to use version probably makes sense, but I think that it would be good to preserve the ability to run ./pants --version, one way or another. It's always a bit annoying when none of --version, -version, or -v do anything for a unix process.

    Could we special case "exactly one argument to pants and it is --version"?

    1. Well, to start with we can definitely make -v do the right thing.

      We could special-case -version, perhaps in a followup though?

  3. Should we invert this and rename the global plugins flag to pants-plugins instead?

    Since it is loaded from the [DEFAULT] section of pants.ini, the current name is already somewhat ambiguous.


    Which makes me wonder... what would happen if all of the global options were instead in a pants-scoped global subsystem?

    Then, with a very small special case to scope the options that immediately follow ./pants on the command line to the pants subsystem, you'd get working positional/short options passed to that subsystem, without ambiguity about the version or plugins flags.

    Also, it would immediately suck all of the global options out of the [DEFAULT] section and into a [pants] section, which I think we've wanted for a while.

    1. Yeah, renaming the global --plugins to --pants-plugins makes total sense. In general, we should protect the global scope from hogging useful generic terms like 'plugins'. It's an advanced option anyway, so it doesn't matter if the name is clunky (not that pants_plugins is that clunky).

      I initially really liked your idea of having the global subsystem be pants. However the more I think about it the more complicated it gets. For example, tasks can access global options via their own options (self.get_options().spec_excludes instead of self.options.for_global_scope().spec_excludes), so there would have to be a big sweep removing that (and I actually find that convenient in some cases). It also means that implementing recursive options gets more complicated. And of course, it would be a hassle for users to do --pants-quiet where previously they could just do --quiet or just -q.

      But what I think you've hit on is that there should be a very small set of truly global useful-on-the-cmd-line options: -q, -z, -l etc. And the rest (including the bootstrap options) can be in a pants scope. But obvi I'd like to do that in a separate change, because that's no small thing.

      For now I'm confortable changing --plugins to --scalac-plugins - I think this should happen anyway, because compile.zinc now invokes multiple compilers, and I see no reason to privilege scalac. This is less true of --protoc-plugins, but I see no harm in being explicit even there. You never know how tasks and the tools they invoke might evolve.

    2. That all sounds very reasonable, thanks.

      And of course, it would be a hassle for users to do --pants-quiet where previously they could just do --quiet or just -q.

      Since the arguments would be positional, I believe that ./pants -q or ./pants --quiet could continue to work unambiguously... in any other position you'd need to scope them to pants.

      For now I'm confortable changing --plugins to --scalac-plugins - I think this should happen anyway, because compile.zinc now invokes multiple compilers, and I see no reason to privilege scalac. This is less true of --protoc-plugins, but I see no harm in being explicit even there. You never know how tasks and the tools they invoke might evolve.

      Yep, sounds good.

  4. 
      
ST
  1. Preemptive shipit with pants-plugins moved to another scope, and -v fixed.

    1. Er... I mean: plugins renamed to pants-plugins.

    2. Do you mind if instead of renaming it here, I move it (and many of its brethren) to a 'pants' scope, as discussed above, in a follow-up change?

    3. That would be lovely too.

  2. 
      
BE
BE
  1. Thanks Stu! More options improvements to come. Submitted as 9c2fc4777623176ef3f41ad20da5addc7ad5e759.

  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

9c2fc4777623176ef3f41ad20da5addc7ad5e759

ZU
  1. 
      
  2. s/compile.zinc/gen.protoc/

    This is an option passed to the protoc executable command line. It was be renamed to another option name in [gen.protoc]

    [gen.protoc]
    protoc_plugins: [ 'foo' ]
    
    1. Ooops, that was a copy-paste error. Will push a fix.

  3. 
      
ZU
  1. 
      
  2. Need to add binary.jvm also

    1. I don't follow. Didn't I do that in line 362?

    2. Yep you did. My mistake. The problem I had was in a test, so it wasn't going to be detected by scanning pants.ini anyway.

  3. 
      
Loading...