Properly order resources for pants goal test and pants goal run.

Review Request #845 — Created Aug. 7, 2014 and submitted

zundel
pants
zundel/duplicate-resource-paths
449
pants-reviews
benjyw, jinfeng, jsirois, patricklaw
Fixed a problem with goal run and test for resources being put on the classpath in the
wrong order when multiple resource sources define a file with different contents
at the same resource path.

Added a project with resources that collide and an integration test to prove
that resources are bundled and added to the classpath in the right order.

Note that the JVM tests will still fail if all run together with a command like:

   pants goal test maven_src/resource_collision::

because pants runs all the tests in the same junit test runner instance.
Added an integration test for pants goal bundle and pants goal run.

Ci is baking at https://travis-ci.org/pantsbuild/pants/builds/32999609
  • 0
  • 0
  • 9
  • 0
  • 9
Description From Last Updated
ZU
  1. I'd like to include running the tests in maven_src as a part of ci.sh, but right now they break.  I thought about working around this by adding tests/python/pants_test/test_junit_tests_integration_test.py to invoke the tests individually, but that seems kind of silly.
  2. .gitignore (Diff revision 1)
     
     
    I think the exclusion means to exclude top level dirs in the repo, but it is matching any subdirectory with that string as a suffix.  The 'lib' line is the one causing a problem for me.  I am not 100% confident in making this change for all of the dirs being excluded above.
    1. I ended up just changing the ones I was confident in.
  3. Hopefully this entire BUILD file can go away soon.
  4. I added this line in an attempt to get pants goal test maven_src/resource_collision:: to work. It doesn't seem to have any effect.  Did I specify it correctly?
    1. @Benjy, did I do something wrong here or is this known to be broken.  Should I leave it in?  
  5. Once the entire path was changed to use OrderedSet, this change was the key to getting the resource paths on the classpath in the correct order.
  6. 
      
PA
  1. lgtm mod issues raised
  2. .gitignore (Diff revision 1)
     
     
    How did this work?  We should be producing (uncommitted) pyc files constantly.
    1. It turns out this list is redundant with the top entries in .gitignore.
  3. This return should be outside of the context manager (as should everything after `java_run.communicate()`, if I understand this correctly)
  4. 
      
ZU
ZU
  1. 
      
  2. .gitignore (Diff revision 1)
     
     
    If you hit 'expand changes' it turns out this list I removed is redundant with the top several entries in .gitignore.
  3. 
      
BE
  1. 
      
  2. .gitignore (Diff revision 2)
     
     
    Why the changes in this file?
  3. Thanks for the comprehensive test. However I don't love having maven_src at the root level of the workspace. This is for testing, so it should probably be under test/ or something. Or some name that makes it clear that this is not for pants sources.
    1. I understand your hesitation of adding a new toplevel directory and the name 'src' does imply that it contains pants core code.  
      
      I debated a bit about where to put it.  The  code isn't just a test, it is an example of how to use maven_layout that doesn't exist elsewhere.  Our other examples are in /src/ .  Granted, some projects are silly and contrived like this one, so when we had this issue before, we created a special java package under /src/java/com/pants/testproject.  That isn't all that clean and it isn't going to inform us on how to fix it here.
      
      What I don't like about stuffing it under tests/ is that when we go to write examples, where will we put the tests for those?  The only way that would work for maven would be to put them under /src/maven_src/<example>/src/main/test ?  So I concluded that neither /src/ or /tests/ were the right place to put this code.
      
      What do you think about this idea?  
      
      /maven_layout/examples/<project>/  # for example code using the maven filesystem layout
      /maven_layout/testproject/<project>/  # for code that tests pants using the maven filesysystem layout
      
      the only thing I don't like about that is that it is confusing to have the maven_layout() directive with the same name, but hopefully we are going to get rid of that mechanism soon.
    2. This now makes use of the 'testprojects' top level dir.  
  4. 
      
ZU
  1. 
      
  2. .gitignore (Diff revision 2)
     
     
    rbcommons isn't helping us out much here.  I had 2 comments on this change elsewhere.
    
    1) the issue I ran into was that lib/ is not specific enough.  My change to add a directory named '.../lib/...' couldn't be added to git.
    
    2) I changed the other dirs I knew were supposed to be relative to the repo root, not anywhere in the tree.
    
    3) the patterns from *.class on down are redundant.
  3. 
      
ZU
ZU
JI
  1. 
      
  2. i assume this is not passing for you neither (see my RB: https://rbcommons.com/s/twitter/r/922/)
    
    How should we go forward on this test file? You check in first as is, and then I continue my fix; or you take over the additional fixes? or...?
    1. I hacked on the tests I marked 'xfail' in this patch for a while and gave up.  Marking them 'xfail' allows us to commit the changes and include the file back in even though not all of them are working.
      
      A lot of them were throwing new exception types and made me wonder if some of these should be moved to another test like tests/python/pants_test/base/test_build_file_address_mapper.py
    2. Fixing up this is on my shortlist of TODOs when I finish up some internal feature work.  If someone gets to it first, you might want to take a look at the AddressMapper CR to see how BuildGraph, BuildFileParser, and AddressMapper have changed out from under this test.  Feel free to IM or email me if you're working on this test set with any questions you might have.
  3. 
      
ZU
  1. 
      
  2. I hacked on the tests I marked 'xfail' in this patch for a while and gave up.  Marking them 'xfail' allows us to commit the changes and include the file back in even though not all of them are working.
    
    A lot of them were throwing new exception types and made me wonder if some of these should be moved to another test like tests/python/pants_test/base/test_build_file_address_mapper.py
     
  3. 
      
JS
  1. I buy Target.walk and its underlyings having an ordering guarantee, maybe closure or a new ordered_closure, but this change seems like its tossing in additional OrderedSets that are unrelated to these one or 2 gurantees to build from.  I suspect this is just you wanting to stamp out all sources of scramble.  I'd prefer a tighter api where we doc what returns / acts in-order and then doc the rest as un-specified order, but I won't push on this except for the positive doc cases.  It would be nice to have a record of the apis we explicitly guarantee order for so a few months from now when we get an in-memory build graph server and we can actually measure the slowness caused by OrderedSet we don't refactor away the actual critical ones.
    1. I guess with a server it wouldn't be slowness, but memory consumption in the steady state ... but hopefully my gist is clear.
    2. It may look like that, but I did not just throw these in willy nilly.  I walked my way upward from failing builds adding in ordering along the way.  Maybe I ordered a few too many, but it isn't a simple problem.  There are multiple paths that break depending on whether you are doing goal run, goal test, or goal bundle.  I'll make another pass and add some doc where I think we need ordering guarantees.
  2. 
      
ZU
JS
  1. Thanks Eric
  2. build-support/bin/ci.sh (Diff revision 4)
     
     
    You may or may not like this modernish bashism:
    # array
    known_failing_targets=(
      testprojects/maven_layout/resource_collision/example_a/src/test/java/com/pants/duplicateres/examplea:examplea
      testprojects/maven_layout/resource_collision/example_b/src/test/java/com/pants/duplicateres/exampleb:exampleb
    )
    # replace #beginning (^) of each array element with --exclude-target-regexp=
    exclude_opts="$(echo ${known_failing_targets[@]/#/--exclude-target-regexp=})"
  3. 
      
ZU
JS
  1. Ship It!
  2. 
      
ZU
ZU
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

@Benjy, I think I addressed all of your feedback, I'll be happy to circle back if you have more.

commit 541f13c
Loading...