Fix ivy resolve args + added ability to provide custom ivy configurations

Review Request #1671 — Created Jan. 27, 2015 and submitted

fkorotkov
pants
179ef3f...
pants-reviews
benjyw, ity, tejal, zundel

Fixed resolve-ivy-args flag.

Added resolve-ivy-confs flag to provide custom configurations to ivy. This flag will be used by IJ plugins to get sources and javadocs for libraries.

Tests are passing https://travis-ci.org/fkorotkov/pants/builds/48516401

  • 0
  • 0
  • 6
  • 1
  • 7
Description From Last Updated
FK
BE
  1. 
      
  2. Huh, so these were just ignored?

  3. Can conf names have whitespace or quotes in them? I don't think you need safe_shlex_split here. the confs option is already of list type so you can just assign

    self._confs = self.get_options().confs

    1. I did it because when I ran ./pants resolve --resolve-ivy-confs="default sources javadoc" it was passed as ["default sources javadoc"].

    2. Yeah, the argparse-y way to do that is --resolve-ivy-confs=default --resolve-ivy-confs=sources --resolve-ivy-confs=javadoc

      I can see maybe supporting comma-separated lists, that seems more intuitive for this particular flag?

    3. When I tryid to check if it's working I user --resolve-ivy-confs="default sources javadoc". This patch handles both cases. I looked it up in jvm_task.py how it handles --args.

    4. I agree with Benjy. We should stick to argparse-y way.

      Can you also change the desricption. It is not clear what to pass.
      register('--confs', action='append',
      help='''Pass configurations to ivy instead of default ones:
      You can specify multiple configurations by using the same flag.
      '''
      )

    5. how about:

      Pass a configuration to ivy in addition to the default ones. Specify multiple configurations by repeating the flag with each additional configuration.
      

      "Pass configurations" seems like it might mean --confs="conf1 conf2" would work. It should say that it only accepts single confs.

  4. The --confs option already defaults to config (key confs in section resolve.ivy) from a different section if not specified on the cmd-line, so this part is now superfluous. But be sure to modify migrate_config.py appropriately, to register the change in section name.

  5. This part will still need to happen, regardless of the current value of self._confs, of course.

    1. The only way that self._confs should get set is by running through this logic. _confs() is the caching of this calculation.

    2. Well, now it's that plus the value of --confs, no?
    3. Well, it should be, but with this patch in the state it is in, now they are mutually exclusive.

  6. 
      
ZU
  1. 
      
  2. So, Patrick had me change this line the last time I touched this file to an explicit comparison to None because in the case that an empty list is computed, the caching of this calculation in self._confs is undermined.

    1. Yes, another reason why the idiomatic way to do this is to keep the memo variable as a class attribute right above the method definition. It makes it clear that you're checking a sentinel value for caching. In this case what we want is a utility cached_property decorator, I've been meaning to toss one into the repo for a long time now. It's an incredibly common pattern.

  3. 
      
ZU
  1. 
      
  2. You didn't say exactly what the purpose of this flag is. Is it to add on 'sources' and 'javadocs'? If so, you can just have your task require them. See prepare() in ide_gen.py.

    round_manager.require('jar_map_default')
    round_manager.require('jar_map_sources')
    round_manager.require('jar_map_javadoc')
    

    Unfortunately, by settin self._confs here in __init__, What you've done here is short circuit the logic in the confs() property below. You can't use --confs and using a product at the same time.

    If you still need to manually specify --confs on the command line, try setting the argument aside, then inside of the confs() property below, add them in to self._confs there.

    1. Sorry. Added to description.

  3. 
      
FK
FK
FK
ZU
  1. 
      
  2. We will never make it past this line if self._confs is not empty after running __init__

    I know this is proably more work than you had in mind, but I suggest that you take this opportunity to fix ide generation as was requested in https://github.com/pantsbuild/pants/issues/935.

    You can keep going with this change, but in fact, you don't really need any changes to the ivy task to support adding in sources and javadoc.

    1) Copy depmap.py and make a new task ide_config or something like that.

    2) In your new goal, fix up the prepare() statement like this:

    @classmethod
    def prepare(cls, options, round_manager):
    super(IdeConfig, cls).prepare(options, round_manager)
    round_manager.require('jar_map_default')
    round_manager.require('jar_map_sources')
    round_manager.require('jar_map_javadoc')

    3) Now no changes are needed to ivy_resolve and we can separate the features needed for the IDE vs. human readable output from the depmap goal.

    1. We will eventually switch to a separate task. But for now I'm fixing --args and it appeared I can't specify --conf with resolve-ivy-args because it will duplicate default --confs or from pants.ini that's why I addede a separate flag.

  3. 
      
NH
  1. 
      
  2. should the confs be quoted? I think

    ./pants resolve --resolve-ivy-confs=default sources javadoc 3rdparty:junit
    

    would fail.

  3. 
      
FK
BE
  1. 
      
  2. This logic is now wrong. You don't need to read from the config file explicitly, as the --confs option will already do that. 
    
    Also, if I understand correctly you want to always add these required confs to self._confs. 
    
    So I think the right logic is to always set self._confs to None in __init__(), and then populated it lazily here, with the union of self.get_options().confs and the required confs. 
    
    Does that make sense?
    1. My bad. Fixed and added some tests :-)

  3. 
      
FK
BE
  1. Looking pretty good now! just a couple of comments left.

  2. There's no need for this second sentence in the help message, as this is true for all action='append' flags, and so should already be expressed generically in the generated help text (or if it isn't, we can make that happen in a separate change).

  3. You can just make ['default'] be the default in the option registration:

    `register('--confs', action='append', default=['default'], help=...)

    Also, I would make a copy of the list you get back from self.get_options().confs, since you're going to mutate it later.

    1. Will add the default value.

      I create a set from the list to add additional configurations from required products. So I don't mutate self.get_options().confs.

  4. 
      
FK
BE
  1. Ah right, I missed that you were already copying into a set. Looks good! Thanks for the change.

  2. 
      
TE
  1. Ship It!
  2. 
      
ZU
  1. Ship It!
  2. 
      
FK
Review request changed

Status: Closed (submitted)

Loading...