Refactor exceptions in build_file.py and build_file_parser.py to derive from a common baseclass and eliminate throwing IOError. Catch errors from parse errors in the BUILD file and format context around the exception.

Review Request #954 — Created Aug. 28, 2014 and submitted

zundel
pants
zundel/refactor-build-file-exceptions
537
5530f74...
pants-reviews
jsirois, patricklaw

Followon to https://rbcommons.com/s/twitter/r/929/

Refactor exceptions in build_file.py and build_file_parser.py to derive from a common baseclass and eliminate throwing IOError.

Revive error message that prints out alternatives to an incorrect build target.

Also, Catch exceptions from parse errors in the BUILD file and print some formatted context around the line of the file that failed.

Exception message: Error parsing /Users/zundel/Src/Pants/examples/src/java/com/pants/examples/hello/greet/BUILD:
4: java_library(name = 'greet',
5: dependencies = [], # A more realistic example would depend on other libs,
6: # but this "hello world" is pretty simple.
7: sources = globs('.java' && foo),
^ SyntaxError: invalid syntax (<string>, line 7)

8:   provides = artifact(org='com.pants.examples',
9:                     name='hello-greet',

10: repo='build-support/ivy:public',),
11: )

ci passed at https://travis-ci.org/pantsbuild/pants/builds/33756154

  • 0
  • 0
  • 6
  • 0
  • 6
Description From Last Updated
PA
  1. 
      
  2. I'm amazed this used to be a dep!

  3. Is this ever actually used anywhere? "target" is another word that shouldn't occur in BuildFile(Parser)

    1. I went through and substituted 'address' for 'target' and 'spec' in the symbols and error messages in this file.

  4. Interesting--this should probably be AddressableConflictException, though I'm not super worried about it. However, this seems like the right CR for rearranging these types of exceptions.

  5. 
      
ZU
PA
  1. lgtm minus the one nit/question about printing context

  2. Not that it's for this CR, but all of these Target.get* calls are super old and certainly broken.

    1. I updated the TODO

  3. I think this is debatable, as they're two different things, but I suspect that this is my core-dev bias showing. sgtm for now.

    1. My logic: Typewise they are different and I think it makes sense to differentiate between the two in symbols used in the code. But from a user standpoint, a 'spec' is either an address or a glob used to specify an address (address selector). The globbing stuff is taken care of cmd_line_spec_parser.py, so these strings should all describe addresses.

  4. I think it's great to show the context here, but is there a reason you can't just use https://docs.python.org/2/library/traceback.html#traceback.print_exception ?

    If so that's fine, but please hoist this exception printing code into its own method, as this is already a very meaty method.

    1. Ok on refactoring into a helper method.

      My thoughts on traceback.print_exception() are that it is convenient to the Pants developer, but the output we are getting is almost illegible to end users and looks like a bug in Pants. Here is the outfile of a BUILD file parsing error with the existing error reporting:

      ERROR:pants.base.build_file_parser:Error parsing /Users/zundel/Src/Pants/examples/src/java/com/pants/examples/hello/greet/BUILD.
      Traceback (most recent call last):
        File "/Users/zundel/Src/pants/src/python/pants/base/build_file_parser.py", line 143, in parse_build_file
          build_file_code = build_file.code()
        File "/Users/zundel/Src/pants/src/python/pants/base/build_file.py", line 199, in code
          code = compile(source.read(), '<string>', 'exec', flags=0, dont_inherit=True)
        File "<string>", line 7
          sources = globs('*.java' && foo),
                                    ^
      SyntaxError: invalid syntax
      Traceback (most recent call last):
        File "/Users/zundel/Src/pants/src/python/pants/base/build_file_parser.py", line 143, in parse_build_file
          build_file_code = build_file.code()
        File "/Users/zundel/Src/pants/src/python/pants/base/build_file.py", line 199, in code
          code = compile(source.read(), '<string>', 'exec', flags=0, dont_inherit=True)
        File "<string>", line 7
          sources = globs('*.java' && foo),
                                    ^
      SyntaxError: invalid syntax
      
      Exception message: invalid syntax (<string>, line 7)
      

      Here is the updated error message after adding in the context and eliminating the duplicate logging:

      Exception message: Error parsing /Users/zundel/Src/Pants/examples/src/java/com/pants/examples/hello/greet/BUILD:
          4: java_library(name = 'greet',
          5:   dependencies = [], # A more realistic example would depend on other libs,
          6:                      # but this "hello world" is pretty simple.
      *   7:   sources = globs('*.java' && foo),
                                         ^ SyntaxError: invalid syntax (<string>, line 7)
      
          8:   provides = artifact(org='com.pants.examples',
          9:                     name='hello-greet',
         10:                     repo='build-support/ivy:public',),
         11: )
      
       Loading addresses from 'examples/src/java/com/pants/examples/hello/greet' failed.
        referenced from examples/src/java/com/pants/examples/hello/main:main-bin
        referenced from examples/src/java/com/pants/examples/hello/main:main
      

      Is there any additional information from traceback.print_exception() that is useful to end users?

    2. Nope, I'm convinced--yours looks way better. I am however mildly worried that it could be buggy (while loops, errors at the beginning/end of files, etc). Could you add a test add exercise a couple corner cases? (imo as long as it raises the correct exception and not some exception that would be the result of going out of bounds or whatever, that's a "successful" test. No need to test actual content too much.)

  5. I like this term

  6. 
      
ZU
JS
  1. 
      
  2. src/python/pants/base/build_file.py (Diff revision 3)
     
     

    pass here too or in none of them

  3. src/python/pants/base/build_file.py (Diff revision 3)
     
     

    this needs some more indent now

  4. This raises ValueError on a bad spec so that will probably need some cleanup too or else trap/raise wrapped here.

  5. 2 spaces -> 1: s/raise /raise /

  6. Fwict this is a symbol not in scope - I think you meant self.InvalidAddressException

  7. The .format is at odd indent, -2 should fix, and then I think the spec_path argument can fold back up to this line.

  8. Kill extra blank line.

  9. 
      
ZU
JS
  1. 
      
  2. So deeply broken you may want to just leave alone - there is no Address.parse method, there is only a SyntheticAddress.parse and even then its 2 arguments semantically are a mis-match for these 2: https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/address.py#L180

  3. Excellent - thanks for hooking this back up.

  4. 
      
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

commit b1545f2
Loading...