search

thestr4ng3r / chiaki

Moved to https://git.sr.ht/~thestr4ng3r/chiaki - Free and Open Source PS4 Remote Play Client
https://git.sr.ht/~thestr4ng3r/chiaki
2.18k stars 370 forks source link

appstream fixes #358

Closed fenio closed 3 years ago

fenio commented 3 years ago

Some fixes to make Debian autotests and appstream-cli validate happy. https://appstream.debian.org/sid/main/issues/chiaki.html

thestr4ng3r commented 3 years ago

cc @AsciiWolf

thestr4ng3r commented 3 years ago

should the filename also be made lowercase-only?

fenio commented 3 years ago

Validators don't complain on file name and https://www.freedesktop.org/software/appstream/docs/chap-CollectionData.html doesn't say about such restrictions so I guess it's fine as long as file name is pretty portable between systems which is true with current name.

AsciiWolf commented 3 years ago

@thestr4ng3r Please, revert this PR. It broke the AppStream metadata.

AsciiWolf commented 3 years ago

Regarding the Debian AppStream validator:

The component ID should only contain lowercase letters.

This is weird, but it's just a hint. Anyway, feel free to keep the id lowercase, but please, also change the file name.

The description contains a web URL in plain text. This is not allowed, please use the tag instead to share links.

I am not happy with this. GNOME Software does not support displaying "help" links. The best solution would probably be adding a downstream patch in Debian to change the text url in description to an actual "help" url, but outside of the description section.

thestr4ng3r commented 3 years ago

So eventually I have decided to remove the AppStream file completely since apparently all it does is cause a mess and waste time if not even the tools manage to agree on the format. Sorry, if you need this, you will have to add it in your own packaging scripts.

AsciiWolf commented 3 years ago

Ugh, why? It did not have any issues at all before this PR. Anyway, it is your choice. But be aware that without an AppStream file, most distros will either refuse to package your app or package it, but it won't be available for installation in any GUI package manager.

AsciiWolf commented 3 years ago

@thestr4ng3r By the way, you still have the appdata file in gui/CMakeLists.txt.

thestr4ng3r commented 3 years ago

Thanks, removing this as well.

Ugh, why?

If something is part of this repository then I am responsible for maintaining it. But this single xml file has already caused me more headaches than most of the actual code, despite me not using it for anything at all, so it does not seem worth keeping.

fenio commented 3 years ago

LOL... to be honest it isn't outcome I was expecting ;)

AsciiWolf commented 3 years ago

But this single xml file has already caused me more headaches than most of the actual code

How? The only problem that I remember was when you changed the id to an incorrect one when merging my original PR. I pointed this out and sent a new PR that fixed it. Were there any other issues except that one (and today's PR from @fenio that could easily be solved)? Anyway, you are right that it is your project and you are free to do whatever you like with it. It's just a little bad for many Linux users who rely on GUI package managers to search and install apps and many desktop-focused Linux distributions that won't package Chiaki because of missing AppStream metadata.

thestr4ng3r commented 3 years ago

How? The only problem that I remember was when you changed the id to an incorrect one when merging my original PR. I pointed this out and sent a new PR that fixed it. Were there any other issues except that one (and today's PR from @fenio that could easily be solved)?

Nope, but that is already more than enough for just a metadata file.

bilelmoussaoui commented 3 years ago

Tbh, the issue here is debian's autotests which should stop enforcing rules that no one else enforces.

fenio commented 3 years ago

Well Debian simply uses appstream-util to validate these files. And appstream-util is part of https://github.com/hughsie/appstream-glib which in turn is part of the whole AppStream effort. I really didn't want to cause file removal. I just wanted to make it XML valid and comply with AppStream policy ;)

Kekun commented 3 years ago

@ximion What would you recommend to solve that upper/lower case issue, using appstream directly or as Debian does appstream-glib?

ximion commented 3 years ago

@fenio

Well Debian simply uses appstream-util to validate these files. And appstream-util is part of https://github.com/hughsie/appstream-glib which in turn is part of the whole AppStream effort.

No, appstream-glib is a 3rd-party reimplementation of AppStream. The reference implementation is "appstream" (which does use GLib, which makes this pretty confusing).

@Kekun The upper/lowercase thing exists for naming consistency, so people only need to remember the name of the ID, not also the casing (we had some crazy cases there in the past, due to typos). However this is only a INFO type hint that appstreamcli emits, not a warning or error. So it does not fail validation. So I would recommend to just ignore that hint here. Renaming an AppStream component ID is a pretty heavy change, as it breaks all associations with the app (like reviews & Co.), so it should only be done in exceptional circumstances. In retrospect, making component-IDs case-sensitive was a mistake, but that's nothing we can fix now (we could in AppStream itself, but not in all apps that consume component-IDs).

The next release of AppStream will make the cid-contains-uppercase-letter issue tag a pedantic warning, so it's an even lower-priority hint and not even shown by default.

ximion commented 3 years ago

Mini-Legend for the different tools:

appstream-glib: 3rd-party reimplementation of AppStream, contains the appstream-util tool, used by Flatpak and GNOME Software and by Fedora/OpenSUSE for metadata creation and validation.

appstream: reference implementation of AppStream, uses GLib, contains the appstreamcli tool, used by Elementary AppCenter, KDE Discover and a few other software centers, used by Debian/Ubuntu/Arch Linux/Alpine for metadata generation & validation.

In Debian, a component is only rejected for inclusion into the catalog if appstreamcli emits an ERROR-type warning (that pretty much means the MetaInfo file is completely invalid and can't be processed). It makes quite some noise about WARNING-type warnings, because those mean that the metadata has some serious problem that upstream should address, and any INFO-type warning is just "nice to have" and usually not even shown on the dashboard. A PEDANTIC-type warning is for things that are case-by-case relevant or if one wants to be extra-compliant. Both of the latter classes of hints can be ignored while still having a perfectly valid metadata file.

fenio commented 3 years ago

Ok so I was wrong wrt origin of tools. Most probably I also overreacted but I really didn't think it will turn into this mess. Apparently @AsciiWolf also overreacted a bit. And apparently @thestr4ng3r simply doesn't like when stuff irrelevant for him adds additional work. To be honest I didn't know about AppStream initiative till I started packaging chiaki. I learnt a lot from this thread. I think AppStream project is reasonable and useful and I really didn't want to make all that damage. Honestly I thought I'm fixing problems instead of generating them :)

How about reverting my PR but keep appstream integration as it was before I submitted it?

ximion commented 3 years ago

Sounds reasonable - but you should ideally remove that URL from the description text and place it in an <url/> block of type "help" - because that was a validator warning that you really want to get rid of ;-) => https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-url Also, the component-ID casing hint should have been a pedantic one, not an info one (this is fixed upstream and will take a bit of time to become visible everywhere).

fenio commented 3 years ago

Yeah that was the trigger... but unfortunately when I started digging into all AppStream stuff I decided to fix also minor issues which weren't even at warning level. My bad. Ok maybe not bad cause you have to believe me I wanted to do good thing :)

I'm fine with starting from scratch. Let's revert all changes made as an effect of my PR and I will submit new PR only after discussing it with @AsciiWolf and @ximion so actual PR will just fix issues instead of generating unnecessary noise which @thestr4ng3r isn't fan of.

Can we agree on that? I believe that all of us here are OpenSource supporters so let's don't fight but focus on creating better stuff :)

bilelmoussaoui commented 3 years ago

@fenio Sounds good :) appstream is a very important project as without, linux users can never discover new apps. Thanks for the help improving those :+1: (completely unrelated, but when I started contributing to GNOME, I broke a bunch of app-ids as well, so we all start from somewhere)

fenio commented 3 years ago

@thestr4ng3r I know it's just metadata file and it causes you just headache but would you consider including new file which will be prepared and consulted among us before submitting it? I can promise we will review it twice or trice before bothering you. I really think that chiaki can benefit from having that file. And I feel guilty for its removal.

thestr4ng3r commented 3 years ago

Sorry, I am not interested anymore in having such a file in the repo. That of course does not mean, there can't exist such a file maintained by someone else or in distribution packages. Just if it is a requirement to have it in the upstream repository then I can't help you.

Please be aware that I am not blaming anybody here, I am blaming the ecosystem around this appstream xml stuff. If its goal is to make it easier for maintainers to keep metadata about apps, then I think it failed, at least in the current state.

fenio commented 3 years ago

Ok understood. Will try to include this file separately in Debian package.

AsciiWolf commented 3 years ago

@thestr4ng3r Well, you can say the same about, for example, desktop files. But I respect your opinion.

I will maintain my own copy of AppStream metadata for the Chiaki Flatpak. I also give permission to use the original AppStream file freely to anyone who want to maintain a Chiaki Linux package. Maybe just change the Copyright comment to your name/e-mail and <update_contact> tag content to your e-mail.

fenio commented 3 years ago

@thestr4ng3r you could also say so when I asked for SSL exception but you agreed that it's reasonable. Please consider adding that file to your repo again. I can implement it in Debian package only but I've got impression that whole AppStream project makes much more sense if it's being supported by apps creators and not by distributions. No one asks you for maintaining this file on your own. We will do it. You can unsubscribe from this PR and simply wait for commonly accepted version of it. I promised we will work on this ourselves. The only thing you will have to do is to accept our efforts at the final stage.

ximion commented 3 years ago

@fenio

My bad. Ok maybe not bad cause you have to believe me I wanted to do good thing :)

Nah, for that I can take the blame - even INFO-type issues should be actionable things that you might want to do to get rid of all issue-hints, but having a component-ID change which is just a suggestion of the spec as "INFO" was probably a bit much (there are other component-ID checks that are more severe though, like not being a rDNS or the ID having hyphens).

By the way: I made https://www.freedesktop.org/software/appstream/metainfocreator/#/ a while back to aid application developers who just want their metainfo files quickly without caring too much about the details ;-)

thestr4ng3r commented 3 years ago

@thestr4ng3r Well, you can say the same about, for example, desktop files.

Maybe I should remove that one as well then...

Jokes aside, if you can come up with a pr that all three of you @AsciiWolf @fenio @ximion can approve, I think we can bring it back.