-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-43950: Add option to opt-out of PEP-657 #27023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
db3cbe3
to
6b3c6d7
Compare
1a555a1
to
0268bfe
Compare
Btw, do we have a test that checks that the flag works? |
Not yet :) |
@pablogsal After making these changes, I am a little worried about the |
Hmmm, is possible, but I would prefer to explicitly test that the flag + env work in some specific tests using a subprocess and that would only left the |
Just added some, please take a look.
For example running The last remaining component for this PR is to make |
Maybe we should do the logic in the code object constructor and that way we don't need to deal with marshal.c and the compiler. |
Great idea, did this in the common code path in Added tests to make sure de-marshaling with the flag results in
Going back to this, do you know if there is an example of such a test that already exists in the suite? Where should it go? A new |
We need more rebasing :( |
Co-authored-by: Pablo Galindo <[email protected]> Co-authored-by: Batuhan Taskaya <[email protected]> Co-authored-by: Ammar Askar <[email protected]>
Rebased! |
🤖 New build scheduled with the buildbot fleet by @ammaraskar for commit d34f7a9 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
buildbot/AMD64 Ubuntu Shared PR has a weird failure...
I wonder if we need a |
Nope, no |
@pablogsal all lights are green, anything else for this one? |
Nop. Great job! 👌 |
|
|
@ammaraskar @isidentical Hummm, have we bumped the magic number? This buildbot failure seems quite related. I'm in some meetings right now and cannot check but may need to revert:( |
AFAIK we did. Checking the failures now... |
The errors on both are:
when running |
The confusing thing is, it only happens with the new tests, existing |
To reproduce locally;
|
We don't preserve the original environment, so shared builds fail. See here: cpython/Lib/test/support/script_helper.py Lines 117 to 121 in bb3e0c2
|
I can confirm that setting |
Aah, let's remove |
Also see this example usage of how it is used (it creates a new env based on the existing one) if you want to isolate these; def get_hash(self, repr_, seed=None):
env = os.environ.copy()
env['__cleanenv'] = True # signal to assert_python not to do a copy
# of os.environ on its own
if seed is not None:
env['PYTHONHASHSEED'] = str(seed)
else:
env.pop('PYTHONHASHSEED', None)
out = assert_python_ok(
'-c', self.get_hash_command(repr_),
**env)
stdout = out[1].strip()
return int(stdout) |
To be merged after #26958
This adds the opt-out option and disables the compiler's column information and traceback printing.
https://bugs.python.org/issue43950