Migrate all JvmTasks to use the new options system.

Review Request #1186 — Created Oct. 20, 2014 and submitted

benjyw
pants
e2226aa...
pants-reviews
johanoskarsson, jsirois, zundel

The common --jvm-options, --args, --debug and --confs flags now
live in the base class, but this required migrating all the
relevant tasks at once, hence the scope of this change.

This also gets rid of manual flag-to-config fallback, as the new
options system does this automatically. However this required some
changes to section and key names in pants.ini. I'm documenting
these changes in a spreadsheet, for smooth config migration in
pants install bases.

This required breaking a couple of inconsistently named flags, e.g.,
--benchmark-caliper-args is now just --benchmark-args.

CI passed.

Manually tested that new flags and config settings work.

  • 1
  • 0
  • 0
  • 1
  • 2
Description From Last Updated
should this be [run.junit] (I don't see under --help) or [test.junit]? JI jinfeng
IT
  1. lgtm, thank for doing these.

  2. formatting: > 100

  3. formatting: multiple newlines

  4. 
      
ZU
  1. 
      
  2. I was in this code the other day and I agree with Benjy. I was unable to use an instance method on a superclass to implement a refactoring because of the way coverage "tasks" are implemented here.

  3. I think this reverts a change I made to self.register_jvm_tool() that landed last week. the advantage of passing ini_section and ini_key to register_jvm_tool is that it puts out extra information in an exception when the dependency can't be found.

  4. FYI, this reverts adding **kwargs I made last week. (Its not currently used for anything though)

  5. nit: use .format()

  6. again, reverts **kwargs change

  7. It looks like you managed to expunge the other references to JvmDebugConfig from the code. Is this the only reference left? Maybe it would be good to just lift the code and put it into this file (which is what I wanted to do in the first place.)

  8. thanks for fixing that.

  9. 
      
BE
ZU
  1. Ship It!

  2. 
      
ZU
  1. 
      
  2. pants.ini (Diff revision 2)
     
     

    One thing I just thought about this change - its going to silently cause my repo to break because I have modified jvm_args to set system properties all over the place like:

    jvm_args: ['-Duser.timezone=UTC', '-Xmx2G']
    

    I would appreciate it if we could add a warning to check the pants.ini config for the old names, and if they exist, spit out a warning indicating that these values are ignored and they need to be updated to the new value.

    1. Hmm, I thought of doing that, but I'm not really sure where in the code that would go, or which keys to check for. It seems very special-case? Let me try and cook something up in a separate change.

      I didn't worry too much about pants.ini compatibility because there are only ~3 pants.ini files in the wild, and this spreadsheet tracks the necessary changes:

      https://docs.google.com/spreadsheets/d/1WGSJwXMioD6WqvXyL72HQTC8bzy4hU6-Uy2bs62fedk/edit?usp=sharing (see the second tab).

      It takes about a minute to effect the changes manually, so I also didn't bother with a migration script.

    2. think about it. no need to block this change.

  3. 
      
BE
Review request changed

Status: Closed (submitted)

JI
  1. 
      
  2. pants.ini (Diff revision 2)
     
     

    benjyw, is this one the default value for jvm_args, or the line you removed was the default value?

    jvm_args: ['-Xmx1g', '-XX:MaxPermSize=256m']

    if this line is, should the new jvm_options be changed to 2g?

    1. Not sure I understand the question. Can you clarify?

    2. never mind I read it wrong.

  3. 
      
JI
  1. 
      
  2. pants.ini (Diff revision 2)
     
     

    should this be [run.junit] (I don't see under --help) or [test.junit]?

    1. oops, this should be test.junit, good catch.

    2. i'll send a patch.

  3. 
      
BE
  1. 
      
  2. 
      
Loading...