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

Fix up support for Python 3.12 #1290

Merged
merged 2 commits into from
Sep 1, 2023
Merged

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented May 25, 2023

Changes

  • Enable new tarfile extraction protections in Python 3.12
    • https://docs.python.org/3.12/whatsnew/3.12.html#other-language-changes
    • This applies the new "data" filter to unpacking tar files
    • This prevents the unpacked files from using absolute filepaths or symlinks from referencing absolute filepaths or filepaths outside of the target directory
    • Additionally, the permissions are sanitized to basically ensure they are sane; e.g. world execute permissions are removed if the owner doesn't have execute permissions.

Notes

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16
Copy link
Member Author

rmartin16 commented May 25, 2023

interesting....setuptools is no longer installed in to virtual envs by default...fyi

@rmartin16
Copy link
Member Author

rmartin16 commented May 25, 2023

Test coverage still fails for Python 3.12. My guess is coverage hasn't been updated to account for AST changes.

pyproject.toml Outdated
@@ -51,6 +52,10 @@ multi_line_output = 3
testpaths = ["tests"]
filterwarnings = [
"error",
# Work around https://github.com/pytest-dev/pytest/issues/10977 for Python 3.12
'ignore:(ast\.Str|ast\.NameConstant|ast\.Num|Attribute s) is deprecated and will be removed.*:DeprecationWarning:',
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not do this sort of masking for a pre-release; if we get closer to 3.12 final and it hasn't been fixed, then sure - but our 3.12 support is mostly for "canary" testing at this point (to catch issues like the tarball filter argument). I don't think we need to introduce workarounds for problems that should be fixed before 3.12 final.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's makes sense; I was kinda leery of it since there isn't a good way to track when it can come back out.

For at least the pytest error, I'm expecting them to fix it pretty quickly and have a new release available...not sure about the dateutil one.

I mostly just want GitHub to add proper support for allow-failure so CI doesn't look like it failed because testing failed on a non-critical platform...:(

Copy link
Member Author

Choose a reason for hiding this comment

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

Furthermore, though, these changes won't get full coverage until the pytest DeprecationWarning is fixed.

@rmartin16
Copy link
Member Author

Pytest support for 3.12 is blocked on the 3.12.0b2 release.

@rmartin16
Copy link
Member Author

rmartin16 commented Jun 10, 2023

@rmartin16
Copy link
Member Author

Sooo....traction with PRs in python-dateutil have been lethargic and Irit hasn't moved on python/cpython#105658.

Given that and the looming 3.12 release, I'm inclined to push a commit with configuration to suppress these errors.

Let me know if you'd rather go a different direction. Alternatively, we could merge this now and just include such mitigations with the updates to CI for 3.12.

@freakboy3742
Copy link
Member

Sooo....traction with PRs in python-dateutil have been lethargic and Irit hasn't moved on python/cpython#105658.

Given that and the looming 3.12 release, I'm inclined to push a commit with configuration to suppress these errors.

Let me know if you'd rather go a different direction. Alternatively, we could merge this now and just include such mitigations with the updates to CI for 3.12.

The coverage issue doesn't bother me that much - I'd be happy merging something to ignore that one on the basis that the line is covered (and the overall coverage report supports that), with an open ticket to revert the ignore once the underlying issue is resolved.

The dateutil issue is more of a concern though. Is that actually an error, or just a very loud warning that is being raised as an error by pytest for some reason? It's not clear to me why a DeprecationWarning is presenting as an error. If it is a hard error, I don't think we can ignore it; but if it's an ignorable warning and cookiecutter still works, then I'd be happy with an explicit ignore (again with a ticket to revisit once datetutil is fixed).

@rmartin16
Copy link
Member Author

The dateutil issue is more of a concern though. Is that actually an error, or just a very loud warning that is being raised as an error by pytest for some reason? It's not clear to me why a DeprecationWarning is presenting as an error. If it is a hard error, I don't think we can ignore it; but if it's an ignorable warning and cookiecutter still works, then I'd be happy with an explicit ignore (again with a ticket to revisit once datetutil is fixed).

We updated the pytest configuration a while back to report all warnings as errors.

[tool.pytest.ini_options]
testpaths = ["tests"]
filterwarnings = [
    "error",
]

Nonetheless, the dateutil error is just about datetime.utcfromtimestamp() being deprecating; it won't be an actual problem in 3.12.

Additionally, the transitive dependency is through arrow which currently includes the dependency python-dateutil >= 2.7.0....so, we'll automatically get the update once released.

briefcase==0.3.16.dev133+g77cf5df2.d20230829
├── cookiecutter [required: >=2.2,<3.0, installed: 2.3.0]
│   ├── arrow [required: Any, installed: 1.2.3]
│   │   └── python-dateutil [required: >=2.7.0, installed: 2.8.2]

@freakboy3742
Copy link
Member

The dateutil issue is more of a concern though. Is that actually an error, or just a very loud warning that is being raised as an error by pytest for some reason? It's not clear to me why a DeprecationWarning is presenting as an error. If it is a hard error, I don't think we can ignore it; but if it's an ignorable warning and cookiecutter still works, then I'd be happy with an explicit ignore (again with a ticket to revisit once datetutil is fixed).

We updated the pytest configuration a while back to report all warnings as errors.

[tool.pytest.ini_options]
testpaths = ["tests"]
filterwarnings = [
    "error",
]

Nonetheless, the dateutil error is just about datetime.utcfromtimestamp() being deprecating; it won't be an actual problem in 3.12.

Additionally, the transitive dependency is through arrow which currently includes the dependency python-dateutil >= 2.7.0....so, we'll automatically get the update once released.

briefcase==0.3.16.dev133+g77cf5df2.d20230829
├── cookiecutter [required: >=2.2,<3.0, installed: 2.3.0]
│   ├── arrow [required: Any, installed: 1.2.3]
│   │   └── python-dateutil [required: >=2.7.0, installed: 2.8.2]

Ah - that makes sense. In which case, I'm happy with smothering the warnings for now, with some open housekeeping tickets to clean up the code once the problems have been resolved upstream.

rmartin16 and others added 2 commits August 31, 2023 23:12
- python-dateutil uses the deprecated datetime.utcfromtimestamp(); its
  next release after 2.8.2 will no longer use this function.
- Missing coverage is being falsely reported for an exit from a finally
  block; the coverage exclusion can be removed with python/cpython#105658.
@rmartin16
Copy link
Member Author

image

@rmartin16 rmartin16 marked this pull request as ready for review September 1, 2023 03:56
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

/me sheds at tear at the beauty... :-)

@freakboy3742 freakboy3742 merged commit cc2dae1 into beeware:main Sep 1, 2023
35 checks passed
@rmartin16 rmartin16 deleted the py312-support branch September 1, 2023 04:38
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.

2 participants