Use ProjectTree in SourceRoots.all_roots().

Review Request #4079 — Created July 13, 2016 and submitted

benjyw
pants
3450
pants-reviews
jsirois, nhoward_tw, stuhood
Instead of the custom logic currently in there.

Required a little rearranging of ProjectTree-related code.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/144861405

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
  1. Thanks Benjy!

  2. This file seems like it would make more sense in src/python/pants/bin; without having it here, the *project_tree.py files wouldn't need a dependency on build_environment.

    1. This is depended on by source_roots.py, so it can't be in /bin.

      I was considering moving the whole thing to a new top-level package, say src/python/pants/project. What do you think?

    2. That would work... or under src/python/pants/source I suppose? It would likely also be fine as a separate target under src/python/pants/base... but I guess you're heading toward 1:1:1 for these packages, so that's probably fine. Not a strong objection either way: will drop the issue.

  3. Ignore patterns is for BUILD files, and is only consumed in BuildFile currently. So this is correct.

  4. Opened https://github.com/pantsbuild/pants/issues/3671 about this: can mention here.

  5. src/python/pants/source/source_root.py (Diff revision 1)
     
     

    Nice!

  6. 
      
  1. Looks good!

  2. src/python/pants/source/source_root.py (Diff revision 2)
     
     

    Why not use the project tree here as well? Then you could get rid of the get_buildroot call in favor of get_project_tree.

    One reason we wouldn't use it here would be to allow fixed roots to ignore the pants ignore options for some reason. If fixed roots are supposed to ignore the ignore patterns, I think it would be good to ensure that's documented.

    1. Allowing fixed roots to ignore ignore patterns seems unwise (esp. since the new engine looks only at the project_tree, I believe). But I like your first idea, will change as you suggest.

    2. While at it, I also got rid of the other uses of get_buildroot in source_root.py, to emphasize that all root computation is relative to the buildroot.

      There was only one place (context.py) where we were relying on SourceRoots relativising absolute paths for us, and even that was unnecessary.

  3. 
      
  1. Ship It!
  2. 
      
  1. 
      
  2. FWIW: I think python_thrift_library should just move to the python backend instead. As it stands, a python-only project must depend on the codegen backend to do codegen, which pulls in jvm stuff.

  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

b9a7932ca5677341189523b550123235aebd0d16

Loading...