Refactor AndroidDistribution to remove SDK checks in __init__

Review Request #676 — Created July 14, 2014 and submitted

mateor
pants
update-dist-up
672
pants-reviews
jsirois
This review is dependent on 672. I kept this
separate to make reviewing (moderately) palatable.

This is a refactor of AndroidDistribution. Instead of validating sdk_paths at instantiation, we
are now only validating SDKs when a tool is requested by a path. This makes better sense, in
that we would prefer to know about the tools available to us instead of the dirs that
traditionally contain them. It also moves action out of _init__. The AndroidTask instantiates
an AndroidDistribution but with unverified variables until the tools are needed.

I also added a command-line flag allowing users to specify a path of an Android SDK if they want.
This is an improvement over the earlier model that only cycled through common environmental aliases.

Users are likely to have just one SDK installed, with the issue being what tools are installed. This
checks the tools when the tasks that use them are invoked. If the user
has a custom SDK, they can now pass that at invocation.

This allows CI and Travis to pass (although Travis is failing on an unrelated issue currently, see below).

test_android_distribution was also refactored.
CI.sh passes locally.
Travis passes  (with 672 patched in as well) [0].

[0] https://travis-ci.org/pantsbuild/pants/builds/30097096
JS
  1. 
      
  2. (path,) is a tuple, (path) is just exactly path.
    1. Whoops, remnant of earlier key.
  3. By signature path is never None here so the `if path:` check looks a bit odd.  I think you need to:
    1.) pydoc the cached classmethod and explain :param string path:...
    2.) doc set_sdk_path as well and probably also default path=None.
  4. Neither target_sdk nor build_tools_version is used now.
    1. I am caching tools in _validated_tools. So each android.jar and aapt is being cached for quick lookup later. I call them 'tools' because are executables and some are not. Am I responding to what you meant?
    2. Nope, I mean - for this constructor signature:
      def __init__(self, sdk_path=None, target_sdk=None, build_tools_version=None):
      
      The target_sdk and build_tools_version paramters are no longer used at all.  They could be killed.
  5. All user-facing methods need self._sdk_path to be non None.  That suggests you can move these checks to the constructor and fail faster.
    1. Yeah, this is ugly. I am still thinking on it. My original version of AndroidDistribution did check for None in the constructor. But the reason for the refactor is that when I force a class to resolve the sdk_path in the constructor, then pants fails on any machine without a valid Android SDK. When pants is run, the AndroidDistribution.Error is raised because the class is instantiated even if it is not an android target. My solution was to allow sdk_path to be None all the way until a tool was used. Do you see a way around that?
      
  6. 
      
MA
JS
  1. Small stuff.
  2. This actually doesn't set anything, it just gets an AndroidDistribution fresh each time its called.  A rename is in order to avoid confusion.
  3. Odd indents in this except block - -2 this line and even after that I think the next line will need maybe a +2 tweak.
  4. 
      
MA
JS
  1. In master as 1/2 of this commit: https://github.com/pantsbuild/pants/commit/333044596751dc85d2e52ae36c1bd1c04f540e47
    Please mark this review as submitted.
  2. 
      
JS
  1. Ship It!
  2. 
      
MA
Review request changed

Status: Closed (submitted)

Loading...