Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-46712: Let generate_global_objects.py Run on Earlier Python Versions #31637

Merged
merged 3 commits into from
Mar 1, 2022
Merged

bpo-46712: Let generate_global_objects.py Run on Earlier Python Versions #31637

merged 3 commits into from
Mar 1, 2022

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Mar 1, 2022 •
edited
Loading

@ericsnowcurrently
Copy link
Member Author

Hmm, I'm not able to reproduce that failure locally.

@gvanrossum gvanrossum self-requested a review March 1, 2022 19:05
gvanrossum
gvanrossum reviewed Mar 1, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit title doesn't match what I see in the PR -- how come all these extra ids were added? Were they somehow lost in the offending PR (GH-31261)? I don't see how.

@ericsnowcurrently
Copy link
Member Author

The commit title doesn't match what I see in the PR -- how come all these extra ids were added? Were they somehow lost in the offending PR (GH-31261)? I don't see how.

All the added ids are the ones used in the frozen modules. They should have been added in that PR (from running make regen-global-objects). There is a CI check that should have caught this and blocked the PR. I'll have to see why it didn't.

gvanrossum
gvanrossum approved these changes Mar 1, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Perhaps this wasn't caught because I simplified some things in the freeze_modules.py script. (Or if it looks Kumar did so, it was my idea. :-)

@ericsnowcurrently
Copy link
Member Author

...or it may be something else and I was wrong and Kumar's PR is fine. It isn't clear yet. I'm checking.

@gvanrossum
Copy link
Member

Yeah, I compared an old and the new deepfreeze.c output (on Windows) and while there were some differences but not in lines with Py_ID in them. So then the question is where do all these new identifiers come from.

@gvanrossum
Copy link
Member

(Those extra identifiers, like "a", definitely exist in the deep-frozen code. But they don't exist in the hand-picked lists in generate_global_objects.py. Maybe you have some script that automates the hand-picking that would up date generate_global_objects.py, and somehow you ran that script but didn't commit the output? That would be consistent with the test failure in CI.

@ericsnowcurrently
Copy link
Member Author

ericsnowcurrently commented Mar 1, 2022 •
edited
Loading

Yeah, it was my fault. Some changes where I was using Py_ID() in argument clinic (in a different branch) somehow were still around when I ran generate_global_objects.py. make clean took care of it for me.

@ericsnowcurrently
Copy link
Member Author

Sorry for the noise.

@ericsnowcurrently ericsnowcurrently merged commit 21099fc into python:main Mar 1, 2022
@ericsnowcurrently ericsnowcurrently deleted the fix-global-objects-generation branch March 1, 2022 21:29
@ericsnowcurrently ericsnowcurrently mentioned this pull request Mar 31, 2022
Merged
ericsnowcurrently added a commit that referenced this pull request Mar 31, 2022
@ericsnowcurrently
bpo-47146: Stop Depending On regen-deepfreeze For regen-global-objects (
e7bb7c2 
gh-32218)

This effectively reverts the Makefile change in gh-31637. I've added some notes so it is more clear what is going on.

We also update the "Check if generated files are up to date" job to run "make regen-deepfreeze" to ensure "make regen-global-objects" catches deepfreeze.c.

https://bugs.python.org/issue47146
@kumaraditya303 kumaraditya303 mentioned this pull request Apr 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ericsnowcurrently @gvanrossum @the-knights-who-say-ni @bedevere-bot @miss-islington