Fix "ValueError: too many values to unpack" when parsing interpreter versions.
Review Request #3411 - Created Feb. 2, 2016 and submitted
|benjyw, jsirois, nhoward_tw, patricklaw|
When encountering failures translating
wheelduring 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.
safe_concurrent_createto 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.
CI is green @ https://travis-ci.org/pantsbuild/pants/builds/106402640
Verified interpreter_cache repro test fails pre-fix both in CI and locally.
|Definitely a better implementation, but why the name change? We generally name functions as imperatives ("do this") rather than as ...||Benjy Weinberger|
Awesome! That bug has been really annoying. I've got a few comments, but otherwise it LGTM!
The docstring should talk about the string it yields and the directory creation too.
Could you add a test showing that the file is written even if there is an exception?
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.
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
Address Nick's feedback.
Revision 2 (+81 -21)
Status: Closed (submitted)
thanks Nick, Yi & Patrick! this is in @ d4e95750f94b046d40e252ff8a4aada43000f68e
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
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: