Get rid of some direct uses of config in jvm_compile.

Review Request #1357 — Created Nov. 18, 2014 and submitted

benjyw
pants
fe9b856...
pants-reviews
patricklaw, zundel

These are now replaced with registered options.

Got rid of the javac_args config key, as it was basically the same as
--args, except that pants added the -C escapes for you. This was confusing and
inconsistent anyway.

Created --source and --target options, and removed a random reference
to the old way of specifying those from test_rcfile.py, to avoid confusion
(even though the test still passed).

All relevant unit and integration tests pass.

CI baking.

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
ZU
  1. 
      
  2. pants.ini (Diff revision 1)
     
     

    I don't think we need these in pants.ini if the default is already '6' in the code, but maybe we want to make the default 'None' in the code (see below)

    1. True, I guess I wanted to leave it there for illustrative purposes, but I will leave it but make the default be None/unspecified, as you suggest below.

      I plan to write a script to clean up pants.ini entries that are identical to the defaults, so we can all run it in our respective repos.

  3. pants.ini (Diff revision 1)
     
     

    this is a breaking change. If I upgrade pants I'm going to get

    -source 6 -target 6 -source 8 -target 8

    on my javac command line and pandemonium will ensue. Also, some poor soul might do the same trying to configure pants in the future. Maybe we should warn or error if javac_args contains the -target or -source flag?

    1. Most of the config->new options migration changes will be breaking. In general I prefer to use migrate_config.py as the solution, rather than peppering the code with checks for legacy options. For example, in this case you'd need to rename 'javac_args' to 'args' and add -C prefixes to the args in order to get the javac command line you mention. And migrate_config.py specifically tells you what to do to migrate this config key.

      On the other hand this is a rather special case, as it's possible to naively break things even in the steady state. So I've added a check.

  4. Yep, that is exactly what happened and it is broken today.

    DEBUG] Executing via NailgunClient(host=u'localhost', port=53718, workdir='/Users/zundel/Src/pants'): /Library/Java/JavaVirtualMachines/jdk1.8.0_25.jdk/Contents/Home/bin/java -Xmx2G -cp /Users/zundel/.ivy2/cache/com.sun.tools/jmake/jars/jmake-1.3.8-3.jar com.sun.tools.jmake.Main - 
    ...  
    -C-Tnowarnprefixes -C%(pants_workdir)s/gen 
    ... examples/src/java/com/pants/examples/hello/greet/Greeting.java examples/src/java/com/pants/examples/hello/main/HelloMain.java
    

    I don't think interpolation is necessary, I think we can just substitute it right into this string and fix it.

    1. The fix assumes that we always use the default pants_workdir, which is currently true (no code overrides it anywhere). In the future this will go through the regular new options system, and this will no longer be the case.

  5. type=int ?

    You aren't required to set --source or --target at all. Maybe we should default to None and then not pass the flag down to javac if it isn't specified.

    1. Done the default=None bit. But this isn't an int. E.g., '1.6' is a valid value, '6' is an alias for it. In general I tend to believe that ints are for things that you do arithmetic on, or that need to be sorted as integers. These are just tokens, so they're better represented as strings, even if they happen to correspond to integers in a colloquial way.

  6. (Maybe this is a good reason to always specify the target level)

    1. I think it's OK not to require it, if you know what you're doing. You may want the default javac behavior? However you definitely must specify it if you have multiple java versions in a single repo.

  7. nit: shouldn't there be a blank n/l between system imports and our project imports?

  8. 
      
BE
ZU
  1. Ship It!

  2. 
      
PA
  1. Ship It!

  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 82e549d1b9263c3f170533613b1bc0ce545541e2.
Loading...