-
Notifications
You must be signed in to change notification settings - Fork 123
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 non-utf8 when found in uploaded RPMs. #1028
Conversation
@@ -44,7 +44,9 @@ def get_package_xml(pkg_path, sumtype=util.TYPE_SHA256): | |||
return {} | |||
# RHEL6 createrepo throws a ValueError if _cachedir is not set | |||
po._cachedir = None | |||
primary_xml_snippet = change_location_tag(po.xml_dump_primary_metadata(), pkg_path) | |||
primary_xml_snippet = po.xml_dump_primary_metadata() | |||
primary_xml_snippet = primary_xml_snippet.decode('utf-8', 'replace') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that you moved this to be sooner in the workflow than where string_to_unicode
was being called. What's the reasoning for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I read your comment on the PR which explains this.
end_portion = string_to_unicode(primary_xml_snippet[end_index:]) | ||
location = string_to_unicode("""<location href="%s"/>""" % ( | ||
file_utils.make_packages_relative_path(relpath))) | ||
first_portion = primary_xml_snippet[:start_index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the doc block above needs to be changed to reflect that it takes unicode
in and returns unicode
back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense.
@@ -44,7 +44,9 @@ def get_package_xml(pkg_path, sumtype=util.TYPE_SHA256): | |||
return {} | |||
# RHEL6 createrepo throws a ValueError if _cachedir is not set | |||
po._cachedir = None | |||
primary_xml_snippet = change_location_tag(po.xml_dump_primary_metadata(), pkg_path) | |||
primary_xml_snippet = po.xml_dump_primary_metadata() | |||
primary_xml_snippet = primary_xml_snippet.decode('utf-8', 'replace') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I read your comment on the PR which explains this.
<checksum type="sha256" pkgid="YES">b88a43acd5c9239</checksum> | ||
<summary>rabbit IDE</summary> | ||
<description>The rabbit (professional) IDE.</description> | ||
<packager>Frosty \u2603\x9a</packager> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
https://pulp.plan.io/issues/1903
Seemed to make more sense to decode entire primary XML fragment instead of doing it element-by-element. This approach is simpler and more comprehensive.