Change how we detect the buildroot.

Review Request #3489 — Created Feb. 23, 2016 and submitted

benjyw
pants
pants-reviews
jsirois, kwlzn, mateor, patricklaw, stuhood, zundel
Instead of looking for pants.ini we look for the pants runner script
(or any file named 'pants') in the cwd and its ancestors.

This is because we don't want to give the location of pants.ini
any special extra meaning.  We want pants to work with multiple cascading
config files (instead of the clunky --config-file-override mechanism)
or even no config file at all. A future change will implement configurable
config file locations, and will explain in greater detail why that's a
good thing...

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

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
PA
  1. Ship It!
  2. 
      
MA
  1. Ship It!
  2. 
      
NH
  1. It looks like these other test locations expect pants.ini as the marker:
    - base_test.py near # We need a pants.ini, even if empty. get_buildroot() uses its presence.
    - ReproOptionsTest looks like it may also implicitly rely on pants.ini, though I'm not as sure.

    1. All the tests pass as is, so hmm... Maybe it's just the comment that needs changing. Will take a look.

    2. Yep, verified that all tests pass when removing that pants.ini creation entirely. Which makes sense, since all tests set the BuildRoot explicitly now.

  2. src/python/pants/base/build_root.py (Diff revision 1)
     
     

    While you're in here, could you add a test for the not found error case?

  3. 
      
TA
  1. Ship It!
  2. 
      
MO
  1. looks good to me, but we should probably update setup_repo.md to reflect the fact that a file named pants is now has significance.

  2. 
      
ST
  1. Would still prefer to see this be less hardcoded.

  2. src/python/pants/base/build_root.py (Diff revision 1)
     
     
     
     

    I continue to lightly object to using a single hardcoded file to locate. Would prefer that this be a default list of heuristics from which things could be removed.

    Strawman:
    - have a prioritized list of [.git, .hg, pants.ini, pants], such that if you wanted to
    - special case an environment variable (similar to a PATH) to allow overriding the list to add or remove things

    1. Burning down the strawman: using an environment variable alone to override wouldn't be enough. We want builds to be hermetic...

      If the file were a true sentinel and for just this one purpose I would have no problem using a file as a sentinel.

    2. I'm not a fan of the prioritized list search - preferably there'd be only *one* explicit way to mark something a buildroot that is uniform across all pants repos. #simplify

    3. @Kris: how do you address this TODO then?

    4. Ah, I see your suggestion below.

    5. @Eric: the build would still be hermetic if (if you had to override the default search path) you set the environment variable in your pants script, and didn't allow overriding it. But I think the vast majority of users would be fine with a default set of heuristics, which is why things like "create an explicit BUILDROOT file" grate on me a bit.

    6. !?! If that's the way its meant to be used, seems like it should be a command line option (which you can also set via env variables)

    7. That's why it was a strawman ;)

    8. How about we make this more complicated if and when there's a need? It would be super-easy to change. And in practice it seems fine to insist on a runner script in buildroot. We basically do already. And if the fallback is "touch an empty file called pants in the buildroot", that seems fine to me. The only issue will arise if anyone has such a file in a subdir AND runs pants from subdirs (which is itself not officially supported/tested), and I'm happy to cross that bridge when we come to it, if others are OK with it.

    9. What I mean is that changing it by adding detection of a BUILDROOT sentinel file in the future wouldn't break any current repos, so I don't see a strong need to anticipate all eventualities now.

  3. src/python/pants/base/build_root.py (Diff revision 1)
     
     

    Should add an explanation for how to fix this, or link to a spot in the docs explaining how a user would fix it.

    1. Also added docs about the requirement.

  4. 
      
ZU
  1. I agree with the comments raised by the other reviewers but I don't see anything further that would block.

    I ran find . -name pants -type f in our repo and it came back with just one ./pants at the root, just to be sure it would work for us.

  2. 
      
KW
  1. 
      
  2. src/python/pants/base/build_root.py (Diff revision 1)
     
     
     
     

    seems to me that a file named pants could be more likely to exist in a pants repo subdir than one called BUILDROOT. BUILDROOT is also more explicit - and it'd be really nice to have a single clear and explicit way to configure a buildroot.

    so why not just jump to BUILDROOT now? may save us some time/grief in the long run.

    I also don't quite see how depending on pants (the path to the runner script) is any better philosophically than depending on pants.ini (the path to the config).

    1. Because we ~require a pants runner script, and we don't/shouldn't require a pants.ini, and certainly shouldn't require it to be in the buildroot.

  3. 
      
BE
NH
  1. Ship It!
  2. 
      
BE
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as f5e2d61a127cc6225bb47f23cd2f2519c63dc535.

BE
  1. Submitted as f5e2d61a127cc6225bb47f23cd2f2519c63dc535. Thanks for all the reviews and comments. Please be on the lookout for any issues when you deploy this.

  2. 
      
Loading...