Use xml.etree.ElementTree instead of deprecated cElementTree #1278

mgorny · 2020-05-27T13:43:24Z

The xml.etree.cElementTree is deprecated, and has been removed in Python
3.9. At the same time, xml.etree.ElementTree has already been using
cElementTree implicitly since Python 3.3. Update test_flatten to use
the latter to provide compatibility with newer Python versions.

Fixes: ticket:9834

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9834
The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
I have updated the automated tests.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

rodrigc · 2020-06-08T23:00:24Z

@mgorny As you are working on your PR's for Twisted, can you run:

tox -e lint

in the top-level directory of your tree before pushing to GitHub?
This will identify some small style/whitespace errors in your PR which show up as errors
in the CI for Twisted.

rodrigc · 2020-06-09T00:40:51Z

@mgorny can you run tox -e lint in the top-level of your tree before pushing to GitHub, and take a pass at fixing any whitespace/style errors reported by the linter?

In your other PR, I fixed a few simple ones here: 686f20f

rodrigc · 2020-06-09T00:41:36Z

src/twisted/internet/base.py

@@ -971,7 +971,7 @@ def argChecker(arg):
            """
            if isinstance(arg, unicode):
                try:
-                    arg = arg.encode(defaultEncoding)


I guess you put this in here for debugging some of the CI failures on Windows?

rodrigc · 2020-06-09T00:42:02Z

src/twisted/web/test/test_flatten.py

@@ -9,7 +9,7 @@
 import sys
 import traceback

-from xml.etree.cElementTree import XML


This part looks OK.

This does look fine, and I wonder why it would provoke these Windows failures.

mgorny · 2020-06-09T07:15:42Z

@mgorny As you are working on your PR's for Twisted, can you run:
tox -e lint
in the top-level directory of your tree before pushing to GitHub?
This will identify some small style/whitespace errors in your PR which show up as errors
in the CI for Twisted.

I did but I don't think it found anything in my changes. I wasn't sure if I'm supposed to make irrelevant style fixes in existing code.

mgorny · 2020-06-09T07:18:00Z

Actually, I don't think there were any reports for files I've modified.

rodrigc · 2020-06-09T07:20:47Z

@mgorny The linter failed on your branch here: https://travis-ci.com/github/twisted/twisted/jobs/341031233

You can confirm by running tox -e lint

mgorny · 2020-06-09T07:22:23Z

@mgorny The linter failed on your branch here: https://travis-ci.com/github/twisted/twisted/jobs/341031233

You can confirm by running tox -e lint

My commit does not touch this file. @hawkowl 's does.

hawkowl · 2020-06-09T07:27:57Z

Yeah, sorry. That was me trying to debug Azure Pipelines breaking on your name so then it goes green :/ feel free to force push it back up to remove the commits.

…

On Tue, 9 Jun 2020, at 17:22, Michał Górny wrote: > @mgorny <https://github.com/mgorny> The linter failed on your branch here: https://travis-ci.com/github/twisted/twisted/jobs/341031233 > You can confirm by running `tox -e lint` My commit does not touch this file. @hawkowl <https://github.com/hawkowl> 's does. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1278 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZHMXEVXKZV5XPVGY56LCLRVXPL3ANCNFSM4NMEX2SA>.

mgorny · 2020-06-09T07:33:57Z

Yeah, sorry. That was me trying to debug Azure Pipelines breaking on your name so then it goes green :/ feel free to force push it back up to remove the commits.

No luck, then?

hawkowl · 2020-06-09T08:10:37Z

I know that Azure Pipelines is passing in Unicode fine, it's just that older Python 3s (before the UTF-8 on Windows PEP) choke on outputting it because the default encoding goes to cp1252 :( So, that means that we might just have to run the Twisted tests on Windows with -X utf8 or so, but then we're running the tests with a "non-standard" version/configuration of Python. But, since the default behaviour breaks for dumb reasons (accepting Unicode environment variables that it can't then output), perhaps running without -X utf8 or equiv on Windows should be deprecated/warned about as defective by design and unsupported (especially since Windows isn't DOS anymore, and all Linux/MacOS tests rely on proper Unicode support). I'll raise this on the ML, since this may have other solutions (such as manually configuring the output in Python space, perhaps) that I haven't thought of. As far as this/other PRs: I think that due to the issue being that Windows is extremely dumb, we can safely ignore the tests that fail on Windows on all your PRs as unrelated. I hope to get to reviewing these soon, just been caught up with starting a new job. Sorry for the hassle, and thanks again for your contributions. 💜

…

On Tue, 9 Jun 2020, at 17:34, Michał Górny wrote: > Yeah, sorry. That was me trying to debug Azure Pipelines breaking on your name so then it goes green :/ feel free to force push it back up to remove the commits. No luck, then? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1278 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZHMXGC73SKCWBI2PHUROTRVXQXJANCNFSM4NMEX2SA>.

glyph

Sadly we're still failing CI on this, and I don't think the tweak to fix this is a good idea. But also, are we just seeing these CI failures on trunk too?

If you could please back out the unrelated stuff, we'll separately try to address the build failure on trunk.

glyph · 2020-06-18T05:46:58Z

src/twisted/test/test_process.py

@@ -483,7 +483,7 @@ class GetEnvironmentDictionary(UtilityProcessProtocol):
    and expose it to interested parties.
    """
    programName = b"twisted.test.process_getenv"
-
+    


This trailing whitespace is probably not necessary :).

glyph · 2020-06-18T06:01:56Z

azure-pipelines/run_test_steps.yml

+      PYTHONUTF8: "1"
+      PYTHONIOENCODING: "utf8:surrogateescape"


It looks like this was an attempt to fix the failing windows tests… which are still failing.

In general I am concerned about adding env vars like this to the test suite, since we might inadvertently introduce features into Twisted where it can't work without custom configuration on Windows, so we should remove this.

glyph · 2020-06-18T06:02:21Z

src/twisted/web/test/test_flatten.py

@@ -9,7 +9,7 @@
 import sys
 import traceback

-from xml.etree.cElementTree import XML


This does look fine, and I wonder why it would provoke these Windows failures.

mgorny · 2020-06-18T06:17:57Z

Ok, reverted to my original change.

The xml.etree.cElementTree is deprecated, and has been removed in Python 3.9. At the same time, xml.etree.ElementTree has already been using cElementTree implicitly since Python 3.3. Update test_flatten to use the latter to provide compatibility with newer Python versions. Fixes: ticket:9834

Also, remove an extraneous comment about Python 2, since the minimum Python version that Twisted supports is Python 3.5

rodrigc

The original change for using xml.etree.ElementTree is OK.

The CI errors have nothing to do with this change.

rodrigc · 2020-06-20T20:29:44Z

@mgorny it looks like the CI errors have to do with the fact that your GitHub ID has non-ASCII characters in its name, because I cannot reproduce the problem on Azure when I submit the same code change from my branch under my GitHub ID.

rodrigc · 2020-06-20T20:30:57Z

@mgorny This problem is actually kind of interesting. :)

I will get your Python 3.9 changes into the tree today, but if you are cool with it, I'd like to collaborate with you a bit and try to fix this Unicode thing on the Twisted side.
We should not block people from submitting fixes to Twisted if they have non-ASCII characters in their name!! This is 2020!

mgorny · 2020-06-20T20:47:38Z

Sure. I presume you'd need me to submit some boilerplate pull request to get my ID on it, correct?

rodrigc · 2020-06-20T22:04:26Z

Fixed in

rodrigc · 2020-06-20T22:17:56Z

@mgorny OK, no rush on this, but whenever you have time, can you do the following:

Take my branch 9858-rodrigc-mgorny-unicode, make a new branch from that.
Make a small but trivial change to some .py file in the Twisted tree. Try to do the minimal change, so as to not break any of the linters (we don't want to have linter failures adding to the confusion).
Make a new PR with your new branch using your mgorny GitHub ID.

I bet you that the CI will fail on Windows for that....

@hawkowl did a really good write-up about the problem of UTF-8 vs. CP1252 here: https://twistedmatrix.com/pipermail/twisted-python/2020-June/065140.html

It took me a while to connect the dots that your GitHub ID was triggering these failures!

rodrigc suggested changes Jun 9, 2020

View reviewed changes

rodrigc reviewed Jun 9, 2020

View reviewed changes

glyph requested changes Jun 18, 2020

View reviewed changes

mgorny force-pushed the 9834-mgorny-elementtree branch from 3b1e2f2 to e030503 Compare June 18, 2020 06:17

rodrigc force-pushed the 9834-mgorny-elementtree branch from e030503 to de862b3 Compare June 20, 2020 18:39

rodrigc mentioned this pull request Jun 20, 2020

Fix Win32UnicodeEnvironmentTests.test_encodableUnicodeEnvironment for Azure pipelines #1302

Merged

Pass the correct os.environ dictionary to GetEnvironmentDictionary()

bbcb813

Also, remove an extraneous comment about Python 2, since the minimum Python version that Twisted supports is Python 3.5

rodrigc force-pushed the 9834-mgorny-elementtree branch from de862b3 to bbcb813 Compare June 20, 2020 19:22

rodrigc mentioned this pull request Jun 20, 2020

Use xml.etree.ElementTree instead of deprecated cElementTree #1303

Merged

rodrigc approved these changes Jun 20, 2020

View reviewed changes

rodrigc closed this Jun 20, 2020

mgorny mentioned this pull request Jun 21, 2020

[DO NOT MERGE] PR for testing windows encoding problems #1305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use xml.etree.ElementTree instead of deprecated cElementTree #1278

Use xml.etree.ElementTree instead of deprecated cElementTree #1278

mgorny commented May 27, 2020 •

edited

Loading

rodrigc commented Jun 8, 2020

rodrigc commented Jun 9, 2020

rodrigc Jun 9, 2020

rodrigc Jun 9, 2020

glyph Jun 18, 2020

mgorny commented Jun 9, 2020

mgorny commented Jun 9, 2020

rodrigc commented Jun 9, 2020 •

edited

Loading

mgorny commented Jun 9, 2020

hawkowl commented Jun 9, 2020 via email

mgorny commented Jun 9, 2020

hawkowl commented Jun 9, 2020 via email

glyph left a comment

glyph Jun 18, 2020

glyph Jun 18, 2020

glyph Jun 18, 2020

mgorny commented Jun 18, 2020

rodrigc left a comment

rodrigc commented Jun 20, 2020

rodrigc commented Jun 20, 2020

mgorny commented Jun 20, 2020

rodrigc commented Jun 20, 2020

rodrigc commented Jun 20, 2020 •

edited

Loading

Use xml.etree.ElementTree instead of deprecated cElementTree #1278

Use xml.etree.ElementTree instead of deprecated cElementTree #1278

Conversation

mgorny commented May 27, 2020 • edited Loading

Contributor Checklist:

rodrigc commented Jun 8, 2020

rodrigc commented Jun 9, 2020

rodrigc Jun 9, 2020

Choose a reason for hiding this comment

rodrigc Jun 9, 2020

Choose a reason for hiding this comment

glyph Jun 18, 2020

Choose a reason for hiding this comment

mgorny commented Jun 9, 2020

mgorny commented Jun 9, 2020

rodrigc commented Jun 9, 2020 • edited Loading

mgorny commented Jun 9, 2020

hawkowl commented Jun 9, 2020 via email

mgorny commented Jun 9, 2020

hawkowl commented Jun 9, 2020 via email

glyph left a comment

Choose a reason for hiding this comment

glyph Jun 18, 2020

Choose a reason for hiding this comment

glyph Jun 18, 2020

Choose a reason for hiding this comment

glyph Jun 18, 2020

Choose a reason for hiding this comment

mgorny commented Jun 18, 2020

rodrigc left a comment

Choose a reason for hiding this comment

rodrigc commented Jun 20, 2020

rodrigc commented Jun 20, 2020

mgorny commented Jun 20, 2020

rodrigc commented Jun 20, 2020

rodrigc commented Jun 20, 2020 • edited Loading

mgorny commented May 27, 2020 •

edited

Loading

rodrigc commented Jun 9, 2020 •

edited

Loading

rodrigc commented Jun 20, 2020 •

edited

Loading