-
Notifications
You must be signed in to change notification settings - Fork 338
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
Add SplitArtifacts=metainfo #3542
base: main
Are you sure you want to change the base?
Conversation
487b856
to
cb31259
Compare
mkosi/__init__.py
Outdated
if home_url: | ||
url = urlparse(home_url) | ||
if not url.netloc: | ||
home_url = None | ||
if context.config.appstream_id: | ||
id = context.config.appstream_id | ||
elif home_url: | ||
netloc = url.netloc.split(".") | ||
netloc.reverse() | ||
netloc.remove("www") | ||
id = ".".join(netloc) | ||
id += f".{osid}" | ||
else: | ||
id = osid |
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.
This handles home_url
in a single branch and hopefully also addresses @septatrix comment.
if home_url: | |
url = urlparse(home_url) | |
if not url.netloc: | |
home_url = None | |
if context.config.appstream_id: | |
id = context.config.appstream_id | |
elif home_url: | |
netloc = url.netloc.split(".") | |
netloc.reverse() | |
netloc.remove("www") | |
id = ".".join(netloc) | |
id += f".{osid}" | |
else: | |
id = osid | |
if context.config.appstream_id: | |
id = context.config.appstream_id | |
elif home_url: | |
url = urlparse(home_url) | |
if url.netloc: | |
netloc = url.netloc.removeprefix("www.").split(".") | |
id = ".".join(reversed(netloc)) + f".{osid}" | |
else: | |
home_url = None | |
id = osid | |
else: | |
id = osid |
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.
it's the same result, doesn't make any difference
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 having the logic for home_url
in a single place is cleaner and removing the leading www.
with removeprefix
is shorter, and I'd argue cleaner, with the added check whether www
is actually in there, that is needed by remove
.
That being said, why do you want to strip the www
in the first place? Above you said it's not a valid tld or domain and that is true, but neither is the fallback linux
and in the reverse url scheme, it would end up at the end, just before osid
, anyway. Also, this is only a fallback path, people should set the appstream id in the first place (maybe using specifiers), and singling out www
seems a bit odd.
I generally agree that it wouldn't make sense to have www
in an appstream ID, but neither would other subdomains like download
, as when I grab current systemd from OBS. Can we do without stripping www
or what is the logic here that I don't see?
@septatrix list.remove
only removes the first occurence, so this also behaves the same. I misremembered this as well.
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.
Can we do without stripping
www
or what is the logic here that I don't see?
This is from the spec:
This means the reversed-DNS HOME_URL value from /etc/os-release (without any https/www parts) combined with the ID of the operating system will produce the operating system's component id.
@septatrix
list.remove
only removes the first occurence, so this also behaves the same. I misremembered this as well
No they do not behave the same.
It removes the first occurrence, not the occurrence iff it is at index 0. Therefore a "www" subdomain somewhere in the middle of a URL would also be stripped which I am pretty sure is not how the spec is supposed to be interpreted (because then it would likely also mean removing any "www" component, not only the first occurrence.
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.
Thanks @septatrix, I really shouldn't look at this before having coffee :)
67131e4
to
a171192
Compare
GNOME Software parses .metainfo.xml files to provide nice metadata for artifacts it downloads/updates. Follow the spec and add parameter for it, filling the content from os-release. https://www.freedesktop.org/software/appstream/docs/sect-Metadata-OS.html
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.
LGTM apart from the www stuff.
if home_url: | ||
url = urlparse(home_url) | ||
if not url.netloc: | ||
home_url = None | ||
if context.config.appstream_id: | ||
id = context.config.appstream_id | ||
elif home_url: | ||
url = urlparse(home_url) | ||
netloc = url.netloc.split(".") | ||
netloc.reverse() | ||
if "www" in netloc: | ||
netloc.remove("www") | ||
id = ".".join(netloc) | ||
id += f".{osid}" | ||
else: | ||
id = osid |
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.
Let's shorten it down to this
if home_url: | |
url = urlparse(home_url) | |
if not url.netloc: | |
home_url = None | |
if context.config.appstream_id: | |
id = context.config.appstream_id | |
elif home_url: | |
url = urlparse(home_url) | |
netloc = url.netloc.split(".") | |
netloc.reverse() | |
if "www" in netloc: | |
netloc.remove("www") | |
id = ".".join(netloc) | |
id += f".{osid}" | |
else: | |
id = osid | |
id = osid | |
if context.config.appstream_id: | |
id = context.config.appstream_id | |
elif home_url: | |
url = urllib.parse.urlparse(home_url) | |
if url.netloc: | |
netloc = url.netloc.removeprefix("www").split(".") | |
id = f"{'.'.join(reversed(netloc))}.{osid}" | |
else: | |
home_url = None |
It's clearer and looking at the spec the intention seems to be the strip a leading www
but not the first. The alternative interpretation would be to remove any www
, but that doesn't seem reasonable to me.
metainfo = '<?xml version="1.0" encoding="UTF-8"?>\n' | ||
metainfo += '<component type="operating-system">\n' | ||
metainfo += f" <id>{id}</id>\n" | ||
metainfo += f" <name>{name}</name>\n" | ||
metainfo += f" <summary>{osid} image built with mkosi</summary>\n" | ||
metainfo += f" <description><p>{description}</p></description>\n" | ||
if home_url: | ||
metainfo += f' <url type="homepage">{home_url}</url>\n' | ||
if icon: | ||
metainfo += f' <icon type="remote">{icon}</icon>\n' | ||
metainfo += " <metadata_license>FSFAP</metadata_license>\n" | ||
metainfo += " <releases>\n" | ||
metainfo += ( | ||
f' <release version="{context.config.image_version}" date="{timestamp}" type="development">\n' | ||
) | ||
metainfo += " <description></description>\n" | ||
metainfo += " </release>\n" | ||
metainfo += " </releases>\n" | ||
metainfo += ' <bundle type="systemd" class="sysupdate"></bundle>\n' | ||
metainfo += "</component>\n" |
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 still think that's pretty much an eyesore, but well, it's XML. Here's one last attempt to make this readable without sigil soup around it
metainfo = '<?xml version="1.0" encoding="UTF-8"?>\n' | |
metainfo += '<component type="operating-system">\n' | |
metainfo += f" <id>{id}</id>\n" | |
metainfo += f" <name>{name}</name>\n" | |
metainfo += f" <summary>{osid} image built with mkosi</summary>\n" | |
metainfo += f" <description><p>{description}</p></description>\n" | |
if home_url: | |
metainfo += f' <url type="homepage">{home_url}</url>\n' | |
if icon: | |
metainfo += f' <icon type="remote">{icon}</icon>\n' | |
metainfo += " <metadata_license>FSFAP</metadata_license>\n" | |
metainfo += " <releases>\n" | |
metainfo += ( | |
f' <release version="{context.config.image_version}" date="{timestamp}" type="development">\n' | |
) | |
metainfo += " <description></description>\n" | |
metainfo += " </release>\n" | |
metainfo += " </releases>\n" | |
metainfo += ' <bundle type="systemd" class="sysupdate"></bundle>\n' | |
metainfo += "</component>\n" | |
metainfo = textwrap.dedent( | |
f"""\ | |
<?xml version="1.0" encoding="UTF-8"?> | |
<component type="operating-system"> | |
<id>{id}</id> | |
<name>{name}</name> | |
<summary>{osid} image built with mkosi</summary> | |
<description><p>{description}</p></description> | |
{f'<url type="homepage">{home_url}</url>' if home_url else ""} | |
{f'<icon type="remote">{icon}</icon>' if icon else ""} | |
<metadata_license>FSFAP</metadata_license> | |
<releases> | |
<release version="{context.config.image_version}" date="{timestamp}" type="development"> | |
<description></description> | |
</release> | |
</releases> | |
<bundle type="systemd" class="sysupdate"></bundle> | |
</component> | |
""" | |
) |
The only downside is, that the optional things home_url
and icon
become empty lines when unset. I think that's not unreasonable, since in XML it's syntactically insignificant whitespace.
GNOME Software parses .metainfo.xml files to provide nice metadata for artifacts it downloads/updates. Follow the spec and add parameter for it, filling the content from os-release.
https://www.freedesktop.org/software/appstream/docs/sect-Metadata-OS.html