Hi Maxim, Maxim Cournoyer <maxim.courno...@gmail.com> skribis:
> Ludovic Courtès <l...@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’.