Add AndroidTask and AaptGen task

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

mateor
pants
update-aapt-up
676
pants-reviews
jsirois
This adds an AndroidTask base task and an AaptGen codegen task using the aapt tool.
The AndroidTask merely obtains an AndroidDistribution for subclasses to work with.

The AaptTask processes resource folders and outputs an R.java class. This will be
compiled along with the regular .java files in the android_compile phase coming up. The
aapt tool itself packs an enormous amount of action and this class will continue to 
grow as Android support grows. I could see an AaptMixin or AaptBase class in the future.

The aapt_gen class allows a command-line override of the target_sdk and build-tools versions
set by the manifest parser in the AndroidTarget class.

Currently the SyntheticTarget output by the aaptGen class is a JavaLibrary. This will likely be
changed as I create a task and target to compile .java files for Android builds next.
CI.sh passes locally.

Travis is currently failing elsewhere, but I have
manually tested all of the setup_parser flags, including the new
ignore-assets flag.
  • 1
  • 0
  • 6
  • 3
  • 10
Description From Last Updated
order these IT ity
MA
JS
  1. 
      
  2. This can be killed now - support for omitting unused entrypoints is in on master.
  3. I'm not a big fan of "dependency bags" like this.  I think it makes sense to inline the direct deps to the "real" targets that use them even if it is not DRY.
  4. inline all this gen_langs stuff, you can just say:
    
    def is_forced(self, lang):
      return lang == 'java'
  5. This does more than deal with BUILD - it would be good to at least TODO a default list with configurability.  This is expansive and limited at the same time - for example: What about mercurial? and picasa.ini is old enough to probably be generally not present.
    1. This is the default ignore-assets that is hard-coded into aapt source code but with BUILD appended. I have to add the entire string because any --ignored-assets passed to aapt clobbers the default. So for right now, this is just the built-in ignores + BUILD. But it will need to be configurable somewhere down the line, a TODO will be added.
  6. kill extra blank line
  7. s/,/, /
  8. s/ = /=/
  9. I'm pretty sure this will get - for example - jarred up wrong, not that anyone would jar R.java source or the compiled form.  However, the key bit of the add_new_target API is that the address.spec_path becomes the source root and the sources paths are taken relative to that.  So, for example if I have:
    .pants.d/aapt_gen/com/pantsbuild/example/res/R.java
    
    Where:
    self.workdir: .pants.d/aapt_gen
    package: com/pantsbuild/example/res
    file: R.java
    
    And you want things like jarring to jar up:
    com/pantsbuild/example/res/R.java
    
    Then you need an address with spec_path: .pants.d/aapt_gen/
    And sources = com/pantsbuild/example/res/R.java
    
    IE: the target SourceRoot is .pants.d/aapt_gen in this example.
    The docs here are out of date, but the code tells the story: https://github.com/pantsbuild/pants/blob/master/src/python/pants/goal/context.py#L216
    
    1. Thanks for the breakdown, this stuff really helps. I will be read the context code.
  10. Is R.java truly dependency free?  If so - great.  If not, injecting deps on whatever runtime library is associated with the target_sdk would be a good follow-up RB.
    1. I will need to add the target_sdks android.jar to the java compile stage that comes next. This Synthetic target is kind of a skeleton because I haven't figured out what to do next yet. I am sure this will need to be adjusted, I will look at it for the upcoming iteration of this patch.
  11. Typically user-facing products go to dist/ in pants and non-user facing outputs go to self.workdir (under .pants.d).  I would like to support android paradigms where they exist, but pants has opinions here and placing outputs in source trees is not one of them.
  12. Stick to pep257/rst conventions here, summary/blank line/paragraphs/blank line/params/returns/throws/:
    """Summary sentence one line.
    
    :param string build_tools_version: the appt tool version to fetch (e.g. "19.1.0").
    :returns: the path to the aapt tool binary.
    """
  13. Prefer lazy access for heavyweight ops - we're shooting for light weight constructors although there are certainly bad Task examples out there.
    
    Consider using a @property.
    1. Thanks, I will do that.
  14. Kill these extra blank lines
  15. odd indent, perhaps:
        self.assertEqual(expected_file, AaptGen._calculate_genfile(os.path.join(rel_path, 'bin'),
                                                                   package))
  16. 
      
MA
MA
IT
  1. 
      
    1. Thank you for taking the time to review the patch. I will begin on the changes from your comments. A couple things I wanted to confirm below.
    2. No problem!
    1. Do you mean order the __future__'s? This looks like the standard pants template to me. Could you show me what you mean?
    2. I meant the imports themselves. (absolute_import,division ... and so on
    3. I only see that ordering being done on one other file, tests/python/pants_test/authentication/test_netrc_util.py. All the other pants files use the template in this patch (which is where I borrowed it from). I may wait for John to chime in on this since I know they have worked really hard to have a common style. But thanks for your other suggestions, I updated this RB with your feedback incorporated.
    4. I agree with Ity that future imports should be fixed, but this is a larger problem and deserves a dedicated houscleaning RB that only fixes that one issue across all files.  Some non-sorted ones got in and then got copy-pastaed around.
  2. since you are doing the prefix with _ for private method - lets maintain the format for vars
  3. this could be a @staticmethod
    1. These methods you recommend to change to static are all overriden from code_gen.py, for which they are not static. Nor are they for the other codegen subclasses. I can make them @staticmethod, but they would be alone in that if I do. Could you confirm you recommend it here?
    2. in that case, you definitely do not want to do that. these are methods that subclasses should override - a staticmethod would not work in this respect.
  4. how about a one-liner comment on what these flags mean?
  5. see detect_duplicates: excludes for an easy way to do this. it would be good to have these not confined to method level scope at least.
    1. Good idea, I will do that. Thanks for pointing to the example!
  6. ordering of @classmethod - before anything else in the class
  7. other gen targets tend to use this as well (eg protobuf, jaxb) - would be good to extract this out. 
  8. strange indentation here onwards.
  9. 
      
MA
JS
  1. Notes for follow-up, I'm going to patch this in as-is.
  2. Benjy's config/args proposal/refactor will fix this, but for now you need to be careful about dest since the desyt names are global to the Options object shared across all Tasks active for a run.  The mkflag(..) only handles flag name namespacing.  So I think these are conflict free now, but a good idiom to circle back and fixup these with prior to Benjy's change landing is prefixing dests, ie:
    s/dest="target_sdk"/dest="appt_gen_target_sdk"/
  3. The test above does:
        with self.assertRaises(AssertionError):
          self.assert_files(
    
    Thats a little strange but understandable - self.assert_files does the opposite of what you want.
    
    Here though you use unittest2 and have self.assertNotEqual[1] which is more direct.
    
    [1] https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertNotEqual
  4. 
      
JS
  1. Actually - this needs some armor around the tests:
    $ PANTS_DEV=1 ./pants goal test tests/python/pants_test/:all
    *** Running pants in dev mode from /home/jsirois/dev/3rdparty/pants/src/python/pants/bin/pants_exe.py ***
    
    15:12:29 00:00 [main]
                   See a report at: http://localhost:34802/run/pants_run_2014_07_22_15_12_29_751
    15:12:29 00:00   [bootstrap]
    15:12:29 00:00   [setup]
    15:12:29 00:00     [parse]
                   SUCCESS
    Traceback (most recent call last):
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/bin/pants_exe.py", line 186, in <module>
        main()
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/bin/pants_exe.py", line 180, in main
        _run()
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/bin/pants_exe.py", line 161, in _run
        result = command.run(lock)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/commands/goal.py", line 260, in run
        return engine.execute(context, self.phases)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/engine/engine.py", line 50, in execute
        self.attempt(context, phases)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/engine/round_engine.py", line 170, in attempt
        phase_executors = list(self._prepare(context, phases))
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/engine/round_engine.py", line 164, in _prepare
        self._visit_phase(phase, context, phase_info_by_phase)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/engine/round_engine.py", line 156, in _visit_phase
        self._visit_phase(phase_dependency, context, phase_info_by_phase)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/engine/round_engine.py", line 156, in _visit_phase
        self._visit_phase(phase_dependency, context, phase_info_by_phase)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/engine/round_engine.py", line 120, in _visit_phase
        task = task_type(context, task_workdir)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/backend/android/tasks/aapt_gen.py", line 62, in __init__
        self._android_dist = self.android_sdk
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/backend/android/tasks/android_task.py", line 26, in android_sdk
        self._android_sdk = AndroidDistribution.cached(self.forced_sdk)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/backend/android/distribution/android_distribution.py", line 47, in cached
        dist = cls.locate(target_sdk=target_sdk, build_tools_version=build_tools_version)
      File "/home/jsirois/dev/3rdparty/pants/src/python/pants/backend/android/distribution/android_distribution.py", line 68, in locate
        'set ANDROID_HOME in your path' % ('Android SDK'))
    pants.backend.android.distribution.android_distribution.Error: Failed to locate Android SDK. Please set install SDK and set ANDROID_HOME in your path
    
    An example of conditionally executing can be found here where the scaladoc binary is checked for:
      https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/tasks/test_jar_publish_integration.py#L26
    
    The key is @pytest.mark.skipif and then a condition that can check for the absence of the SDK without raising.
    1. I can't seem to replicate this. My way of mocking a machine w/o an SDK is to change the envvars we check for in AndroidDistribution (e.g. ANDROID_HOME to FAKE). I don't have any tests that should require a live SDK specifically to avoid this issue. 
      
      This review is dependent on 676 to pass ci, are both patched in on this run?
    2. Aha - I missed/forgot this dependency.  It would be nice though to submit this change and not blow up ci until 676 gets in.
    3. Hrm, ok - I can patch both in at once - but going forward this defeats the point of small review to some extent.  The RBs should be atomically comittable and green to be useful small chunks.  If that can't happen, bigger and green is better.
  2. 
      
JS
  1. Note to self or eventual patcher - this RB needs manual __init__.py intervention:
    $ git slog -1 | grep __init__
     src/python/pants/backend/android/tasks/__init__.py     |   0
     tests/python/pants_test/android/__init__.py            |   0
     tests/python/pants_test/android/tasks/__init__.py       |   0
  2. 
      
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. 
      
MA
Review request changed

Status: Closed (submitted)

Loading...