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

check finder type before passing path #76448

Merged
merged 4 commits into from Dec 10, 2021
Merged

check finder type before passing path #76448

merged 4 commits into from Dec 10, 2021

Conversation

s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Dec 2, 2021

SUMMARY

Attempt to fix #76225 (comment)

ISSUE TYPE
  • Bugfix Pull Request

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.13 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Dec 2, 2021
@s-hertel s-hertel marked this pull request as ready for review December 6, 2021 15:13
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Dec 6, 2021
@s-hertel s-hertel mentioned this pull request Dec 6, 2021
Closed
@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Dec 6, 2021
nitzmahone
nitzmahone approved these changes Dec 6, 2021
View reviewed changes
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Fix looks sane, I've got some ideas for how to test...

@nitzmahone
Copy link
Member

nitzmahone commented Dec 7, 2021 •
edited

Something didn't feel right here- the fix is fine, but looking at current pkg_resources, I think this fix is actually masking a different problem: https://github.com/pypa/setuptools/blob/main/pkg_resources/__init__.py#L2207-L2213

At first glance, this is actually a setuptools bug that was introduced here: pypa/setuptools#1563 - they added support for find_spec, which is allowed to return None, but pkg_resources._handle_ns assumes it'll always return a spec and blindly dots into the result to get a loader (where the old path got the loader directly, which could be None). The AttributeError that's supposed to trigger py2 compatibility (because py2 loaders don't implement find_spec) is inadvertently triggered by dotting into None, so it's falling back to the py2 loader method, which (now that it's patched to work under py3) returns None, allowing the code to work. I think this is probably OK short-term, but might be worth a patch to setuptools, because if we didn't have a loader with py2 fallback methods (as was the original plan), this would just fail.

The setuptools patch is pretty trivial...

@nitzmahone
Copy link
Member

nitzmahone commented Dec 7, 2021 •
edited

Proposed a setuptools patch for this going forward, but we should also keep find_module working under py3.x in the meantime: pypa/setuptools#2918

@nitzmahone
Copy link
Member

FYI @felixfontein re: c.g test failures from the collection loader py3 update

@s-hertel s-hertel force-pushed the fix_collection_loader_FileFinder branch from e6e7235 to 21fd0db Compare December 8, 2021 16:52
@nitzmahone nitzmahone merged commit ed6581e into ansible:devel Dec 10, 2021
@felixfontein
Copy link
Contributor

@s-hertel @nitzmahone thanks for fixing this!

bcoca pushed a commit to bcoca/ansible that referenced this pull request Dec 15, 2021
@s-hertel @bcoca
check finder type before passing path (ansible#76448)
7a776dd 
* check finder type before passing path

ci_complete

* Reduce nesting

* Test find_module does not cause a traceback with Python 3 FileFinder

* Update lib/ansible/utils/collection_loader/_collection_finder.py
@ansible ansible locked and limited conversation to collaborators Dec 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nitzmahone nitzmahone nitzmahone approved these changes

Assignees
No one assigned
Labels
affects_2.13 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@s-hertel @nitzmahone @felixfontein @ansibot