Created attachment 371296 [details] [review]
gdesktopappinfo: Filter out some binary names in search

The executable name can be a useful bit of information to match on in
searches where it differs from the name (for example because the latter
is localised), but will produce surprising results where the real appli-
cation is executed by a shared binary (for example interpretors like
gjs or python, or sandboxes like flatpak).

Address this by adding a blacklist of binary names that are ignored
in search.

Review of attachment 371296 [details] [review]:

Please add some tests.

::: gio/gdesktopappinfo.c
@@ +388,3 @@
 };
 
+const char * exec_key_match_blacklist[] = {

I think this could be `const gchar * const exec_key_match_blacklist[]`.

It could do with a comment above like `/* Common prefix commands to ignore from Exec= lines. */`.

@@ +1103,3 @@
                     value = slash + 1;
+
+                  /* Don't match on blacklisted binaries like interpretors */

Nitpick: s/interpretors/interpreters/

@@ +1106,3 @@
+                  for (blacklisted = exec_key_match_blacklist;
+                       *blacklisted;
+                       blacklisted++)

You could actually eliminate this loop and use g_strv_contains() instead.

Review of attachment 371296 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +396,3 @@
+  "python3",
+  "wine",
+  NULL

"wine64", "env", "sh", "bash".

Checking for the "python" prefix rather than a complete match could also be helpful incase something uses `python3.7` or such.

(In reply to Patrick Griffis (tingping) from comment #3)
> Checking for the "python" prefix rather than a complete match could also be
> helpful incase something uses `python3.7` or such.

Then you get false negatives on any application which starts with ‘python’, like ‘python-dbusmock’. How about a prefix match, but not matching anything with a hyphen in?

(In reply to Philip Withnall from comment #4)
> (In reply to Patrick Griffis (tingping) from comment #3)
> > Checking for the "python" prefix rather than a complete match could also be
> > helpful incase something uses `python3.7` or such.
> 
> Then you get false negatives on any application which starts with ‘python’,
> like ‘python-dbusmock’. How about a prefix match, but not matching anything
> with a hyphen in?

To be honest, I prefer the simplicity of g_strv_contains() until there's a clear indication that this is an issue in practice. It's of course possible that I'm not using the "right" python apps, but all the ones I have on my system install an executable script and reference that in the .desktop file ...

Created attachment 371303 [details] [review]
tests: Use gnome-clocks from flatpak

It's the future (and provide us with a new test case) ...

Created attachment 371304 [details] [review]
gdesktopappinfo: Filter out some binary names in search

The executable name can be a useful bit of information to match on in
searches where it differs from the name (for example because the latter
is localised), but will produce surprising results where the real appli-
cation is executed by a shared binary (for example interpretors like
gjs or python, or sandboxes like flatpak).

Address this by adding a blacklist of binary names that are ignored
in search.

(In reply to Florian Müllner from comment #6)
> It's the future (and provide us with a new test case) ...

*provides (fixed locally)

Review of attachment 371303 [details] [review]:

Sure, feel free to push with the local modification, once the other patch is a_c-n.

Review of attachment 371304 [details] [review]:

Lovely, thanks.

Attachment 371303 [details] pushed as 1489955 - tests: Use gnome-clocks from flatpak
Attachment 371304 [details] pushed as 1e2579d - gdesktopappinfo: Filter out some binary names in search

Gah, it looks like I broke the test suite when building with meson (I only tested with autotools) ...

♥ CI ♥

Specifically, https://gitlab.gnome.org/GNOME/glib/-/jobs/26560:


168/194 desktop-app-info                        FAIL     0.71 s

--- command ---
G_TEST_SRCDIR='/builds/GNOME/glib/gio/tests' G_TEST_BUILDDIR='/builds/GNOME/glib/_build/gio/tests' GIO_MODULE_DIR='' /builds/GNOME/glib/_build/gio/tests/desktop-app-info
--- stdout ---
/desktop-app-info/delete: OK
/desktop-app-info/default: OK
/desktop-app-info/fallback: OK
/desktop-app-info/lastused: OK
/desktop-app-info/extra-getters: OK
/desktop-app-info/actions: OK
/desktop-app-info/search: 
--- stderr ---
**
GLib-GIO:ERROR:../gio/tests/desktop-app-info.c:555:assert_strings_equivalent: assertion failed (g_strv_length (expected_words) == g_strv_length (result_words)): (27 == 26)

(In reply to Philip Withnall from comment #13)
> **
> GLib-GIO:ERROR:../gio/tests/desktop-app-info.c:555:assert_strings_equivalent:
> assertion failed (g_strv_length (expected_words) == g_strv_length
> (result_words)): (27 == 26)

Right, it looks like it somehow picked up the removal of "gnome-clocks.desktop", but not the addition of "org.gnome.clocks.desktop". For some reason the same test is working locally (with either autotools or meson) :-(

(In reply to Florian Müllner from comment #14)
> (In reply to Philip Withnall from comment #13)
> > **
> > GLib-GIO:ERROR:../gio/tests/desktop-app-info.c:555:assert_strings_equivalent:
> > assertion failed (g_strv_length (expected_words) == g_strv_length
> > (result_words)): (27 == 26)
> 
> Right, it looks like it somehow picked up the removal of
> "gnome-clocks.desktop", but not the addition of "org.gnome.clocks.desktop".
> For some reason the same test is working locally (with either autotools or
> meson) :-(

Odd. You can fork the GLib repository on GitLab and push test commits to your fork, and the CI will run on them, which will allow you to debug further.

So what breaks are the two `assert_list` calls that involve ALL_USR_APPS in test_search(). Not sure yet why though ...

Oh, I think I figured it out - g_desktop_app_info_load_from_keyfile() checks whether the executable in Exec= actually exists. I do have flatpak installed locally, so the new .desktop file is visible when I run the tests there, but it's apparently not in the docker image.

(In reply to Florian Müllner from comment #17)
> Oh, I think I figured it out - g_desktop_app_info_load_from_keyfile() checks
> whether the executable in Exec= actually exists. I do have flatpak installed
> locally, so the new .desktop file is visible when I run the tests there, but
> it's apparently not in the docker image.

Ah, that could be it.

Maybe change the test to use `sh` as a prefix command instead then. While you’re at it, you could probably add a test for this behaviour (checking to see if the binary exists) if such a test doesn’t already exist.

(In reply to Philip Withnall from comment #18)
> Maybe change the test to use `sh` as a prefix command instead then.

Ah, that sounds better than my fix of adding a "flatpak" executable (symlink to /usr/bin/true) and including an appropriate PATH in the test environment ...

Created attachment 371324 [details] [review]
tests: Fix desktop-app-info test

g_desktop_app_info_load_from_keyfile() refuses to load .desktop files
where the executable doesn't exist. Therefore whether or not the .desktop
file added in commit 148995544 is actually considered during tests depends
on /usr/bin/flatpak being installed. This isn't a safe assumption to make,
so use /bin/sh to test filtering of "prefix" commands.

Created attachment 371325 [details] [review]
tests: Add .desktop file for non-existent binary ...

... to test that it is filtered out correctly by
g_desktop_app_info_load_from_keyfile().

... aaaand passed: https://gitlab.gnome.org/GNOME/glib/-/jobs/26631

Review of attachment 371324 [details] [review]:

Good to commit with this comment.

::: gio/tests/desktop-files/usr/applications/org.gnome.clocks.desktop
@@ +343,3 @@
 Keywords[ug]=time;timer;alarm;world clock;stopwatch;time zone;ۋاقىت;ئۆلچىگۈچ;قوڭغۇراق;دۇنيا سائىتى;ۋاقىت رايونى;
 Keywords=time;timer;alarm;world clock;stopwatch;time zone;
+Exec=/bin/sh run --branch=stable --arch=x86_64 --command=gnome-clocks org.gnome.clocks

Since this looks like it should be /usr/bin/flatpak, it would be useful to add a comment above briefly explaining why we’ve set it to /bin/sh instead. For example:

# GDesktopAppInfo requires that the command exists, so use /bin/sh rather than /usr/bin/flatpak, since that’s guaranteed to exist on all test systems (particularly CI machines).

Review of attachment 371325 [details] [review]:

Shouldn’t there be some kind of explicit test in desktop-app-info.c which checks that frobnicator.desktop is never returned for searches, even if those searches include ‘frobnicator’ or ‘does-not-exist’ in their terms?

(In reply to Philip Withnall from comment #24)
> Shouldn’t there be some kind of explicit test in desktop-app-info.c which
> checks that frobnicator.desktop is never returned for searches, even if
> those searches include ‘frobnicator’ or ‘does-not-exist’ in their terms?

I didn't consider that necessary, given that the assert_list() calls already test that implicitly. But if you prefer, I can also add an explicit test case.

Comment on attachment 371324 [details] [review]
tests: Fix desktop-app-info test

Attachment 371324 [details] pushed as 6343555 - tests: Fix desktop-app-info test

(In reply to Florian Müllner from comment #25)
> (In reply to Philip Withnall from comment #24)
> > Shouldn’t there be some kind of explicit test in desktop-app-info.c which
> > checks that frobnicator.desktop is never returned for searches, even if
> > those searches include ‘frobnicator’ or ‘does-not-exist’ in their terms?
> 
> I didn't consider that necessary, given that the assert_list() calls already
> test that implicitly. But if you prefer, I can also add an explicit test
> case.

Yeah, I think an explicit assert_search() call or two for terms in frobnicator.desktop would be good, since assert_list() is testing g_app_info_get_all() whereas assert_search() is testing g_desktop_app_info_search().

(In reply to Philip Withnall from comment #27)
> Yeah, I think an explicit assert_search() call or two for terms in
> frobnicator.desktop would be good, since assert_list() is testing
> g_app_info_get_all() whereas assert_search() is testing
> g_desktop_app_info_search().

Yikes, very good point indeed:
https://gnome.pages.gitlab.gnome.org/-/glib/-/jobs/26854/artifacts/_build/meson-logs/testlog.txt

In your opinion, is it the test or the code that's wrong?

(As it turns out, it's something that we do take into account in gnome-shell[0], so I don't discard that the GIO behavior is intentional)

[0] https://gitlab.gnome.org/GNOME/gnome-shell/blob/master/js/ui/appDisplay.js#L1114

(In reply to Florian Müllner from comment #28)
> (In reply to Philip Withnall from comment #27)
> > Yeah, I think an explicit assert_search() call or two for terms in
> > frobnicator.desktop would be good, since assert_list() is testing
> > g_app_info_get_all() whereas assert_search() is testing
> > g_desktop_app_info_search().
> 
> Yikes, very good point indeed:
> https://gnome.pages.gitlab.gnome.org/-/glib/-/jobs/26854/artifacts/_build/
> meson-logs/testlog.txt
> 
> In your opinion, is it the test or the code that's wrong?

Hmm. My first thought would be that g_desktop_app_info_search() should be checking whether the executables exist. However, to fit that in with the way the code actually works would mean searching for all the programs when we build the search index — that’s a lot of stat()ing and the results could be outdated if the contents of your $PATH change after the index is built.

I think it makes more sense for g_desktop_app_info_search() to *not* check whether executables exist, and for it to leave that to whatever code makes use of the results (e.g. g_desktop_app_info_load_from_keyfile()). That reduces the chances of the results being stale, and avoids the stat() bomb.

So actually I think it’s the test which is wrong. But we can keep the test and just change it to assert that frobnicator.desktop *is* found, and explain why in the comment.

Thanks for sticking with this!

Created attachment 371435 [details] [review]
tests: Add .desktop file for non-existent binary ...

(In reply to Philip Withnall from comment #29)
> But we can keep the test and just change it to assert that
> frobnicator.desktop *is* found, and explain why in the comment.

Sounds good to me.

Review of attachment 371435 [details] [review]:

Go go go!

Attachment 371435 [details] pushed as f9f497a - tests: Add .desktop file for non-existent binary ...