bpo-23892: Introduce sys.implementation.opt_levels#9826
Conversation
| 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. |
ericsnowcurrently
left a comment
There was a problem hiding this comment.
As I noted on the BPO issue, I think there are problems with the naive approach I suggested. We should discuss more there.
Also, I'd expect changes to Lib/importlib/_bootstrap_external.py...
Also, didn't zipimport get rewritten recently?
| *opt_levels* is a tuple containing the available compiler-supported | ||
| optimization levels. See :pep:`488` for more information. | ||
|
|
||
| .. versionadded:: 3.8 |
| 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)| 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