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

continuous integration: resolve docutils installation step build failures #11331

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Apr 15, 2023

Feature or Bugfix

  • Maintenance

Purpose

  • Restore continuous integration test success.

Detail

  • pip's vendored libraries have begun warning about the deprecation of pkg_resources
  • Our build stages intentionally treat warnings as errors by default
  • This change temporarily permits this specific warning deprecation warnings to occur and be reported-but-not-considered-failure.
  • The first commit on this branch will attempt to replicate the problem, and the second commit will attempt to resolve it.

Relates

…en warnings about pkg_resources deprecation are encountered during docutils dependency installation
…en any deprecation is encountered during docutils dependency installation

This uses a more-permissive check than was originally introduced in 671bc0e (ignoring pkg_resources deprecation warnings).

Commits with 671bc0e continue to fail with: DeprecationWarning: Deprecated call to ``pkg_resources.declare_namespace('sphinxcontrib')``
@jayaddison
Copy link
Contributor Author

jayaddison commented Apr 15, 2023

Lowering the severity of the specific pkg_resources is deprecated as an API message was insufficient to allow the (non-development) docutils package install to succeed with warnings-as-errors; the third commit adds further lenience, allowing any deprecation warnings during (again, non-dev-version) docutils package installation to succeed.

@picnixz
Copy link
Member

picnixz commented Apr 16, 2023

Instead of suppressing some warnings as errors, why not:

  • freeze the pip version, or
  • filter the exact DeprecationWarning causing the issue instead of silencing all DeprecationWarning exceptions ? (if possible)

@jayaddison
Copy link
Contributor Author

Thanks @picnixz - reasonable suggestions. Replying inline:

Instead of suppressing some warnings as errors, why not:

* freeze the pip version, or

This doesn't appear to be a bug/problem with pip, it's a message for our information, so I don't think we need to pause upgrades to the pip version we're building with.

* filter the exact DeprecationWarning causing the issue instead of silencing all DeprecationWarning exceptions ? (if possible)

That's worth more consideration, yep. I attempted a precise, single-message filter in 671bc0e, but it turns out that there was a second deprecation warning (also pkg_resources-related):

Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages

Perhaps it would be better to provide filters for both of those messages.

I did briefly consider a message filter on the single-word string pkg_resources, to catch both - but it felt wrong somehow to use a single message filter to attempt to handle two not-directly-related warnings.

…-fail when any deprecation is encountered during docutils dependency installation"

This reverts commit c0c428e.
…everity of all DeprecationWarnings during docutils (non-dev) installation, apply two message filters - one each for the warnings encountered so far
@jayaddison
Copy link
Contributor Author

Ah, an important detail from the documentation explaining why the message-filtering isn't working as-expected as of 09984f4:

In -W and PYTHONWARNINGS, message is a literal string that the start of the warning message must contain (case-insensitively), ignoring any whitespace at the start or end of message.

  • ✔️ pkg_resources is deprecated matches from the start of pkg_resources is deprecated as an API
  • pkg_resources.declare_namespace does not match from the start of Deprecated call to pkg_resources.declare_namespace('sphinxcontrib')

@jfbu
Copy link
Contributor

jfbu commented Apr 16, 2023

@jayaddison trying out at jfbu@b95fc0e (it worked in my local testing).

However I escaped the ` out of copy paste from my usage of PYTHONWARNINGS in console.

Which was bad.

@jfbu
Copy link
Contributor

jfbu commented Apr 16, 2023

@jayaddison Can you cherry-pick jfbu@6bbe3f6 it lets CI pass

…g-message filter-pattern for non-dev docutils installation must match from the start of the warning text

Fixup for commit a49ec48.
@jayaddison
Copy link
Contributor Author

Ah, thanks @jfbu! I was experimenting with something similar. Since yours is proven working, let's go with that :)

@jayaddison
Copy link
Contributor Author

And for completeness to explain why 40e547a didn't work:

The documentation mentions that regular expressions are supported during some warning filtering:

message is a string containing a regular expression that the start of the warning message must match, case-insensitively.

... but that excludes PYTHONWARNINGS environment variables (and I should have noticed that in the text I quoted previously) - there the message filter is evaluated as a literal string only:

In -W and PYTHONWARNINGS, message is a literal string that the start of the warning message must contain (case-insensitively), ignoring any whitespace at the start or end of message.

Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I hesitated about the added comment as we have very few in such files and I am not sure about our policy here about long lines, but well, this does not undergo linting! And it can always be adjusted later.

@jfbu
Copy link
Contributor

jfbu commented Apr 16, 2023

Thanks for contribution, I will merge this now because this helps other contributed PRs not end up receiving unjust and unfair red crosses 😄

@jayaddison
Copy link
Contributor Author

Thanks again @jfbu. About that comment line:

My hope is that we can resolve the ResourceWarning warnings (see #11317), and then to simplify that line and remove the comment at the same time.

@jfbu jfbu merged commit 0fb6716 into sphinx-doc:master Apr 16, 2023
21 checks passed
@jayaddison jayaddison deleted the issue-11330/pip-pkg-resources-ci-warning-fixup branch April 16, 2023 13:05
@jfbu
Copy link
Contributor

jfbu commented Apr 16, 2023

Thanks for the documenting of the rationale behing the comment and other choices. I streamlined the commit message and added hard linebreaks out of habit. Hope ok with you.

(I did not do a great job as I forgot entirely about first one of four items for linewrap and the title...)

@AA-Turner AA-Turner added this to the 6.2.0 milestone Apr 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build failure: DeprecationWarning during docutils dependency install
4 participants