Molsen/add pex root option

[digwanderlust]

No description provided.

[kwlzn]

the help string for this option references the default value used here, so it seems to me that the help output for this option would go from:

    --interpreter-cache-dir=INTERPRETER_CACHE_DIR
                        The interpreter cache to use for keeping track of
                        interpreter dependencies for the pex tool. [Default:
                        /Users/kwilson/.pex/interpreters]

to this:

    --interpreter-cache-dir=INTERPRETER_CACHE_DIR
                        The interpreter cache to use for keeping track of
                        interpreter dependencies for the pex tool. [Default:
                        {pex_root}/interpreters]

how about updating the help strings both here and for --interpreter-cache-dir with the literal defaults (e.g. the ~/.pex/... path) so it's more user friendly?

[digwanderlust]

My concern with that is that it is then no longer clear to the user that changing pex root will change a portion of this file path. And leaving the default of a static pex root as the actual default will be result in it being possible for the user to update pex root and not have it impact several of the dirs that are supposed to be subdirs of pex root.

[digwanderlust]

fixed

[kwlzn]

I think you're misunderstanding my original ask. it seems like setting a default value for --pex-root doesn't really change anything here and would obviate the if options.pex_root: check below. can you reconcile that if you decide to continue down this route? otherwise I think this was better as it was before.

regarding the defaults strings - this definitely needs to be made clearer. at a minimum tho, we should have sane values for the 99% case where folks aren't passing --pex_root that doesn't require users to understand how .format strings work and apply them in a contextless situation.

how about including the /literal/ default in each options /help string/ along with a note about how modifying pex_root will also affect the default value?

[kwlzn]

lgtm overall mod a couple of inline comments and two additional requests:

1. would you mind adding some quick test coverage - integration or otherwise? while you're in here repairing this it'd be good to add some extra insurance to make sure it stays that way.

2. could you rebase your commit history and update the PR title? for the PR workflow, this all gets merged into master - so ideally your PR would land as one big squash commit like an RB patch.

[kwlzn]

this lgtm. any thoughts on the test coverage ask?

[digwanderlust]

A lot of the changes were basically default changes. I would like to add a test for the nested dirs under pex_root to make sure they get updated correctly. Any other specific areas you want me to tackle?

[kwlzn]

not crazy about this bit, how about just:

def main(args, log=log, exiter=sys.exit):
...
  exiter(pex.run(...))
...
if __name__ == '__main__':
  main(sys.argv[1:])

and then eliminating all of the if log_callback & if args checks + the raise SystemExit in favor of just passing in test-context values for these?

[digwanderlust]

I'd rather not put mutable objects in as the default options, which is why I went with None. As far as the sys.argv[1:] being placed in main I'd rather not put that logic somewhere it can't be easily tested.

[kwlzn]

I'm not sure I understand your statements or reasoning here.

"I'd rather not put mutable objects in as the default options, which is why I went with None." -> what default option here is mutable? log is a function defined above. sys.exit is also a function. if anything, the whole 'log can be either be a global or local depending on the value of log_callback' strikes me as most peculiar.

"As far as the sys.argv[1:] being placed in main I'd rather not put that logic somewhere it can't be easily tested." -> somehow I feel like the slicing of sys.argv isn't a super critical thing to test? also, by precedent injection of args is actually a super common pattern in twitter.common.app. what's the theoretical harm here in your mind? or why not alternatively main(sys.argv)?

I'm a firm -1 on avoiding needless clutter in main() for tests and particularly not a fan of this approach as-is - would prefer that you either clean this up in-line with the suggestions or find an alternate route.

[digwanderlust]

mutable object was in ref to sys.argv. I generally think that the slice should still be in testable code it would make a difference for example if you were to change option frameworks to docopt or argparse. However, that being said its not super critical and I've moved it.

Also removed the log=None default.

Spoke with you offline and you generally seem okay with SystemExit approach.

[digwanderlust]

looking through this, updating log in main() doesn't actually help since several other methods are using log as a global as well. I'll update the code later with an alternative that allows you to update the logger for all methods.

[kwlzn]

this change still feels weird to me - and I think to others it will also. since the two are fundamentally identical, why not just stick with the more intuitive/common form of sys.exit?

fwiw, you shouldn't need to explicitly raise here to be able to handle SystemExit in your tests:

>>> try:
...     sys.exit(1)
... except SystemExit:
...     print('not exiting!')
...     
... 
not exiting!

[digwanderlust]

No strong objection. I'll update the code to use sys.exit rather than raise SystemExit.

[kwlzn]

one quick comment - otherwise lgtm

[kwlzn]

thanks @digwanderlust - merged!