Skip to content

Conversation

@nnja
Copy link
Contributor

@nnja nnja commented Feb 2, 2019

This fix correctly sets the spec origin to
"frozen" for the _frozen_importlib module. Note that the
origin was already correctly set in _frozen_importlib_external.

https://bugs.python.org/issue35321

@vstinner
Copy link
Member

vstinner commented Feb 2, 2019

This fix correctly sets the spec origin to
"frozen" for the _frozen_importlib module. Note that the
origin was already correctly set in _frozen_importlib_external.
@nnja nnja force-pushed the fix_35321_frozen_spec branch from 172f816 to eb3dac6 Compare February 2, 2019 20:13
@nnja nnja changed the title bpo-35537: Set the spec origin to frozen in frozen modules bpo-35321: Set the spec origin to frozen in frozen modules Feb 2, 2019
@nnja
Copy link
Contributor Author

nnja commented Feb 2, 2019

@vstinner drat, copied and pasted the wrong issue number. It's actually https://bugs.python.org/issue35321. I updated the commit message, GitHub issue title & message.

@geryogam
Copy link
Contributor

geryogam commented Feb 2, 2019

Thank you @nnja for this PR!

Copy link
Member

@warsaw warsaw left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nnja - LGTM.

@warsaw
Copy link
Member

warsaw commented Feb 5, 2019

I added the backport tags for 3.7 and 3.6, but I'd like to get @ned-deily 's take on that.

@warsaw warsaw merged commit 69091cb into python:master Feb 5, 2019
@bedevere-bot
Copy link

@warsaw: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Thanks @nnja for the PR, and @warsaw for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @nnja and @warsaw, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 69091cb497b2f0fe7e2789b30b43cf78caf9de9b 3.7

@miss-islington
Copy link
Contributor

Sorry, @nnja and @warsaw, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 69091cb497b2f0fe7e2789b30b43cf78caf9de9b 3.6

@ned-deily
Copy link
Member

AFAICT this is not a 3.6 regression: the behavior has been around for a long time. Even though it is minor and the risk of breakage is small, the benefit seems to be also small. My take is that this doesn't meet the criteria for a security-fix-only branch so it should not go into 3.6 or earlier. Sorry!

@warsaw
Copy link
Member

warsaw commented Feb 5, 2019

Thank you @ned-deily - we'll just back port this to 3.7.

@nnja
Copy link
Contributor Author

nnja commented Feb 6, 2019

I have a question about how to backport this into 3.7. (maybe @warsaw or @ned-deily can help?)

I used the cherry_picker tool to try to apply this commit to 3.7. When I looked at the merge conflict
I realized it's in Python/importlib.h which is the generated header file for this frozen module.

As expected, this file has irresolvable conflicts.

What's the process for this type of backport?

@warsaw
Copy link
Member

warsaw commented Feb 6, 2019

I'm not sure this is the right way to do it (does the devguide provide any help?). What I generally do is to use the tool to do the merge in the older branch, then revert Python/importlib.h, then make regen-importlib to get a proper importlib.h. Then that's the branch I propose for merging.

I wonder if @Mariatta has any thoughts on adding this to @miss-islington by default?

@Mariatta
Copy link
Member

Mariatta commented Feb 6, 2019

you can resolve the conflict manually (regenerate the files) and the do cherry_picker --continue.

Currently miss-islington can't do this automatically. There is open ticket here: python/miss-islington#41.

@nnja
Copy link
Contributor Author

nnja commented Feb 7, 2019

I'll try this approach and follow up if I run into any issues. Thank you @Mariatta and @warsaw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants