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

Fix complex __import__ statements failing to parse with Python 2.7 - 3.7 #14232

Merged
merged 2 commits into from Jan 21, 2022
Merged

Fix complex __import__ statements failing to parse with Python 2.7 - 3.7 #14232

merged 2 commits into from Jan 21, 2022

Conversation

Eric-Arellano
Copy link
Contributor

@TansyArron found that this line causes Pants to crash:

__import__("{}.{}".format(module_prefix, modname), fromlist="dummy")

This is because of buggy conditional logic. Oops!

[ci skip-rust]
[ci skip-build-wheels]

@Eric-Arellano
Fix complex __import__ statements failing to parse with Python 2.7 …
b862d48 
…- 3.7

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jan 21, 2022
@Eric-Arellano
Fix complex __import__ statements failing to parse with Python 2.7 …
bd61f03 
…- 3.7 (Cherry-pick of pantsbuild#14232)

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano mentioned this pull request Jan 21, 2022
Merged
thejcannon
thejcannon approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

Not blocking, but shouldn't this be duplicated with the Python3 cases as well?

Also seems like most of th tests should be run with Py3 and Py2, and then only specific cases be run with one-or-the-other.

@Eric-Arellano
Copy link
Contributor Author

but shouldn't this be duplicated with the Python3 cases as well?

Duplicate the tests, you mean? Yeah, I suppose

@Eric-Arellano
Copy link
Contributor Author

Duplicate the tests, you mean? Yeah, I suppose

Looks like we only have special tests for Py27, 38, and 39. So nothing to add to.

@thejcannon
Copy link
Member

Sorry meant duplicated in test_dunder_import_call

@Eric-Arellano
Fix test
652df33 
I can't run Py27 on my computer, so was hoping it would Just Work

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano enabled auto-merge (squash) January 21, 2022 23:15
Eric-Arellano added a commit that referenced this pull request Jan 21, 2022
@Eric-Arellano
Fix complex __import__ statements failing to parse with Python 2.7 …
855322a 
…- 3.7 (Cherry-pick of #14232) (#14233)

See #14232.

[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 523bdb2 into pantsbuild:main Jan 21, 2022
@Eric-Arellano Eric-Arellano deleted the fix-py2-dep-inference branch January 21, 2022 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjyw benjyw benjyw approved these changes

@TansyArron TansyArron TansyArron approved these changes

@thejcannon thejcannon thejcannon approved these changes

@stuhood stuhood Awaiting requested review from stuhood

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@Eric-Arellano @thejcannon @benjyw @TansyArron