Skip to content

python 3.13 - deprecate cgi module #12296

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

Merged
merged 1 commit into from
Mar 13, 2025
Merged

Conversation

mapellidario
Copy link
Member

@mapellidario mapellidario commented Mar 6, 2025

Related to #12208

Status

  • manual tests
  • unittests
  • external reviews

Description

the module cgi has been considered a "dead battery" and removed from python standard library from python 3.13 [1].

List of the proposed changes:

  • src/python/WMCore/WebTools/RESTApi.py: Removed mentions cgi.FieldStorage. It is not used anywhere else in the code, I do not see how these lines are still relevant.
  • src/python/WMCore/ReqMgr/Web/utils.py: the function quote() does not seem to be used in this file, I removed it, so that i could remove the import of cgi.
  • src/python/WMCore/ReqMgr/Web/tools.py: cgi.escape() has been removed since python 3.8 [2] and they suggest to use html.escape() instead. Since we have proper try/except clauses around cgi.escape(), this would not crash our service but emptying some data. Migrating to html.escape() is straightfoward so i just did it. Apparently, we use the function quote for jinja templates
    kwargs.update(**{"quote":quote})
    So I did not remove it just yet. However, I can not find any reference to that in these jinja templates [3], but maybe I am missing something. Since I would still prefer not to have functionality that we do not use, @vkuznet, do you know or remember if we really-really-really need to revive this tools.py:quote() function? Do you think it's needed in any template? . Or can we remove it in the same way I did for utils.py and forget about html.escape()?

Is it backward compatible (if not, which system it affects?)

yes

Related PRs

none

External dependencies / deployment changes

none


[1] https://docs.python.org/dev/whatsnew/3.13.html#removed-modules-and-apis

[2]

[3] https://github.com/dmwm/WMCore/tree/master/src/html/ReqMgr/jinja_templates

@dmwm-bot
Copy link

dmwm-bot commented Mar 6, 2025

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 tests no longer failing
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 19 warnings and errors that must be fixed
    • 7 warnings
    • 85 comments to review
  • Pycodestyle check: succeeded
    • 169 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/WMCore-PR-Report/447/artifact/artifacts/PullRequestReport.html

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

These changes look good to me, Dario.

I would suggest to remove the other instance of quote(), as I also cannot see it being used. However, I'd rather wait for Valentin's feedback on this.

Copy link
Contributor

@vkuznet vkuznet left a comment

Choose a reason for hiding this comment

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

Dario, the proposed changes seems fine to me, and I'll suggest to remove tools.py:quote() similar as you did in utils.py as it is no longer in use.

@mapellidario
Copy link
Member Author

Dear Valentin and Alan, thanks for the review, I implemented your suggestions and removed tools.py:quote() as well :)

@dmwm-bot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 1 changes in unstable tests
  • Python3 Pylint check: failed
    • 20 warnings and errors that must be fixed
    • 6 warnings
    • 84 comments to review
  • Pycodestyle check: succeeded
    • 167 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/WMCore-PR-Report/473/artifact/artifacts/PullRequestReport.html

@amaltaro amaltaro requested a review from vkuznet March 13, 2025 01:01
@amaltaro
Copy link
Contributor

Thank you, Dario and Valentin.

@amaltaro amaltaro merged commit e82cb98 into dmwm:master Mar 13, 2025
2 of 3 checks passed
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.

4 participants