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

Refuse to run if Categories= is missing in desktop file #873

Open
probonopd opened this issue Oct 21, 2018 · 13 comments
Open

Refuse to run if Categories= is missing in desktop file #873

probonopd opened this issue Oct 21, 2018 · 13 comments

Comments

@probonopd
Copy link
Member

probonopd commented Oct 21, 2018

appimagetool shoud refuse to run if Categories= is missing in desktop file.

Should help in these cases:
AppImage/appimage.github.io#2

@azubieta
Copy link
Contributor

Why to refuse? Why at run-time? It's not enough to verify it when the AppImage is created.
I don't see the leak of the Categories entry something that must prevent the execution of an AppImage. Please elaborate this issue more.

@TheAssassin
Copy link
Member

@probonopd we shouldn't invent our own validation, we use desktop-file-validate and it worked fine for quite a while.

@probonopd
Copy link
Member Author

probonopd commented Nov 23, 2018

Why to refuse? Why at run-time? It's not enough to verify it when the AppImage is created.

Exactly.

We should refuse the creation of an AppImage if Categories= is missing.

we shouldn't invent our own validation, we use desktop-file-validate and it worked fine for quite a while.

As far as I know, desktop-file-validate lets this slip. But the applications then land in "Others" in the menu, which is ugly.

I then have a lot of work rejecting those and getting things sorted in https://github.com/AppImage/appimage.github.io, and am sick and tired of it. See AppImage/appimage.github.io#2

Since @develar is using his own tools, he should check for this too, if I may have a wish :-)

@develar
Copy link
Contributor

develar commented Nov 23, 2018

Since @develar is using his own tools, he should check for this too, if I may have a wish :-)

According to your suggestions (thanks!), in case of electron-builder this value is never empty — https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/targets/LinuxTargetHelper.ts#L102 If we cannot do something smart, warn printed and "Utility" is used.

@probonopd
Copy link
Member Author

Cool, thanks @develar 👍

@TheAssassin
Copy link
Member

We shouldn't try to solve issues in Freedesktop standards by adding more and more constraints.

We could show a warning...

@probonopd
Copy link
Member Author

probonopd commented Nov 24, 2018

As appimagetool is a developer not a user tool, I am for showing a fatal warning. So that AppImage generation cannot continue unless the Categories key exists. Not having one is bad, and gets the AppImage rejected in https://github.com/AppImage/appimage.github.io anyway, only that it causes me and the developer additional work if not caught early.

@TheAssassin
Copy link
Member

I am against restricting developers in that way. We should only show recommendations how to improve users' UX. Such a warning will make people just add Categories=Utilities; and will move the AppImages in there. That's worse than Other, as that will at least make users notify that there's an issue, and perhaps make them request better desktop files upstream.

@probonopd
Copy link
Member Author

The point is, I am currently we are not letting such AppImages pass tests in AppImageHub. Either we let it slip in both places nor none, or else we are just creating additional work for ourselves. And I hate avoidable chores...

@TheAssassin
Copy link
Member

There is a big difference between "necessary" and not.

linuxdeploy would be the right place to add such checks for instance. But appimagetool is too late in the toolchain to add constraints.

@probonopd
Copy link
Member Author

probonopd commented Nov 24, 2018

One can look at it as "late", but one can also look at it as "low level", meaning most AppImages will run through appimagetool, so checking here means that we catch issues no matter which higher-level tool is used. This doesn't prevent us from putting the same check into the higher-level, "earlier" tools as well.

@TheAssassin
Copy link
Member

TheAssassin commented Nov 24, 2018

"low level" is the right word. Low level tools shouldn't apply any non-fatal constraints. And it's a non-fatal one in the sense that it's a valid desktop file.

@probonopd
Copy link
Member Author

Low level tools shouldn't apply any non-fatal constraints.

Where does this theory come from?

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

No branches or pull requests

4 participants