Fix "ValueError: too many values to unpack" when parsing interpreter versions.

Review Request #3411 — Created Feb. 2, 2016 and submitted

kwlzn
pants
kwlzn/interpreter_crash
2038, 2872
pants-reviews
benjyw, jsirois, nhoward_tw, patricklaw

When encountering failures translating setuptools and/or wheel during interpreter bootstrap, the current interpreter cache setup logic can leave temporary-named directories behind (e.g. CPython-2.7.8.tmp.52b0f0a2542a44bfb16f247ec9f6215a). This causes issues during subsequent runs when trying to parse interpreter versions from the cache.

  • Convert safe_concurrent_create to a context manager (with a slight rename to align with the contextmanager context).
  • Safeguard tmp path conversions with a finally block.
  • Misc cleanup.
  • Test coverage.

Closes #2038

CI is green @ https://travis-ci.org/pantsbuild/pants/builds/106402640

Verified interpreter_cache repro test fails pre-fix both in CI and locally.

  • 1
  • 0
  • 3
  • 0
  • 4
Description From Last Updated
Definitely a better implementation, but why the name change? We generally name functions as imperatives ("do this") rather than as ... BE benjyw
NH
  1. Awesome! That bug has been really annoying. I've got a few comments, but otherwise it LGTM!

  2. src/python/pants/util/dirutil.py (Diff revision 1)
     
     

    The docstring should talk about the string it yields and the directory creation too.

  3. Could you add a test showing that the file is written even if there is an exception?

  4. tests/python/pants_test/util/test_dirutil.py (Diff revision 1)
     
     
     
     

    Rather than raising here, I think it'd be more clear if you just passed in the with body and asserted the file didn't exist.

    Also, you could test that even if the tmp file wasn't written to, the directory structure will be created.

    1. sure, makes sense. a good slice of my intent with this was to generally ensure correct exception propagation inline in this test, but I suppose that's not strictly necessary given the code in question (and is now covered by another test).

  5. 
      
WI
  1. Nonblocking comment. When parsing the version of python, is it better to directly call '/path/to/python --version' rather than trying to parse the directory it is in? so there is no need to deal with the tmp names

    1. this is specific to paths in the interpreter cache, which already come from the equivalent of prior calls to python --version serialized as directory names.

  2. 
      
PA
  1. =Nick on the test comments. Otherwise lgtm.

  2. 
      
KW
NH
  1. Ship It!
  2. 
      
KW
Review request changed

Status: Closed (submitted)

Change Summary:

thanks Nick, Yi & Patrick! this is in @ d4e95750f94b046d40e252ff8a4aada43000f68e

BE
  1. 
      
  2. src/python/pants/util/dirutil.py (Diff revision 2)
     
     

    Definitely a better implementation, but why the name change? We generally name functions as imperatives ("do this") rather than as nouns ("the thing that will be done").

    E.g., in this very file we have safe_concurrent_rename, not safe_concurrent_renaming.

    Or if you want a contextmanager example, we open files like this:

    with open(path) as f:

    not like this:

    with opened(path) as f:

  3. 
      
Loading...