review process for spec changes

[ramcq]

Hi @ximion - as always - thank you for all you do for open source! Please don't take anything herein as a criticism, I am just exploring ways we could work more closely together in the FLOSS app metadata space to improve user outcomes.

Some changes to Appstream specifications (thinking most recently of moving from developer_name to developer → name) can cause significant disruption to workflows and tooling, and as a specification which is both written and parsed in many places, some very hard to update, changes to the specification structure - particularly non-additive - need to be made carefully with an eye to impact on tooling and the user experience - perhaps bearing Postel's principle in mind!

It so happens this change triggered a bug in Flatpak's appstream parsing, but it's one which some users (thinking of Flatpak users on LTS distros) might continue to experience for multiple years. It's possible that this change of disambiguating developers with a structured machine-readable IDs could've been achieved in a different way that would've been less disruptive to existing tooling/workflows, and more eyes on the change at review phase might've brought us to a better outcome for users.

I believe in the Wayland community that there is an informal consensus process the involves the larger implementers (KDE, GNOME and the wlroots/tiling gang) acking spec changes before they are accepted as core protocols. Would you be amenable to considering a similar process where we look for 2-3 acks (eg KDE Discover, GNOME Software and Flathub?) for changes to the structure of the XML specification?

If this approach is of interest, I'm not sure the best way to invoke a group of "spec reviewers" but maybe GitHub would allow us to define a group of spec reviewers? I'd be happy to offer my services; thinking perhaps @razzeee @barthalion @aleixpol @Pointedstick @wjt @TingPing @hfiguiere might also be worth considering.

Thanks,
Rob

[ximion]

Please don't take anything herein as a criticism, I am just exploring ways we could work more closely together in the FLOSS app metadata space to improve user outcomes.

No worries, I appreciate your feedback and this is actually something I have already been working on to some extent :-)

However, I do disagree with your example of the new developer tag being such a disruptive change based on multiple reasons:

• The tag was introduced in the 1.0 AppStream release which does contain breaking-ish changes and was communicated in advance via a mailing list post, a blog post, as well as a large notice in the NEWS file. It was impossible to miss that 1.0 will contain API breaks as well as specification changes that might be difficult (since a few things that have been deprecated for more than 6 years have been removed from the code and spec).
• The new tag was backported to the 0.16.x stable series, so even if you could not switch to AppStream 1.0 you could still read it. In fact, great care has been put into making the 0.16.x series an easy point from which to jump to 1.0 by adding most of 1.0's features there, with the actually 1.0 release just removing pieces which were previously deprecated.
• AppStream 1.0 did not stop to parse developer_name, it has been deprecated but not removed. Therefore, if you do not want to use the new tag, you are free to do so and will only get an info type hint if you validate your MetaInfo file, asking you to migrate to the new tag. (lib)AppStream's parser absolutely adheres to Postel's principle, we parse a lot of stuff in an extremely lenient way to break as little stuff as possible. The validator is where all the strictness is instead.
• The Flatpak bug is a perfect illustration why I strongly discourage people from trying to reimplement AppStream and write their own parser - there are a few subtleties to pay attention to, and bugs happen (not to mention people having to implement the YAML bits too!). Having them happen in one place to centrally fix is better. Also, it enables us in AppStream to add compatibility code so frontends can parse many versions of the specification without noticing that fact, while an independent implementation will have to catch up with the spec. This Flatpak bug would have also triggered with any MetaInfo file that has an agreement section or similar, and I am amazed that it hasn't been triggered before. Maybe I should submit a patch to make it use AppStream's parser instead, so this won't happen again. It's unfortunate, but I really do blame this on Flatpak, independent parsers are completely out of our control and there is nothing we can say in the spec that makes them magically compliant - not as long as people want to extend the spec. And "there is a bug in software X so we can not make this change" is a fairly poor reason to not make it. If anything, it is a reason to fix said software and maybe delay the change, but usually those bugs only show up after the fact.

To be clear, I'm not saying that we should not discuss this - but in case of the developer tag, I actually believe the way it was added was adequate.

[...] and more eyes on the change at review phase might've brought us to a better outcome for users.

From experience, this does not happen. We were cooking display_length for a year in various places, and it was excruciatingly hard to get any reviewers. Then when the change was merged, a few issues were noted and we had to make adjustments. Same for splitting release information out, new URL tags, screenshot environments and many, many more. The one notable exception was likely the branding tag, and I think it was Elementary developers who provided a ton of useful feedback while it was in its review phase. But that was the exception.

I believe in the Wayland community that there is an informal consensus process the involves the larger implementers (KDE, GNOME and the wlroots/tiling gang) acking spec changes before they are accepted as core protocols. Would you be amenable to considering a similar process where we look for 2-3 acks (eg KDE Discover, GNOME Software and Flathub?) for changes to the structure of the XML specification?

Absolutely not for this specific approach. I had my run into the Wayland process, and I think it is atrocious, especially since it allows a small group to essentially block any change they do not like. It is also not an informal agreement, it is very much formalized via https://gitlab.freedesktop.org/wayland/wayland-protocols/-/blob/main/GOVERNANCE.md

Also, with Wayland you can have multiple (versioned!) protocols that each desktop environment then has to implement, while with AppStream there is one specification and one primary implementation - anything that goes in will have to be implemented by me and also maintained by me for basically forever (removing stuff is near impossible, the only reason we could drop anything with 1.0 at all was the fact that we knew for certain that certain things were no longer used by maintained software).

I also can not force people from KDE and GNOME and others to review changes - usually once there is a new proposed feature, people show up if they are interested, or don't if they aren't. There are also some important cases here to consider: What if for example Budgie or Elementary comes with a request for a new tag that they need but KDE and GNOME do not need? The latter wouldn't ACK it (and also wouldn't NACK it, presumably), so the change would stay in limbo forever, leading to unhappy people.
Then again, if I let in all the changes that do not have objections, it will lead to a very unhappy me, as I would have to maintain a lot of stuff forever for others and the spec would get progressively more cluttered.

Furthermore, AppStream has a lot more interest than from just the Linux desktops - I got requests from Automotive people who do weird things with it, as well as people from the scientific community who wanted it to include citation information as well. I do not think even with a "review board" we could address these issues adequately.

I'd be happy to offer my services

You certainly know the magic words that I like to hear ;-)

It is not like AppStream does not have any policy at all for changes done to it. In the past, the process was basically me and Richard Hughes debating changes until we were both happy with the result. Then it became us two and a bunch of people debating the changes, with me theoretically being able to make the ultimate maintainer decision, but in practice only ever doing it when Richard was also happy (I can't recall a single instance where an objected-to change was merged).

Currently, the "process" is that any minor stuff just goes in straight away, but major changes are reviewed by whoever wants to participate, and as soon as the discussion has reached an endpoint, I let the change sit for a while and then merge it once there are no objections. Sometimes the objections are from me though, to hold off on things for maintenance reasons (like in #497 due to bad experiences with a similar change in the past, not because the proposal was bad).

I mentioned at the very beginning of this long reply that this issue was also on my mind, but it had nothing to do with the developer element (I hadn't even seen that Flatpak issue...), but had everything to do with the validator change in #604 where we decided that the change would be fine and I didn't run my usual tests, which gave KDE's CI admins a bad time. That is why I am currently writing down the informal policy for adding new above-INFO-priority hints to the validator (have them as INFO hints first, progressively raise their priority based on time passed), so we can just follow a known process next time.

For spec changes, I would love more reviews, so thank you for volunteering! I am not sure which method to reach a consensus I would like, I just don't think the Wayland-style way would work for this project... In the past, discussions always converged to an end point though, and something that was significantly reworked then made it (like with the internet tag #421 - but even there, we simply didn't have enough reviewers).
If there was a formal process, I would like if at least everyone would have commented on an issue and the proposer would have to address their concerns (similar to scientific reviews), but with a process like this it could still stall if people could not come to an agreement. Currently, I break these ties with my maintainer hat on.

In general, as far as giving us any policy goes, I will need to think about this a lot more (way more than this super long post allowed me for ^^), ideas are very appreciated.
In the meanwhile, writing down a few of the rules-so-far would already help, and I'm on it. It will not protect against Flatpak bugs, but it will prevent us from making half of KDE fail validation again ;-) (if that issue had been reported any sooner, I would also have made a bugfix release to address it that way, but it was already too late).

Thank you for your message and especially for volunteering to look over spec changes - AppStream's scope is so vast nowadays that help is appreciated! Code review is already a chore, but spec review is even more work.

[ramcq]

Absolutely agree that Flatpak carrying it's own boutique (and terrible) parser is nothing other than a Flatpak bug; and likely the failure of Flatpak to forsee or avoid this issue (perhaps go back 2 years, port Flatpak to libappstream, come back to the future...) is mostly just maintenance bandwidth. Validator changes also give Flathub a hard time!

Also agree that requiring specific named people / entities to sign off is a very easy way to cause stalls/breakage in the process - as we were discussing at FOSDEM! Perhaps right now the most important first step is just to make sure a reasonable cross-section of the people who should know / react are made aware in a timely manner. We could think along the lines of 1) define a spec reviewers group, 2) tag them into non-trivial change proposals, and 3) a certain number of acks are required, set as some reasonable proportion (40-60%) of the number of reviewers.

[ximion]

Validator changes also give Flathub a hard time!

I was admittedly watching this with some excitement as well as concern ^^ - Flatpak-builder switched to appstreamcli finally, from using appstream-util before, and the former has a ton more validation. So suddenly people had to fix a ton of lingering issues. On Debian and other distros, those issues were raised before, but due to the very lenient compose code, there wasn't much pressure to fix issues. And Flathub made many tags even stricter.
So on the one hand, yay for better metadata, but on the other hand the impact of this change was huge and most importantly likely very unexpected for app authors (but there was no alternative to it).
(Yet, I am also convinced that people will not fix things unless the CI turns red, and info-severity hints will be ignored by most people. Which is fair, writing code is a lot more exciting than writing metadata!)

Absolutely agree that Flatpak carrying it's own boutique (and terrible) parser is nothing other than a Flatpak bug; and likely the failure of Flatpak to forsee or avoid this issue (perhaps go back 2 years, port Flatpak to libappstream, come back to the future...) is mostly just maintenance bandwidth.

Same issue everywhere :-) This is why I raised flathub-infra/website#2565 as a note for the Flathub website, but website code is probably the one instance where there are legitimate reasons for not using libappstream (I do for software.pureos.net though, so far it works well with the underlying database design).

Also agree that requiring specific named people / entities to sign off is a very easy way to cause stalls/breakage in the process - as we were discussing at FOSDEM!

That's why I was quite surprised by this particular suggestion 😄 - of course, not to say no process is better ^^

We could think along the lines of 1) define a spec reviewers group, 2) tag them into non-trivial change proposals, and 3) a certain number of acks are required, set as some reasonable proportion (40-60%) of the number of reviewers.

I am still not convinced about 3) - it will make me chase after people to achieve quorum, and will make reaching decisions harder as people go away. It will also force people to vote on stuff they don't care about, or more niche usecases will be ignored because the majority doesn't ACK it.

However, one thing I think we could start with and see if it works at all is to compile a list of people who want to get notified of changes for review in a particular area of AppStream and who can then review that change in the first place. Maybe we can even have a bot that just pings potential reviewers once a bug/PR has been tagged accordingly. And if the review of these people goes positively, we can merge it, and if it needs changes we can iterate on it.
TBH, after thinking about this for the last hour, I do want to retain the privilege to veto things that are out of scope for the project or that would be excessive to maintain. I maintain this project in my free time, and having a group of people decide that I have to maintain $crazy_feature now, and forever does not seem good. I doubt I would need to do that often though, but it definitely has happened in the past (for example, when complete package manager semantics were proposed to be added to AppStream)...

That said, the volume of changes proposed and added to AppStream is also fairly manageable - I always expected it to settle, but after a decade of doing this, I doubt we will ever be "done" here. But given that we are not the Linux kernel, or even Wayland, we may not need to create a very complex governing process (I would like to know how one without a BDFL that also avoids stalling and infine-loop discussions would look like - I guess it would ultimately need a FESCo/Debian TC like body, which seems like overkill for a small project like this).

[ximion]

Oh, tl;dr: We should make a list of people that are interested in reviewing changes, so I can ping them. Would also be interesting to know if someone was also interested in code review... ;-)

@ramcq Every single person you ping'ed above has already been involved in at least one specification review or bug report / PR to alter validation - it's the usual suspects 😉 (with cassidyjames and sophie-h missing ^^)

[TingPing]

Maybe I should submit a patch to make it use AppStream's parser instead, so this won't happen again.

I agree this would be for the best. The main problem is flatpak tries to target fairly old systems where libappstream will be quite outdated, but it could land in a future version at least.

Maybe we could also get a CI job here that runs flatpak in some form to help avoid future issues?

[ximion]

The main problem is flatpak tries to target fairly old systems where libappstream will be quite outdated, but it could land in a future version at least.

I usually take that into account when making library changes - older AppStream versions should parse newer AppStream versions mostly, some metadata may of course be missing, and in rare events switching the catalog generator to older spec versions was advised. The only actual breakage on that front occurred with 1.0, because that dropped a lot of old compatibility code, so 1.0-AppStream will not support very old versions well (anything above 0.16.2 will again be fine though, so this is a possible upgrade path for older systems that have not yet been ported).

Maybe we could also get a CI job here that runs flatpak in some form to help avoid future issues?

I'd be very open to that, if we could come up with some kind of meaningful testcase. I'll definitely work on porting Flatpak, it already has a dependency on libappstream anyway, so we might as well use it for parsing :-) (this was only ever done anyway to support both AppStream and appstream-glib in the same process, which is no longer a concern).