Adjust the output file locations for the Antlr task

Review Request #4161 — Created Aug. 14, 2016 and submitted

lex-square
pants
3787
pants-reviews
jsirois, stuhood, wisechengyi

Adjust the output file locations for the Antlr task.

The Antlr rules don't currently lead to a happy IntelliJ configuration. This PR is one way to fix it, although I'm not really sure what the best approach is.

The root problem is that Antlr dumps its output files into a path determined by the relative path specified for the input file. This PR takes the simple approach of moving all the files once they have been generated.

As a possible alternative, the files could be left in their current location, and the IntelliJ support could be modified to generate a better source root for the resulting code. It seemed slightly better, though, to try and fix the file locations earlier in the pipeline, right when they are generated. Antlr has predictable output, so it's not too hard to just fix it up.

I checked the behavior with the checked-in Antlr samples:

./pants  idea ./examples/src/java/org/pantsbuild/example/antlr3::
./pants  idea ./examples/src/java/org/pantsbuild/example/antlr4::

I also ran the the unit-test.sh script.

Travis CI has passed: https://travis-ci.org/pantsbuild/pants/builds/152249047

  • 0
  • 0
  • 7
  • 0
  • 7
Description From Last Updated
  1. 
      
  2. 3rdparty/python:scandir would be preferable compared to os.walk

  3. It might be good to add a test case, so it would be clear what we are expecting.

  1. Thanks for the contribution Lex.

    Can you update the RBcommons as follows?
    - Put the PR number in the Bug field.
    - Add stuhood, wisechengyi, and jsirois to the reviewers list
    - Add a link to the passing CI build in the 'tests' section. I see it passed at https://travis-ci.org/pantsbuild/pants/builds/152249047

    Also, it would be good to add a test if this case is not already covered. The easiest kind to make is to create some files under testprojects that have this property and compile them. Then, add a pants command that compiles and runs the code in tests/python/pants_test/backend/codegen/tasks/test_antlr_integration.py Better would be to test to see that the sources go into the right place under the workdir. That might require adding a unit test to tests/python/pants_test/backend/codegen/tasks/test_antlr_gen.py

    1. Thanks. I have mode the requested edits to the RBcommons information (bug#, reviewer, link to Travis). Thanks for the detailed pointers on how to write good tests for this code!

    2. I've added a test now in test_antlr_gen.py that the path names are as expected.

      I looked into test_antlr_integration.py, and I believe it already has a pretty good test for this. It already includes a test that generates ANTLR code, compiles it with Javac, and runs the result.

  2. So, just so I have it right, this shouldn't change the output of the java compiler (that is determined by the package statement in the file) but it will make the generated source code match up with the java package structure declared.

    1. Yes, that is exactly the idea. This patch just affects the path to the Java files within the .pants.d/gen/antlr/HEX/TARGET/HEX directory. Instead of paths like examples/src/antlr/org/pantsbuild/example/exp/ExprAntler3Parser.java being generated, it renames them to be like org/pantsbuild/example/exp/ExprAntler3Parser.java. The class files produced by Javac should be the same, but it makes a difference in an IDE.

  3. 
      
  1. Thanks for the change! A few minor issues:

  2. nit: preferred docstring style is:

    """First sentence ending with a period.
    
    Continuing description after empty line. Lorem
    ipsum etc etc. Then the end quotes get a line to
    themselves.
    """
    
  3. Use os.path.sep instead of '/'

  4. Just use safe_mkdir(package_dir) here (it's atomic).

    It's in pants.util.dirutil, which it looks like this file already gets safe_walk from.

    You probably actually want to use safe_mkdir(package_dir, clean=True) to make sure the directory is present and empty before writing stuff to it.

    1. In this case, it seems possible that the source files already match the expected target names. So it is better not to call clean=True.

  5. You can omit the .replace(...) after making the above change to os.path.sep

  6. Don't use file as a variable name. It's a built-in function that you're unintentionally shadowing here.

  7. dir is also a built-in function and thus a discouraged variable name.

  8. More idiomatic to just use if not os.listdir(full_dir): here

  9. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Lex! Merged to master at fb960ab

  1. Ship It!
  2. 
      
Loading...