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.

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

backport of #32305 causes regressions in various packages #77053

Closed
doko42 opened this issue Feb 19, 2018 · 14 comments
Closed

backport of #32305 causes regressions in various packages #77053

doko42 opened this issue Feb 19, 2018 · 14 comments
Labels
stdlib Python modules in the Lib dir

Comments

@doko42
Copy link
Member

doko42 commented Feb 19, 2018

BPO 32872
Nosy @warsaw, @brettcannon, @doko42, @ncoghlan, @ericvsmith, @ned-deily
PRs
  • bpo-32872 - Revert "[3.6] bpo-32303 - Consistency fixes for namespace loaders... #5911
  • bpo-32872: Avoid regrtest compatibility issue with namespace packages. #6276
  • [3.7] bpo-32872: Avoid regrtest compatibility issue with namespace packages. (GH-6276) #6277
  • [3.6] bpo-32872: Avoid regrtest compatibility issue with namespace packages. (GH-6276) #6278
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2018-03-28.06:46:54.187>
created_at = <Date 2018-02-19.09:03:48.355>
labels = ['library']
title = 'backport of python/cpython#76486 causes regressions in various packages'
updated_at = <Date 2018-03-28.18:27:17.596>
user = 'https://github.com/doko42'

    bugs.python.org fields:

    activity = <Date 2018-03-28.18:27:17.596>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2018-03-28.06:46:54.187>
closer = 'ned.deily'
components = ['Library (Lib)']
creation = <Date 2018-02-19.09:03:48.355>
creator = 'doko'
dependencies = []
files = []
hgrepos = []
issue_num = 32872
keywords = ['patch', '3.6regression']
message_count = 14.0
messages = ['312344', '312351', '312358', '312366', '312397', '312839', '312844', '312845', '314131', '314132', '314564', '314565', '314566', '314617']
nosy_count = 6.0
nosy_names = ['barry', 'brett.cannon', 'doko', 'ncoghlan', 'eric.smith', 'ned.deily']
pr_nums = ['5911', '6276', '6277', '6278']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue32872'
versions = ['Python 3.6']
    @doko42
    Copy link
    Member Author

    doko42 commented Feb 19, 2018

    The backport of issue bpo-32305 causes regressions in several packaged namespace packages:

    https://bugs.debian.org/890621
    https://bugs.debian.org/890754

    while the change is intended, is it appropriate to backport it to 3.6? Please could you have a look, you might still have an appropriate chroot laying around ;)

    @doko42 doko42 added type-crash A hard crash of the interpreter, possibly with a core dump stdlib Python modules in the Lib dir labels Feb 19, 2018
    @warsaw
    Copy link
    Member

    warsaw commented Feb 19, 2018

    Both of those upstreams should be using if getattr(module, '__file__', None) instead. The old behavior was an undocumented quirk.

    @warsaw warsaw closed this as completed Feb 19, 2018
    @ned-deily
    Copy link
    Member

    Now that we know that this change *does* break some existing code, I think it is worth having that talk as mentioned in PR 5481:

    "I suppose it's possible that this will break existing code, but I'd argue that because current behavior runs counter to the documentation and makes no sense given the inconsistencies, it is better to fix them. I propose this change be applied to 3.7 and 3.6, although if you, my friendly reviewer, disagrees about 3.6, we can talk about it!"

    The question I have is: is the problem the backport is trying to fix severe enough to cause package regressions in the middle of a maintenance release cycle? Sure, the third-party packages should be fixed and such a change is fine for 3.7 but we also kinda promise that installing a maintenance release should be painless. I don't know what is the right answer and I don't want to spend a lot of time on this but I'd like to get a bit more input on this before we go ahead with releasing it in 3.6.5.

    @ned-deily ned-deily reopened this Feb 19, 2018
    @brettcannon
    Copy link
    Member

    I'm personally fine if the change gets reverted. I don't think the odd behaviour is severe enough to justify breaking projects in a maintenance release. Besides, they will learn soon enough about their code breaking in Python 3.7.

    @ncoghlan
    Copy link
    Contributor

    +1 from me for making the change 3.7.0+ only - 3.6 isn't doing the right thing, but given folks are relying on it doing the wrong thing, then let's leave it alone given where it is in its lifecycle.

    @ned-deily
    Copy link
    Member

    OK, I agree with Brett and Nick. Barry, are you OK with reverting this change for 3.6? If so, can you do the honors?

    @warsaw
    Copy link
    Member

    warsaw commented Feb 25, 2018

    I'd personally prefer to keep the fix (I ran into some problems w/3.6), but I'll defer to the RM. I'll revert the change for 3.6, but I want to test it with importlib_resources first, since I'll probably have to spin a new release of that package too.

    @ned-deily
    Copy link
    Member

    Thanks, Barry! I don't see that either action is ideal but I am concerned about breaking third-party packages and 2 (already known) breakages is worrisome. And thanks, @Doko, for bringing the matter up.

    @warsaw warsaw closed this as completed Feb 26, 2018
    @doko42
    Copy link
    Member Author

    doko42 commented Mar 20, 2018

    reopening.

    Lib/test/libregrtest/setup.py still needs fixing at least on 3.6, I didn't check the trunk.

    libregrtest/setup.py:

    60c60
    < if hasattr(module, '__file__'):
    ---

        if getattr(module, '\_\_file__', None):

    @doko42 doko42 reopened this Mar 20, 2018
    @doko42
    Copy link
    Member Author

    doko42 commented Mar 20, 2018

    reopening.

    Lib/test/libregrtest/setup.py still needs fixing at least on 3.6, I didn't check the trunk.

    libregrtest/setup.py:

    60c60
    < if hasattr(module, '__file__'):
    ---

        if getattr(module, '\_\_file__', None):

    @ned-deily
    Copy link
    Member

    New changeset e52ac04 by Ned Deily in branch 'master':
    bpo-32872: Avoid regrtest compatibility issue with namespace packages. (GH-6276)
    e52ac04

    @ned-deily
    Copy link
    Member

    New changeset d2d5bd8 by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-32872: Avoid regrtest compatibility issue with namespace packages. (GH-6276) (bpo-6277)
    d2d5bd8

    @ned-deily
    Copy link
    Member

    New changeset a93662c by Ned Deily (Miss Islington (bot)) in branch '3.6':
    bpo-32872: Avoid regrtest compatibility issue with namespace packages. (GH-6276) (GH-6278)
    a93662c

    @ned-deily ned-deily removed release-blocker type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 28, 2018
    @ned-deily
    Copy link
    Member

    New changeset 7f554c5 by Ned Deily (Miss Islington (bot)) in branch '3.6':
    bpo-32872: Avoid regrtest compatibility issue with namespace packages. (GH-6276) (GH-6278)
    7f554c5

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @brettcannon @warsaw @ncoghlan @ned-deily @doko42