Hacky fix for issue with inner classes in .proto files with 'java_mulitple_files' enabled

Review Request #303 — Created May 1, 2014 and submitted

zundel
pants
zundel/diagnose-missing-protobuf-gen
93
pants-reviews
jsirois
commit f9c2474915eeb4ae46529abf276de5deea591595
Author: Eric Ayers <zundel@squareup.com>
Date:   Thu May 1 08:33:26 2014 -0400

    Hacky fix for issue with inner classes in .proto files with 'java_mulitple_files' enabled
    https://github.com/pantsbuild/pants/pull/93
Updated test_cache_manager.py. Also fixes the issue with inner object definitions being expected to generate .java files from protoc in our repo.
ZU
  1. OK, There is obviously a problem here.  I am not sure how to tell rbt that I only have one commit in my diff.
  2. 
      
ZU
ZU
JS
  1. 
      
  2. src/python/pants/tasks/cache_manager.py (Diff revision 2)
     
     
    Can all this go or else just be wrapped in a cache-specific Exception type with extra data and re-raised?  The context is a "big" thing to toss in here.
    
    If you want to keep and do agree on wrapping, the undocumented as of yet prefeered style is inner exception types and this combines with the global pep8 except 2 space indents, 100 cols max... so:
    
    class CacheManager(object):
      class AccessError(Exception):
        """Inidicates a problem accessing the cache."
      
      ...
      def _key_for(self, target, dependency_keys):
        try:
          ...
        except IOError as e:
          raise self.AccessError('Problem calculating a key for %s: %s' % (target, e))
    1. I feel pretty strongly we should have a better diagnostic message.  When this code fails, the error you get is too cryptic to even begin to track it down:
      
      can't find file .pants.d/gen/.../Type.java
      
      (I have 1800 build files in the repo, please don't make me guess where the problem is!)
      
      I'll refactor it to raise an exception as you suggested (and fixup the formatting)
    2.   This is what the updated exception will look like:
      
        ...
        File "/usr/local/Cellar/python/2.7.6/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__
          return self.gen.next()
        File "/Users/zundel/Src/pants/src/python/pants/tasks/task.py", line 201, in invalidated
          invalidation_check = cache_manager.check(targets, partition_size_hint)
        File "/Users/zundel/Src/pants/src/python/pants/tasks/cache_manager.py", line 202, in check
          all_vts = self._sort_and_validate_targets(targets)
        File "/Users/zundel/Src/pants/src/python/pants/tasks/cache_manager.py", line 258, in _sort_and_validate_targets
          cache_key = self._key_for(target, dependency_keys)
        File "/Users/zundel/Src/pants/src/python/pants/tasks/cache_manager.py", line 329, in _key_for
          % (e.filename, target.id, e))pants.tasks.cache_manager.CacheValidationError: Problem validating file /Users/zundel/Development/java/.pants.d/gen/protoc/gen-java/com/squareup/protos/multipass/service/Type.java for target multipass.multipass-protos.src.main.proto.proto.protobuf_gen: [Errno 2] No such file or directory: u'/Users/zundel/Development/java/.pants.d/gen/protoc/gen-java/com/squareup/protos/multipass/service/Type.java'
  3. src/python/pants/tasks/protobuf_gen.py (Diff revision 2)
     
     
    Can this go now?
  4. A handy pattern to give some more visibility to this comes from pytest, see xfail: http://pytest.org/latest/skipping.html#skipping
    
    Then we'll see ......X.....
    
    All thats needed is breaking this 1 test method into 2 or more.
  5. 
      
JS
  1. 
      
  2. src/python/pants/tasks/protobuf_gen.py (Diff revision 2)
     
     
    I filed https://github.com/pantsbuild/pants/issues/96 to track a real parse - that would be better to point to here.
  3. 
      
ZU
JS
  1. lgtm - s nits and I'll patch this in.
    
    Thanks again.
  2. src/python/pants/tasks/cache_manager.py (Diff revisions 2 - 3)
     
     
    This indent is a bit odd - typically hanging align:
    raise self.CacheValidationErrorError("Problem validating file %s for target %s: %s"
                                         % (e.filename, target.id, e)))
    
    Or else line continuation indent (+2 or +4):
    raise self.CacheValidationErrorError(
        "Problem validating file %s for target %s: %s" % (e.filename, target.id, e)))
  3. Yeah - string concat instead please to keep this under the 100 column limit.
  4. 
      
ZU
JS
  1. Ship It!
  2. 
      
JS
  1. In @ https://github.com/pantsbuild/pants/commit/59f4c8d706884309f2681e39955cc1ce80a5962b - please mark this review submitted.
  2. 
      
ZU
Review request changed

Status: Closed (submitted)

Loading...