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

announce (and warn about) end of Python 2.7 support starting in v2.24.x #21387

Merged
merged 11 commits into from
Sep 11, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Sep 10, 2024

Announce that v2.23.x is the last Pants release series to proactively test that it works with Python 2.7.

Also, log a warning when Python 2 is detected in the default interpreter constraints or in an interpreter_constraints field. The warning can be disabled in case the user really knows what they want via the new --no-python-warn-on-python2-usage option.

Copy link
Contributor

@huonw huonw 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 taking this on

@tdyas tdyas force-pushed the warn_on_python2_usage branch from 96e7d3b to f199786 Compare September 10, 2024 01:54
@tdyas tdyas force-pushed the warn_on_python2_usage branch from 441d51a to db26aac Compare September 10, 2024 14:38
@tdyas
Copy link
Contributor Author

tdyas commented Sep 10, 2024

Fixed what should be the final test failure at https://github.com/pantsbuild/pants/actions/runs/10785314963/job/29910366528?pr=21387#step:11:369 which is due to tests being skipped locally intentionally because not all Pythons were present on my system.

Not sure why those tests passed ever because the Address field used for the expected value in the comparisons was a dummy value which does not reflect the addresses of generated targets,.

@tdyas tdyas requested a review from huonw September 10, 2024 15:20
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Looks good except for getting @memoized working properly.

Comment on lines 524 to 525
@memoized
def inner(interpreter_constraints: tuple[str, ...]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the memoized here won't do quite the right thing, as each call of warn_on_python2_usage_in_interpreter_constraints will create a new inner and thus a new batch of memoization.

I think the @memoized needs to be a on a top level function?

Copy link
Contributor Author

@tdyas tdyas Sep 10, 2024

Choose a reason for hiding this comment

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

You're right. The lists being passed in to function in an earlier iteration are not hashable, so my brain was trying an "easy" fix to consolidate turning them into tuples for @memoized to work. Will just reorganize them into two free functions so the @memorized is technically on an outer function.

@tdyas tdyas merged commit d686c95 into pantsbuild:main Sep 11, 2024
25 checks passed
@tdyas tdyas deleted the warn_on_python2_usage branch September 11, 2024 00:31
@huonw huonw mentioned this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants