bug#36376: Application menu of desktop environment not automatically updated

[Ludovic Courtès]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>> So with this new patch it’s monitoring /var/guix/profiles/per-user/USER
>> and /var/guix/profiles instead, which correctly detects changes.  It
>> detects a bit “too much” (for instance, running ‘guix pull’ triggers the
>> inotify hook because it changes
>> /var/guix/profiles/per-user/USER/current-guix) but that’s probably OK.
>>
>> If you’re using GNOME, Xfce, or another GLib-based desktop environment,
>> I’d welcome tests on the bare metal!  Just apply the patch on a checkout
>> of ‘master’ and run:
>
> I've now done so!  It works :-)!  I do have noticed something a bit off,
> see the comments below.

Thanks for testing!

>> +@@ -249,11 +258,11 @@ desktop_file_dir_changed (GFileMonitor      *monitor,
>> +    *
>> +    * If this is a notification for a parent directory (because the
>> +    * desktop directory didn't exist) then we shouldn't fire the signal
>> +-   * unless something actually changed.
>> ++   * unless something actually changed or it's in /var/guix/profiles.
>> +    */
>> +   g_mutex_lock (&desktop_file_dir_lock);
>> +
>> +-  if (dir->alternatively_watching)
>> ++  if (dir->alternatively_watching && dir->guix_profile_watch_dir == NULL)
>                                       ^^^^^^
>                                       Why is this needed/desirable?

As the comment above states it, the ‘if’ is here so that the “changed”
signal is not fired when we’re just watching a parent directory.

However, in our case, ‘dir->alternatively_watching != NULL’ possibly
means that we’re watching a Guix profile, in which case we do want to
fire the “changed” signal.

This “&&” allows us to disambiguate between “watching a parent
directory” and “watching a Guix profile”.

>> +@@ -1555,6 +1564,32 @@ desktop_file_dirs_lock (void)
>> +       for (i = 0; dirs[i]; i++)
>> +         g_ptr_array_add (desktop_file_dirs, desktop_file_dir_new 
>> (dirs[i]));
>> +
>> ++      {
>> ++        /* Monitor the system and user profile under /var/guix/profiles and
>> ++         * treat modifications to them as if they were modifications to 
>> their
>> ++         * /share sub-directory.  */
>> ++        const gchar *user;
>> ++        DesktopFileDir *system_profile_dir, *user_profile_dir;
>> ++
>> ++        system_profile_dir =
>> ++          desktop_file_dir_new ("/var/guix/profiles/system/profile/share");
>> ++        system_profile_dir->guix_profile_watch_dir = g_strdup 
>> ("/var/guix/profiles");
>> ++        g_ptr_array_add (desktop_file_dirs, desktop_file_dir_ref 
>> (system_profile_dir));
>> ++
>> ++        user = g_get_user_name ();
>
> This seems to get the user of the running glib application; e.g. for
> GNOME Shell it returns 'gdm'...
>
>> ++        if (user != NULL)
>> ++          {
>> ++            gchar *profile_dir, *user_data_dir;
>> ++
>> ++            profile_dir = g_build_filename ("/var/guix/profiles/per-user", 
>> user, NULL);
>> ++            user_data_dir = g_build_filename (profile_dir, "guix-profile", 
>> "share", NULL);
>> ++            user_profile_dir = desktop_file_dir_new (user_data_dir);
>> ++            user_profile_dir->guix_profile_watch_dir = profile_dir;
>> ++            g_ptr_array_add (desktop_file_dirs, desktop_file_dir_ref 
>> (user_profile_dir));
>> ++            g_free (user_data_dir);
>> ++          }
>> ++      }
>> ++
>
> ...which means the above puts the watch on the
> "/var/guix/profiles/per-user/gdm" directory, which doesn't exist.

Yes, that profile is typically never populated.

> sudo strace -f -s200 -p$(pgrep gnome-shell | head -n1) reports entries
> such as:
>
> 92   00:48:47 poll([{fd=6, events=POLLIN}, {fd=7, events=POLLIN}, {fd=9, 
> events=POLLIN}, {fd=19, events=POLLIN}, {fd=28, events=POLLIN}], 5, -1 
> <unfinished ...>
> 390   00:48:47 <... poll resumed>)      = 0 (Timeout)
> 390   00:48:47 inotify_add_watch(17, "/var/guix/profiles/per-user/gdm", 
> IN_MODIFY|IN_ATTRIB|IN_CLOSE_WRITE|IN_MOVED_FROM|IN_MOVED_TO|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF|IN_UNMOUNT|IN_ONLYDIR)
>  = -1 ENOENT (No such file or directory)
> 390   00:48:47 poll([{fd=3, events=POLLIN}, {fd=17, events=POLLIN}], 2, 3996) 
> = 0 (Timeout)
> 390   00:48:51 inotify_add_watch(17, "/var/guix/profiles/per-user/gdm", 
> IN_MODIFY|IN_ATTRIB|IN_CLOSE_WRITE|IN_MOVED_FROM|IN_MOVED_TO|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF|IN_UNMOUNT|IN_ONLYDIR)
>  = -1 ENOENT (No such file or directory)
> 390   00:48:51 poll([{fd=3, events=POLLIN}, {fd=17, events=POLLIN}], 2, 3998) 
> = 0 (Timeout)
> 390   00:48:55 inotify_add_watch(17, "/var/guix/profiles/per-user/gdm", 
> IN_MODIFY|IN_ATTRIB|IN_CLOSE_WRITE|IN_MOVED_FROM|IN_MOVED_TO|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF|IN_UNMOUNT|IN_ONLYDIR)
>  = -1 ENOENT (No such file or directory)
>
> The fallback mechanism should have been disabled in
> desktop_file_dir_changed (dir->guix-profile_watch_dir != NULL so
> desktop_file_dir_get_alternative_dir doesn't get called), so it seems
> this shouldn't work.

What shouldn’t work?

We keep adding a watch on a non-existent directory, but I think that’s
fine.

> Could it be that the watches used by glib implement a recursive
> inotify-based watch and that it works because any changes under the
> /var/guix/profiles directory get picked up, including those of user
> profiles.  The sources seem to suggest so, for example in
> gio/inotify/inotify-path.c.  If that is the case, it could lead to a new
> interesting problems: applications from other users could end up being
> shown inadvertently.

We’re watching /var/guix/profiles (for the system profile) and
/var/guix/profiles/per-user/USER, but .desktop files are loaded from the
user’s own profile, so that should be fine.

Thanks a lot for looking into this!

Ludo’.