Scrub timestamp from antlr generated files to have stable fp for cache

Review Request #2382 — Created June 16, 2015 and submitted

qma
pants
1701
bcd7a26...
pants-reviews
areitz, fkorotkov, jsirois, stuhood

Add integration test and fix unit test

Removes only first line

Remove line used for debugging.

Running CI: https://travis-ci.org/pantsbuild/pants/builds/67450219

Local Testing:

(in master)
$ PANTS_DEV=1 ./pants clean-all compile ./examples/src/antlr/org/pantsbuild/example/exp::
$ head /Users/qma/workspace/pants/.pants.d/gen/antlr/antlr3/gen-java/examples/src/antlr/org/pantsbuild/example/exp/ExpAntlr3Lexer.java
// $ANTLR 3.4 examples/src/antlr/org/pantsbuild/example/exp/ExpAntlr3.g 2015-06-16 15:45:46

  package org.pantsbuild.example.exp;


import org.antlr.runtime.*;
import java.util.Stack;
import java.util.List;
import java.util.ArrayList;

(in this branch)
$ PANTS_DEV=1 ./pants clean-all compile ./examples/src/antlr/org/pantsbuild/example/exp::
$ head /Users/qma/workspace/pants/.pants.d/gen/antlr/antlr3/gen-java/examples/src/antlr/org/pantsbuild/example/exp/ExpAntlr3Lexer.java

  package org.pantsbuild.example.exp;


import org.antlr.runtime.*;
import java.util.Stack;
import java.util.List;
import java.util.ArrayList;
  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
QM
JS
  1. This is great, but it absolutely needs a test.  Please huddle with tweeps for pointers if you need them, but at the very least an integration test would be great.
    1. FYI: Travis CI bandwidth is at a premium and your CI run failed on unit-test shard 2 with a legit error so I stopped the run: https://travis-ci.org/pantsbuild/pants/builds/67114016
      We encourage active management of CI runs like this since bandwidth is at a premium.  Normally only the pull-requestor should manage their own CI runs, but I took this as an opportunity for spreading the word.
    2. Thanks. I will watch CI in future. Now running https://travis-ci.org/pantsbuild/pants/builds/67150157

  2. 
      
QM
QM
QM
QM
ST
  1. 
      
  2. Don't we know that this only occurs at the head of the file? antlr3 isn't exactly changing underneath us... doing the simple obvious thing is probably fine.

    1. This seems safer to me than "if antlr3: remove the first line". You are right that antlr3 is unlikely to change, but in the off chance it does, this has less chance to over-delete. (for example, the first line of antlr4 generated file is package or import)

    2. So apply your regex to only the first line.

    3. Qicheng - please fix your pants-reviews subscription from qma@twitter.com to qma.twitter@gmail.com - your responses via RB are getting bounced by the list since the subscription does not match.
    4. I unsubscribed @twitter and kept @gmail.

    5. Thanks - I un-moderated the qma.twitter@gmail.com account, you should be good to go now.
  3. 
      
QM
QM
QM
QM
ST
  1. Thanks Qicheng.

  2. nitpick: periods on the ends of comments in this codebase

  3. 
      
QM
JS
  1. Small stuff.
  2. It seems to me this would make more sense up in `genlang` as a post-compilation step.  Its the bit of code that knows about the antlr compiler.
    1. Do you mean this line specifically or this entire scrubbing section? The scrubbing code needs the generated generated_sources, which seems to me better to stay here right before generating the temp targts, rather than up in genlang

    2. The entire scrubbing, but this is debatable to be sure so I'll defer to you here.
  3. Its worth a note that this only works as a test because AntlrGen does not use the artifact cache, just the local build invalidator.  As such, the clean-all actually forces it to re-gen _unlike_ the jvm compile tasks.  If AntlrGen starts using the artifact cache this test will pass w/o your fix since the input .g files have not changed between runs.
    1. will add a comment. Do you see other ways to test this that works even when antlr starts using artifact cache?

    2. Yes - with a TaskTest (more unit-y), you could run the gen task only 2x with a 1 second sleep in between (this test should really have this too to make sure the timestamp rolls over, but its slow so almost guarnteed to always take >1 second between the 2 runs).
      You'd directly compare file contents and still do this in a black-box way, consulting the context products map to get the file paths generated, then reading them in.
    3. You lost me there, can you elaborate more what to do in a TaskTest? I don't want to simply verify that the first line is remove, I want to verify that the ultimate compiled artiact has stable fingerprints so cacheable, this technically is a more strict result of this fix, and it will serve to prevent other kind of regression that make antlr targets non cacheable as well.

    4. That's exactly what I'm suggesting.  A tweep is probably in a better position to help you with writing a TaskTest [1], but suffice it to say, it can create a Task object for you and then you can directly call `execute` on it instead of shelling out to pants as is done in integration tests.  I mis-spoke when saying you'd test context products, instead you'd test the sources of the synthetic targets you generate (they are added to the context and they can be found by eximaning Target.derived_from).  But the test would be the same, read the sources from the synthetic target opaquely and confirm they are the same before and after >1 second passes.  This ignores issues of timestamps and does the stricter, more robust check of - gor the same inputs, are the outputs the same - byte-wise.
      
      [1] Here's the base class from which you can grep the tests/python tree for many examples: https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/tasks/task_test_base.py#L25
    5. I guess you wouldn't even need to read the sources, you could be even more robust by just calling Target.(transitive_)invalidation_hash on the synthetic targets and confirming the hash does not change.
  4. 
      
QM
JS
  1. Thanks for adding the unit test Qicheng.
    
    It looks like you need to fix import sorting: https://travis-ci.org/pantsbuild/pants/jobs/67437046
    ```console
    [== 00:00 Running pre-commit checks ==]
    Checking packages
    Checking imports
    ERROR: /home/travis/build/pantsbuild/pants/tests/python/pants_test/backend/codegen/tasks/test_antlr_gen.py Imports are incorrectly sorted.
    To fix import sort order, run `build-support/bin/isort.sh -f`
    ```
    
    After doing that, make sure to update the testing done with the final green CI link and also fill in the RB bugs field with 1701
    1. build-support/bin/isort.sh -f did nothing to import order, but added a newline at end of file. Is it a confused error message? I ran build-support/bin/pre-commit.sh to Success.

    2. Yes - its an inappropriate error message.  The isort tool does 2 things, import sorting and the newline at EOF check.
  2. This deserves a comment line - why sleep?  Its cryptic taken out of context.
  3. Since you bit the bullet and stuck around to make better tests, this test should really get the 1 second sleep with comment as well.
  4. 
      
QM
QM
ST
  1. Thank you Qicheng.

  2. 
      
JS
  1. Thanks Qicheng.
    I updated the testing done to the green CI @ https://travis-ci.org/pantsbuild/pants/builds/67456736 and this is now patched in to master @ https://github.com/pantsbuild/pants/commit/db4c7344a6b30910247b8a12155c81755dbd5cc0
    Please mark this RB as submitted.
  2. 
      
JS
  1. Ship It!
  2. 
      
QM
  1. Thanks John and Stu!

  2. 
      
QM
  1. Thanks John and Stu!

  2. 
      
QM
Review request changed

Status: Closed (submitted)

Loading...