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.

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

Stricter validation hasn't been adjusted in the documentation #302

Closed
kparal opened this issue May 13, 2019 · 25 comments · Fixed by #305
Closed

Stricter validation hasn't been adjusted in the documentation #302

kparal opened this issue May 13, 2019 · 25 comments · Fixed by #305

Comments

@kparal
Copy link

kparal commented May 13, 2019

My application on Flathub started failing to get built, probably due to #296:

$ flatpak run org.freedesktop.appstream-glib validate builddir/*/share/appdata/org.freefilesync.FreeFileSync.appdata.xml
builddir/files/share/appdata/org.freefilesync.FreeFileSync.appdata.xml: FAILED:
• tag-missing           : <translation> not specified
• tag-missing           : <update_contact> is not present
• attribute-invalid     : <screenshot> width too small [https://www.freefilesync.org/images/screenshots/RealTimeSync.png] minimum is 624px
Validation of files failed
program finished with exit code 1

However, when I look into the documentation:
https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html
I don't any violations. One by one:

• tag-missing           : <translation> not specified

The tag is an optional tag which...

• tag-missing           : <update_contact> is not present

The <update_contact/> tag is an optional tag which...

• attribute-invalid     : <screenshot> width too small [https://www.freefilesync.org/images/screenshots/RealTimeSync.png] minimum is 624px

Ideally, all screenshots should have a 16:9 aspect ratio, and should have a width that is no smaller than 620 pixels.

There is no minimum size requirement.

My appdata file seems to conform to the specification. I also checked appstream-glib readme file (I haven't found any extra documentation for it) and I didn't find any extra requirements on top of the specification.

I want to request the documentation to be updated. Or is there a reason for the spec and the implementation to not be in sync?

Furthermore, I'd like to request clearer explanation in the docs. I'm quite confused by the <translation> not specified error. Looking at the documentation, how is a packager who is not a programmer supposed to find out whether the translation system used is gettext or qt? What about proprietary apps using a different translation system? What is a translation domain (is it some gettext jargon?) and who am I supposed to find it out? And why exactly is this mandatory?

Similarly, the error and docs about <update_contact/> doesn't seem to address that some projects might not have a public email contact. Or am I supposed to put my address in, as a packager? The spec is confusing in here. Again, does it have to be mandatory?

Thanks.

@hughsie
Copy link
Owner

hughsie commented May 13, 2019

My appdata file seems to conform to the specification.

AppStreamGlib is just one implementation of the specification, it's not trying to be a perfect implementation and instead makes some pragmatic choices about the specification. For instance, the spec can (and has) broken "API" in the past, but that's not something the library can do. When choosing things that are validation failures and kudos, I'm also trying to do the appropriate amount of "stick" and "carrot" -- and over time we've got from a tiny amount of metadata required from upstream maintainers to the awesome data we have now that allows us to write programs like GNOME Software and KDE discover.

Or is there a reason for the spec and the implementation to not be in sync?

Different personal decisions -- I don't write or maintain the specification anymore.

how is a packager who is not a programmer supposed to find out whether the translation system used is gettext or qt

The matainfo/appdata file is meant to come from the upstream maintainer who does certainly know. In general gettext has .gmo/.mo files and QT has .qm files. We also support .pak and .xpi for browsers too.

What about proprietary apps using a different translation system

We can either add support for them in appstream-glib, or you can do

<translation/>

...to say "we don't support translations".

The spec is confusing in here. Again, must be mandatory?

I don't think update_contact is actually mandatory... That sounds like a bug if so.

There is no minimum size requirement

From a spec point of view, 10x10px is perfectly fine, but clearly from trying to provide an awesome software center there has to be some kind of sensible limit. A 620px wide screenshot on a HiDPI screen looks borderline acceptable, padding and super-sampling a screenshot just looks really bad.

@kparal
Copy link
Author

kparal commented May 13, 2019

Different personal decisions -- I don't write or maintain the specification anymore.

OK. So what is the place place to provide additional help to people writing appdata, e.g. regarding <translation/> as you've done above?

In general gettext has .gmo/.mo files and QT has .qm files. We also support .pak and .xpi for browsers too.

FreeFileSync translations come in form of .lng files. Can you explain what use does this tag have? How does this help the user?

you can do to say "we don't support translations"

Wow, that's something that's certainly missing either from the spec or from some extended howto/guide somewhere. Where should we put it and how to best do it (e.g. where to file an issue against the spec)?

I suppose <translation/> is something I should do in my case of unsupported translation system?

I don't think update_contact is actually mandatory... That sounds like a bug if so.

Looking at the error (I can reproduce the same with appstream-util validate) do you think this is a bug in appstream-glib or should I bug someone else? Is <translation> supposed to be mandatory or a bug as well?

@matthiasclasen
Copy link

I would expect appstream-util to have documentation that explains what the validate command actually does. The current man page is a perfunctory debian-style placeholder. We can do better

hughsie added a commit that referenced this issue May 14, 2019
@hughsie
Copy link
Owner

hughsie commented May 14, 2019

FreeFileSync translations come in form of .lng files

I've looked at that, and they seem easy enough to parse. Can you tell me where they are installed in the filesystem please? e.g. share/locale/foo

@kparal
Copy link
Author

kparal commented May 14, 2019

I've looked at that, and they seem easy enough to parse. Can you tell me where they are installed in the filesystem please? e.g. share/locale/foo

Well, this particular app is "portable" and keeps everything in its directory, so e.g. /app/FreeFileSync/Languages/italian.lng.

So do you think <translation> should be mandatory for appstream-util validate? And can you please clarify its purpose? The spec only says:

It may be used by the AppStream distro metadata generator to determine the translation status of the respective software.

But I don't understand what "translation status" means and how it can have some positive effect for the end user. I'd like to understand what exactly we're discussing here. Is this about detecting which languages the software is translated into? Or something completely different?

If this is about listing the languages, how can we distinguish between "no translations are available" and "translations are unknown and can't be programmatically discovered"? Which one of those is <translation/>?

@hughsie
Copy link
Owner

hughsie commented May 14, 2019

keeps everything in its directory

Okay, so I don't think we should parse those then.

do you think should be mandatory for appstream-util validate

My gut instinct says yes, but it's not a hill I want to die on.

how it can have some positive effect for the end user

It's how we parse the .gmo and .qt files so that we can do the "is available in your language" badge in GNOME Software, which people who don't speak en_* find hugely useful.

how can we distinguish between "no translations are available" and "translations are unknown and can't be programmatically discovered

Some apps, like Chrome, always have 100% translation in every language they support. If this is indeed the case with FFS, you can do something like:

<languages>
<lang percentage="100">ar</lang>
<lang percentage="100">ast</lang>
<lang percentage="100">de</lang>
</languages>

If the maintainer supplies <languages> then we shouldn't require <translation> -- if that's not the case then it's a bug I can easily fix. Most free software apps use gettext, and then <translation> is somewhat useful, as it provides the translation domain so we can inject the <lang> tags at build time.

@aleixpol
Copy link

Can't we programmatically populate the languages information? i.e. seeing which mo files are being installed.

@hughsie
Copy link
Owner

hughsie commented May 14, 2019

Can't we programmatically populate the languages information

Sure... but some apps install other .gmo files from other projects, e.g. in a flatpak build.

@kparal
Copy link
Author

kparal commented May 14, 2019

Thanks a lot for clarifying what <translation> is for and how to use it. So how do we go about updating the spec to make it more useful for people reading it? At least
a) updating the translation types supported (.pak, .xpi)
b) specifying that <translation/> (an empty tag) is valid and what it means
And optionally also clarifying what this tag is for (for automatically discovering which languages are supported and how much translation-complete they are).

Richard, why don't you want to maintain the spec anymore? Some parallel documentation can of course be provided by appstream-glib, but many people probably won't figure out to look into several places. (I'd file a ticket against the spec, but there's no indication where to do it, just a list of people who created it - with your name in it).

@hughsie
Copy link
Owner

hughsie commented May 14, 2019

So how do we go about updating the spec to make it more useful for people reading it

The spec is maintained by someone else and in a different place to this: https://github.com/ximion/appstream -- if you open an issue there @ximion is normally pretty good aboue adding clarifications to the spec.

Richard, why don't you want to maintain the spec anymore

@ximion works on the spec and a reference implementation, I provide a supportable library that uses the spec. We're all doing our best dude.

@tsdgeos
Copy link

tsdgeos commented May 14, 2019

style-invalid : has vertical padding [http://kde.org/images/screenshots/katomic.png]

Is this a joke? What kind of weird police are you to decide how my image has to look like?

@hughsie
Copy link
Owner

hughsie commented May 15, 2019

What kind of weird police are you to decide how my image has to look like

It's called a style guide. It means that we get a software center that looks consistent without all inds of unaligned widgets. If you look at that image with drop shadow and padding on all sides in gnome-software it's inset and the border looks ridiculously wide. Also, because of the drop shadow it looks out of place compared to other apps.

This weird policeman has been dragging up requirements for about the last 7 years.

@tsdgeos
Copy link

tsdgeos commented May 15, 2019

What kind of weird police are you to decide how my image has to look like

It's called a style guide. It means that we get a software center that looks consistent without all inds of unaligned widgets. If you look at that image with drop shadow and padding on all sides in gnome-software it's inset and the border looks ridiculously wide. Also, because of the drop shadow it looks out of place compared to other apps.

It looks perfectly fine (to me) in Discover (I've never used gnome-software) https://i.imgur.com/dlYox88.png https://i.imgur.com/bD8o9Xd.png

Can you point out where "looks good in gnome-software" was discussed as a requirement for applications being able to be distributed through flathub?

@hughsie
Copy link
Owner

hughsie commented May 15, 2019

There are lots of technical reasons for this, for instance rescaling for thumbnail creation, blurring, aspect correction and optimization. I don't think it's a valid use of my time arguing with you here.

@kparal
Copy link
Author

kparal commented May 15, 2019

@ximion works on the spec and a reference implementation, I provide a supportable library that uses the spec. We're all doing our best dude.

I wasn't trying to attack you. Your name is on the spec, and it has no link where to file issues, that's why I'm asking, that is all. Thanks for working on this. I'll file a ticket in the place you mentioned.

@tsdgeos Please file a separate ticket (or don't, because @hughsie is right 🙂), but let's not hijack this ticket, thanks.

@kparal
Copy link
Author

kparal commented May 15, 2019

I think this ticket can be closed. I originally opened it to request documentation fixes, but it turned out documentation is not maintained here, so I reported the tickets in the proper place, as referenced above (after some clarifications from @hughsie, thanks). We found a bug in <update_contact/> being mandatory and fixed it. I think this is resolved.

@hughsie
Copy link
Owner

hughsie commented May 15, 2019

Agreed, thanks.

@hughsie hughsie closed this as completed May 15, 2019
@ximion
Copy link
Collaborator

ximion commented May 16, 2019

Richard, why don't you want to maintain the spec anymore

He actually never maintained the spec, appstream-glib was developed in parallel to libappstream. However, the concept of metainfo files first appeared in appstream-glib and was for a while documented there.
We do our best nowadays though to remove differences between the implementations (as long as we find them, that is).

@kparal
Copy link
Author

kparal commented May 17, 2019

Richard, after talking to @ximion in ximion/appstream#235, I'd like to reopen this issue. appstreamcli validate currently fails with an empty <translation/> tag because its meaning is undefined in the spec:

$ appstreamcli validate org.freefilesync.FreeFileSync.appdata.xml
W - org.freefilesync.FreeFileSync.appdata.xml:org.freefilesync.FreeFileSync.desktop:77
    Found empty 'translation' tag.

E - org.freefilesync.FreeFileSync.appdata.xml:org.freefilesync.FreeFileSync.desktop:77
    'translation' tag has no 'type' property: 

Validation failed: errors: 1, warnings: 1

OTOH, appstream-glib requires me to put the empty tag in, because the software in question doesn't use a supported translation system and without the tag appstream-glib validation fails and then I can't build on Flathub. So whatever I do, my appdata file is seen as invalid by one of the implementations.

Can you please consider strictly following the spec here, as appstreamcli does? <translation/> should be an error, because the meaning is undefined, and therefore the tag should not be mandatory. If we want to change the meaning, it should go into the spec first, so that we have a defined and consistent behavior between both implementations. WDYT? Thanks.

@kalev
Copy link
Collaborator

kalev commented May 17, 2019

I think it should be fine to not require tag for "validate" (and error out on empty tag). If flathub maintainers in the future want to make sure everything on flathub has translations, I'm sure they'll come and talk to us and ask to require the tag, but for now I don't think flathub has that requirement.

@barthalion
Copy link
Contributor

At the moment appstream-glib used by Flathub has been modified not to require it.

@kalev
Copy link
Collaborator

kalev commented May 17, 2019

The plan we had at last hackfest was to make validate-relax behave the way that's suitable for a distro (e.g. Fedora) and validate for what Flathub needs. If Flathub is already patching appstream-glib to not require the translation tag for validate then it makes sense to upstream it to appstream-glib.

kalev added a commit that referenced this issue May 17, 2019
Flathub is already patching appstream-glib to remove this check.

Fixes #302
@ximion
Copy link
Collaborator

ximion commented May 17, 2019

AppStream itself resolves the issue of "what makes validation fail" by simply assigning a severity to the different issues. All "error" priority problems are severe and basically make the XML noncompliant with the spec and result in AppStream implementations having issues to process the file, all "warning" problems will not make the XML noncompliant per-se but may cause reduced metadata to be extracted or may result in less useful data in general. All "info" priority issues are usually style-related and contain hints on how to improve the metadata to make the result look better in software centers. "pedantic" type issues are either experimental checks that need a bit of real-world testing or are more nitpicky issues that the metainfo author may want to address.

Only warnings and errors cause the validation to fail, all other issues will lead to a validation pass (but they will be displayed). Users of appstreamcli may want to make info and below messages fatal if they want though.
This method of handling issues has been quite successful so far :-)

kalev added a commit that referenced this issue May 17, 2019
Flathub is already patching appstream-glib to remove this check.

Fixes #302
kparal added a commit to flathub/org.freefilesync.FreeFileSync that referenced this issue May 17, 2019
They should be non-mandatory again. For <translation> the empty tag is
controversial/non-compliant and for <update_contact> I'm not sure
whether it should be used this way (email of the program author, even if
they don't provide the appstream file).

See also: hughsie/appstream-glib#302
kparal added a commit to flathub/org.freefilesync.FreeFileSync that referenced this issue May 17, 2019
They should be non-mandatory again. For <translation> the empty tag is
controversial/non-compliant and for <update_contact> I'm not sure
whether it should be used this way (email of the program author, even if
they don't provide the appstream file).

See also: hughsie/appstream-glib#302
@maoschanz
Copy link

maoschanz commented Sep 20, 2020

What kind of weird police are you to decide how my image has to look like

It's called a style guide. It means that we get a software center that looks consistent without all inds of unaligned widgets. If you look at that image with drop shadow and padding on all sides in gnome-software it's inset and the border looks ridiculously wide. Also, because of the drop shadow it looks out of place compared to other apps.

This weird policeman has been dragging up requirements for about the last 7 years.

I suggest you communicate these clever requirements to the guy who added huge paddings to all screenshots taken with alt+printscreen.

All my screenshots had the same size and aspect ratio, now should i crop them manually? They'll have inconsistent sizes, and the cropped shadows will look like crap.

Maybe i'll just replace all the transparency with a pink-and-cyan checkboard so this validation doesn't prevent the release of my work: it'll be amazingly consistent 👍

I'm sure it'll not look "ridiculous"

@ximion
Copy link
Collaborator

ximion commented Sep 20, 2020

I suggest you communicate these clever requirements to the guy who added huge paddings to all screenshots taken with alt+printscreen.

That's why appstreamcli doesn't check images that strongly - it makes people do stupid things just to shut up the validator instead of pushing them to do the right thing. Also, there are multiple software centers with different looks, so AppStream itself can only recommend something here and can't enforce (most) styles as it is not the project actually providing a software center (it will do checks for style issues like summary-text lengths and missing punctuation, which are easy fixes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants