Kill the global `--ng-daemons` flag.

Review Request #1852 — Created March 2, 2015 and submitted

jsirois
pants
jsirois/issues/1181/single_nailgun_flag
1181, 1191
1851
76a1bc9...
pants-reviews
benjyw, davidt, peiyu, zundel
Since Config DEFAULT'ing now works for all scopes whether present on the
config files or not, the `--use-nailgun` flag exported by all
NailgunTasks suffices to allow both global (only via Config DEFAULT) and
local configuration.

 .travis.osx.yml                                               |  2 +-
 .travis.yml                                                   |  2 +-
 build-support/bin/ci.sh                                       | 11 ++++-------
 src/python/pants/backend/jvm/tasks/nailgun_task.py            |  5 ++---
 src/python/pants/option/global_options.py                     |  2 --
 tests/python/pants_test/backend/jvm/tasks/BUILD               |  8 ++++----
 tests/python/pants_test/backend/jvm/tasks/test_ivy_imports.py |  2 +-
 tests/python/pants_test/backend/jvm/tasks/test_ivy_resolve.py |  2 +-
 tests/python/pants_test/jvm/nailgun_task_test_base.py         |  2 +-
 9 files changed, 15 insertions(+), 21 deletions(-)
Tested migration script manually by editing pants.ini and
migrate_config.py notes dict to trigger boolean migration path bugfix, 
no migration path new feature, and no migration path with notes new
feature.

CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/53003473
ZU
  1. Add migration info to python/pants/options/migrate_config.py

    1. OK, I think I got them all via this:

      $ PANTS_DEV=1 ./pants --help-all 2>/dev/null | grep -E "\-use-nailgun" | cut -d']' -f2 | sed -r -e "s|-use-nailgun||" | tr - .
      binary
      binary.dex
      binary.dup
      bundle
      bundle.dup
      compile.scalastyle
      compile.scala
      compile.apt
      compile.java
      compile.checkstyle
      detect.duplicates
      gen.antlr
      gen.jaxb
      gen.scrooge
      imports.ivy.imports
      jar
      publish
      resolve.ivy
      thrift.linter
      
  2. 
      
JS
PE
  1. Ship It!
  2. 
      
BE
  1. Ship It!
  2. 
      
JS
JS
ZU
  1. 
      
  2. pants.ini (Diff revision 2)
     
     

    Thanks, that change got left out of my PR a week or so ago when I forgot to push one last time before merging.

  3. It looks like using the global flag may have given us the behavior we wanted, being able to override on a per config section basis. Maybe we should be removing --use-nailgun from each task option and then renaming the global flag ng_daemons to --use-nailgun?

    In my pants environment, I set ng_daemons to "True" globally, then set it to false in the compile.java section. I still got nailgun to run for the ivy step, but it launched jmake directly from the compile.java step.

    Then I set ng_daemons to "False" globally, then set it to true in the compile.java section. Things seemed to work properly in that case too. I got no nailgun on the ivy step, but jmake was launched with nailgun.

    1. The trouble is then every task has that flag, even if it doesn't use ng. Even if it doesn't use the jvm backend at all.

      This may not be such a bad thing - it so happens that JVM tasks are the only things that use JVM tools, but it doesn't have to be that way in principle. You could imagine a tool that runs on the JVM but does stuff unrelated to compiling JVM code. So maybe it's reasonable for "nailgun" to be a core concept.

    2. Per below I think not.  The config fix this depends on in r1851 provides global configurability for mixin flags like this via config only so beyond migration pain killing the global flag provides practical feature parity and reduces to flags to 1 for confusion lessening.
    3. Eric - is this a blocking objection?  Afaict this is the last un-shipited issue.
    4. Ok, I concur.

  4. 
      
ST
  1. 
      
  2. Is there a way that def register_options could 'require' (but not declare) that a global option exists? Right now the flag is duplicated in child opts, and it would be nice if it (mostly) lived in one place.

    1. It's "duplicated" in the sense that it's registered multiple times, once per task that needs it, but there's no code duplication. So it does live in one place in that sense. I think I prefer this to having it be global, which would mean that every task inherits it, not just the tasks that use it.

    2. I can put a point on Benjy's preference: If I use pants in a python only repo or only for python (Aurora) I would be confused and offput by seeing --[no-]use-nailguns in my help.
      Further, at least today, stuffing something in global_options is a privilege only tasks authored in the pantsbuild/pants repo can leverage today.  Its not a generally escape hatch; ie: we can't reasonably ask an author of a set of plugins to move their plugins into pantsbuild/pants such that lifting a global option makes partial sense.  It certainly would be odd to let them live outside the repo and yet accept a patch for their global option with no code using it in the repo!
    3. I can put a point on Benjy's preference: If I use pants in a python only repo or only for python (Aurora) I would be confused and offput by seeing --[no-]use-nailguns in my help.

      If the subtask did not 'require' the option, it would not show up.

      Further, at least today, stuffing something in global_options is a privilege only tasks authored in the pantsbuild/pants repo can leverage today.

      Sure: but only convention is enforcing the fact that things like nailgun usage and logging are "effectively global", and convention is fragile. If a plugin had a sufficiently useful flag, it would be good to get it upstreamed so that it could be used across multiple Tasks.

    4. "If the subtask did not 'require' the option, it would not show up." <- try it - this is not true.
      ```console
      $ git diff
      diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py
      index dfb4a11..53eb052 100644
      --- a/src/python/pants/option/global_options.py
      +++ b/src/python/pants/option/global_options.py
      @@ -23,6 +23,7 @@ def register_global_options(register):
         Global options here on the other hand are reserved for infrastructure objects (not tasks) that
         have leaf configuration data.
         """
      +  register('--rvm', help='You probably don;t care about ruby')
         register('-t', '--timeout', type=int, metavar='<seconds>',
                  help='Number of seconds to wait for http connections.')
         register('-x', '--time', action='store_true',
      $ pants.dev help | grep "\--rvm"
      *** Running pants in dev mode from /home/jsirois/dev/3rdparty/pants/src/python/pants/bin/pants_exe.py ***
      INFO] Detected git repository at /home/jsirois/dev/3rdparty/pants on branch master
      --rvm _DEFAULT_RVM__    You probably don;t care about ruby (default: None)
      ```
      
      And on the latter I don't agree.  If a flag is useful it should be encapsulated by a useful component that uses the flag - no one else.  That component - class - can then be re-used by others.  Setting the value for all users of the component at once is a different problem, one that is solved for config but not env var or CLI.
    5. try it - this is not true.

      Right: my suggestion was for a new feature in options that would help to remove duplication... I was suggesting how that feature would work.

      And on the latter I don't agree. If a flag is useful it should be encapsulated by a useful component that uses the flag - no one else. That component - class - can then be re-used by others. Setting the value for all users of the component at once is a different problem, one that is solved for config but not env var or CLI.

      Ok, I buy that. Thanks! Does that mean that all of everyone should be extending 'LoggingTaskMixin' to get their logging flag registered?

    6. Focusing on your example, I think not - logging is in fact a valid global option since the very core of pants uses it - GoalRunner - outside tasks, targets and BUILD file aliased objects.  Maybe I don't understand the example.
    7. It's been suggested a few times that logging should be either global or task specific, in that I might want to enable per-task logging... that's what I was referring to with LoggingTaskMixin.

    8. OK. Logging already works this way today - almost. Since its a global flag - aka needed to even boot pants - it can be set globally and then over-ridden for a given task, say via a flag as in ./pants gen.scrooge -ldebug compile -lwarn examples/src:: This would mean logging is globally the default of info, but ScroogeGen sees debug and Compile{Apt,Java,Scala} see warn. The only thing missing is for tasks to use this local log level to configure their logger. Almost all loggers today just inherit the root logger's level - in this example info. So 2 steps if you want to work on this:
      1. Don't use the context.log or or else fix the context.log logger to vary by caller
      2. Configure the task logger to log at the tasks logging level, ie, in prepare do something like:

           @classmethod
           def prepare(options, round_manager):
             logger.setLevel(options.level.upper())
      

      Clearly 2 is an abstraction leak - the task should not know it needs to ucase the level name. For that matter it should not know about the flag name and should probably ask a utility in logging/setup.py to configure its logger given the local options and the local logger. And maybe this was what you mean by LoggingTaskMixin, a mixin that both establishes a class variable logger and does this local config in prepare.

  3. 
      
JS
ZU
  1. Ship It!
  2. 
      
JS
  1. Thanks folks - submitted @ https://github.com/pantsbuild/pants/commit/fee7c8af2355618814e931dd3f2c3dc3bd67cc19
  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...