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.

Sign up for GitHub

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

Jump to bottom

gh-93018: Fix for the compatibility problems with expat #93900

Merged
merged 2 commits into from
Dec 7, 2022
Merged

gh-93018: Fix for the compatibility problems with expat #93900

merged 2 commits into from
Dec 7, 2022

Conversation

mcepl
Copy link
Contributor

@mcepl mcepl commented Jun 16, 2022 •
edited by bedevere-bot
Loading

This is another take on #93022, fixes #93018.

  1. I believe it is inappropriate to assert on the error message for the external API. Error messages are not part of the provided contract with its users.
  2. I don't think we really care what exact exception is thrown, it is much more important that non-well-formed XML is discovered.
  3. Changing tests based on the version of library seems as a code smell to me, but that's a personal opinion.

Cc: @corona10 @hartwork

@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Jun 16, 2022
@mcepl mcepl changed the title Fix for the compatibility problems with expat gh-93022: Fix for the compatibility problems with expat Jun 16, 2022
@mcepl mcepl changed the title gh-93022: Fix for the compatibility problems with expat gh-93018: Fix for the compatibility problems with expat Jun 16, 2022
@mcepl
gh-93018: Fix for the compatibility problems with expat
8b3441f 
This is another take on #93022, fixes #93018.

1. I believe it is inappropriate to assert on the error message for the
   external API. Error messages are not part of the provided contract
   with its users.
2. I don't think we are really care what exact exception is thrown, it
   is much more important that non-well-formed XML is discovered.
3. Changing tests based on the version of library seems as a code smell
   to me, but that's personal opinion.
hartwork
hartwork reviewed Jun 16, 2022
View reviewed changes
Copy link
Contributor

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

Hi! I'm unsure of this direction at the moment, I'll leave to you to weigh the pros and cons of this approach.

@kraptor
Copy link

kraptor commented Jun 17, 2022

Hi! I'm unsure of this direction at the moment, I'll leave to you to weigh the pros and cons of this approach.

Hi @hartwork,

I think @mcepl is a good approach (from a linux distribution maintainer).

Note that a linux distro may backport patches (for dependencies like expat security issues are a must). In this case, for example, an older version of expat may have the CVE issue backported that renders this specific python test "broken", therefore testing against the expat version makes no sense.

hartwork
hartwork reviewed Aug 12, 2022
View reviewed changes
Copy link
Contributor

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

I understand that test failures are not nice, no matter what or who is causing them. So if it's tolerable that this PR makes the test suite slightly more forgiving and in a way less precise, then it's probably a good direction with regard to ridding the misleading test failures. Please see two comments on details below. Sorry I didn't find time to reply earlier.

@mcepl
Copy link
Contributor Author

mcepl commented Dec 5, 2022

@hartwork Can we move on here, please?

@hartwork
Copy link
Contributor

hartwork commented Dec 5, 2022

@hartwork Can we move on here, please?

Hi @mcepl, I don't have any merge or push permissions here, so there's not much I can do beyond my approval of August 13th above. Maybe @corona10 can help?

@corona10 corona10 self-assigned this Dec 7, 2022
corona10
corona10 approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10 corona10 merged commit 7031275 into python:main Dec 7, 2022
@mcepl mcepl deleted the expat_compatibility branch December 7, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hartwork hartwork hartwork approved these changes

@corona10 corona10 corona10 approved these changes

Assignees

@corona10 corona10

Labels
tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The compatibility problems start with expat 2.4.4, not just 2.4.5
5 participants
@mcepl @kraptor @hartwork @corona10 @bedevere-bot