bpo-23892: Introduce sys.implementation.opt_levels #9826
Conversation
csabella
requested review from
ericsnowcurrently and
pfmoore
and removed request for
ericsnowcurrently
| @@ -814,6 +814,11 @@ always available. | |||
| ``cache_tag`` is set to ``None``, it indicates that module caching should | |||
| be disabled. | |||
|
|
|||
| *opt_levels* is a tuple containing the available compiler-supported | |||
| optimization levels. See :pep:`488` for more information. | |||
There was a problem hiding this comment.
I think that mentioning the possible values, and the relation to the "optimization" parameter of importlib.util.cache_from_source(), is perhaps warranted here (in addition to linking to PEP-488).
There was a problem hiding this comment.
+1
The relationship between sys.flags.optimize and that "optimization" parameter (as well as the "optimize" parameter of compile()) reflects a gap in the solution presented by PEP 488. That needs to be resolved.
|
Looks solid overall. |
| *opt_levels* is a tuple containing the available compiler-supported | ||
| optimization levels. See :pep:`488` for more information. | ||
|
|
||
| .. versionadded:: 3.8 |
| @@ -736,6 +736,7 @@ def test_implementation(self): | |||
| self.assertTrue(hasattr(sys.implementation, 'version')) | |||
| self.assertTrue(hasattr(sys.implementation, 'hexversion')) | |||
| self.assertTrue(hasattr(sys.implementation, 'cache_tag')) | |||
| self.assertTrue(hasattr(sys.implementation, 'opt_levels')) | |||
|
|
|||
There was a problem hiding this comment.
Hmm, we should probably also make sure that no other required attrs exist. (We could parameterize this part at the same time.)
required = [attr for attr in dir(sys.implementation) if not attr.startswith('_')]
expected = ['name', 'version', 'hexversion', 'cache_tag', 'opt_levels']
self.assertCountEqual(required, expected)
# Make sure (best effort) that sys.implementation.__dir__() is correct.
for attr in expected:
self.assertTrue(hasattr(sys.implementation, attr))We should also make sure that sys.implementation.opt_levels is a tuple of only non-negative ints and alphanumeric strings.
self.assertIs(type(sys.implementation.opt_levels), tuple, sys.implementation.opt_levels)
for opt in sys.implementation.opt_levels:
if type(opt) is int:
self.assertTrue(opt.isalnum(), opt)
else:
self.assertIs(type(opt), int, opt)
self.assertGreaterEqual(opt, 0)| @@ -2034,63 +2034,44 @@ def _compile(file, optimize=-1): | |||
| return False | |||
| return True | |||
|
|
|||
| def _check_cache(pycache, file): | |||
There was a problem hiding this comment.
Thanks! I think even if this ends up not being in sys.implementation, these changes in zipfile still might be worth adding to simplify the code.
| fname = arcname = file_py | ||
| # Compile py into PEP 3147 pyc file. | ||
| if _compile(file_py): | ||
| fname = pycache_opt[sys.flags.optimize] |
There was a problem hiding this comment.
Shouldn't this catch the IndexError and fall back fname = arcname = file_py?
FWIW, this code implies that sys.flags.optimize is guaranteed to be suitable as a tuple index (but such a guarantee does not exist).
Also, PEP 488 opens that door for dealing with sets of optimizations (either as a container of strings/ints or as a hash of them). This code would fall apart then.
We can discuss this more on the BPO issue.
| # file name in the archive. | ||
| fname = pycache | ||
| arcname = file_pyc | ||
| break |
There was a problem hiding this comment.
This "break" implies that the order of sys.implementation.opt_levels is extremely important. In a world where we deal only in sets of optimizations represented as increasing interger "levels" thats fine, but PEP 488 suggests that's not necessarily where we're headed. We should discuss this more on the BPO issue. :)
If we end up sticking with opt levels as currently used then the importance of order should be made clear in the docs.
https://bugs.python.org/issue23892