Allow the buildroot to be a subdirectory of the git worktree

Review Request #886 — Created Aug. 13, 2014 and submitted

stuhood
pants
405
pants-reviews
jsirois, lahosken, patricklaw
This allows the buildroot to be a subdirectory of the git worktree, which supports a usecase we have internally where two pants builds will live within the same git repo for a while until they can be merged.

- Add a class method to attempt to detect a git worktree
- Report changed_files relative_to some directory (usually the buildroot)
- confirmed that files outside the relative_to path/directory are ignored
- added test for worktree detection at various levels of nesting
- `./pants goal changed` and `./pants goal bundle` succeed in a buildroot below the worktree root (and in existing projects)
  • 2
  • 0
  • 5
  • 1
  • 8
Description From Last Updated
Any reason this isn't the default? PA patricklaw
Again it'd be nice to have `get_buildroot()` be the default PA patricklaw
LA
  1. Seems sane to me. You should wait for a smarter person to chime in, tho.
  2. 
      
PA
  1. 
      
  2. Any reason this isn't the default?
    1. The "scm" package doesn't currently depend on the "build_environment" package, and it seemed unnecessary to add it for this. I can if you think the coupling is worthwhile.
      
      My impression is that this class was designed to be used generically outside of pants, which has meant that the module boundaries don't involve passing the buildroot explicitly.
  3. We should pull these functions + global state into singleton classes rather than use python globals, which are the worst
    1. Agreed... there is a TODO next to them about it.
  4. Might want to log here indicating that it was detected but failed to get set?
  5. src/python/pants/goal/workspace.py (Diff revision 1)
     
     
    Again it'd be nice to have `get_buildroot()` be the default
  6. src/python/pants/scm/git.py (Diff revision 1)
     
     
    Just to be super explicit, it's typically `get_buildroot()`.  Or I guess I should be asking: what's the behavior when the user is running pants from not-buildroot?  That situation has been discussed recently as being something we want to support.
    1. > Just to be super explicit, it's typically `get_buildroot()`.
      Well, except that this change was intended to remove that restriction.
      
      Since the worktree detection is based on git being able to find itself above `cwd`, this class will work just fine in any subdirectory of the repo.
  7. src/python/pants/scm/git.py (Diff revision 1)
     
     
    This function name is awkward.  Seems like it should be `rerelativize_to` or just `relativize_to` or maybe `fix_git_relative_path`
  8. src/python/pants/scm/git.py (Diff revision 1)
     
     
    Can drop the [] and let this be a generator comprehension
  9. tests/python/pants_test/scm/test_git.py (Diff revision 1)
     
     
    Given the variety of ways this change can affect SCM detection, some extra integration tests would be appreciated
  10. 
      
ST
DT
  1. 
      
  2. You don't need the pass anymore.
  3. 
      
ST
ST
PA
  1. Ship It!
  2. 
      
JS
  1. I won't block, but this seems less useful than the single change of respecting GIT_DIR and GIT_WORK_TREE when present in the environment.
    1. I agree that that would be an additional nice feature. But I don't think it's the right UI for the particular problem we're solving... not when there is a generally accepted way to detect the worktree automatically. Yes, there will almost always be a pants wrapper script involved, but making it thinner where possible seems reasonable.
  2. src/python/pants/scm/git.py (Diff revision 3)
     
     
    I still contend honoring the standard GIT_* env vars when no explicit worktree or gitdir is passed - if present - is the right thing to do here.
  3. src/python/pants/scm/scm.py (Diff revision 3)
     
     
    This feels like pushing logic into the Scm/Git that the user could be dealing with.
    1. You mean the caller of changed_files should re-relativize? The problem is that the worktree is private (which, IMO, is good), and so they don't have the information necessary to re-relativize.
      
      My initial cut converted this to returning absolute paths, but I was concerned about the sideeffects of that change.
    2. Yes - this comment was based on the premise the caller said export GIT_WORK_TREE=/foo/bar so they could relativize wrt $GIT_WORK_TREE
  4. 
      
ST
Review request changed

Status: Closed (submitted)

Loading...