Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ammaraskar commented Jul 4, 2021 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
db3cbe3 to 6b3c6d7CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1a555a1 to 0268bfeCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pablogsal commented Jul 5, 2021
Btw, do we have a test that checks that the flag works? |
ammaraskar commented Jul 5, 2021
Not yet :) |
ammaraskar commented Jul 5, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@pablogsal After making these changes, I am a little worried about the |
pablogsal commented Jul 5, 2021
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 |
ammaraskar commented Jul 5, 2021
Just added some, please take a look.
For example running The last remaining component for this PR is to make |
pablogsal commented Jul 5, 2021
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. |
ammaraskar commented Jul 5, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
pablogsal commented Jul 7, 2021
We need more rebasing :( |
Co-authored-by: Pablo Galindo <Pablogsal@gmail.com> Co-authored-by: Batuhan Taskaya <batuhanosmantaskaya@gmail.com> Co-authored-by: Ammar Askar <ammar@ammaraskar.com>
ammaraskar commented Jul 7, 2021
Rebased! |
bedevere-bot commented Jul 7, 2021
🤖 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. |
ammaraskar commented Jul 7, 2021
buildbot/AMD64 Ubuntu Shared PR has a weird failure... I wonder if we need a |
ammaraskar commented Jul 7, 2021
Nope, no |
ammaraskar commented Jul 7, 2021
@pablogsal all lights are green, anything else for this one? |
pablogsal commented Jul 7, 2021
Nop. Great job! 👌 |
bedevere-bot commented Jul 7, 2021
|
bedevere-bot commented Jul 7, 2021
|
pablogsal commented Jul 7, 2021
@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:( |
isidentical commented Jul 7, 2021
AFAIK we did. Checking the failures now... |
ammaraskar commented Jul 7, 2021
The errors on both are: when running |
ammaraskar commented Jul 7, 2021
The confusing thing is, it only happens with the new tests, existing |
isidentical commented Jul 7, 2021
To reproduce locally; |
isidentical commented Jul 7, 2021
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
|
isidentical commented Jul 7, 2021
I can confirm that setting |
ammaraskar commented Jul 7, 2021
Aah, let's remove |
isidentical commented Jul 7, 2021
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; defget_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 ownifseedisnotNone: env['PYTHONHASHSEED'] =str(seed) else: env.pop('PYTHONHASHSEED', None) out=assert_python_ok( '-c', self.get_hash_command(repr_), **env) stdout=out[1].strip() returnint(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