GNOME Bugzilla – Bug 770671
Drop hal code
Last modified: 2016-10-19 11:19:52 UTC
HAL has been dead for many years and I doubt it still does anything useful nowadays. In the complete GNOME stack, HAL has been removed, besides gvfs. I think it's time to remove the remaining HAL code in gvfs as well. This includes dropping the hal volume monitor and the hal fallback code in gphoto2 and cdda. This makes the gphoto2 and cdda monitor code vastly simpler and easier to read, as all the #ifdefs can be removed. (build-)tested patch attached.
The diffstat is like this b/common/Makefile.am | 10 b/configure.ac | 46 b/daemon/Makefile.am | 26 b/daemon/gvfsbackendcdda.c | 124 - b/daemon/gvfsbackendgphoto2.c | 317 ---- b/monitor/Makefile.am | 6 b/monitor/gphoto2/Makefile.am | 45 b/monitor/gphoto2/ggphoto2volume.c | 233 --- b/monitor/gphoto2/ggphoto2volume.h | 19 b/monitor/gphoto2/ggphoto2volumemonitor.c | 378 ----- b/po/POTFILES.in | 6 b/test/Makefile.am | 7 monitor/gphoto2/hal-device.c | 297 ---- monitor/gphoto2/hal-device.h | 93 - monitor/gphoto2/hal-marshal.list | 3 monitor/gphoto2/hal-pool.c | 458 ------ monitor/gphoto2/hal-pool.h | 74 - monitor/gphoto2/hal-utils.c | 142 -- monitor/gphoto2/hal-utils.h | 40 monitor/hal/.gitignore | 1 monitor/hal/Makefile.am | 83 - monitor/hal/ghaldrive.c | 959 -------------- monitor/hal/ghaldrive.h | 62 monitor/hal/ghalmount.c | 1192 ----------------- monitor/hal/ghalmount.h | 76 - monitor/hal/ghalvolume.c | 1007 -------------- monitor/hal/ghalvolume.h | 84 - monitor/hal/ghalvolumemonitor.c | 1361 -------------------- monitor/hal/ghalvolumemonitor.h | 59 monitor/hal/gvfs-hal-volume-monitor.service.in | 7 monitor/hal/hal-device.c | 304 ---- monitor/hal/hal-device.h | 93 - monitor/hal/hal-marshal.list | 3 monitor/hal/hal-pool.c | 451 ------ monitor/hal/hal-pool.h | 74 - monitor/hal/hal-utils.c | 171 -- monitor/hal/hal-utils.h | 42 monitor/hal/hal-volume-monitor-daemon.c | 43 monitor/hal/hal.monitor | 5 monitor/hal/org.gtk.vfs.HalVolumeMonitor.service.in | 4 40 files changed, 21 insertions(+), 8384 deletions(-)
Created attachment 334551 [details] [review] Drop hal volume monitor and fallback code in gphoto2 and cdda backend
Thanks for the patch. I had the same idea lately actually. I also doubt it does anything useful. However, as far as I know it was used on bsd systems not long ago, don't you know what is a current situation on bsd systems?
(In reply to Ondrej Holy from comment #3) > Thanks for the patch. I had the same idea lately actually. I also doubt it > does anything useful. However, as far as I know it was used on bsd systems > not long ago, don't you know what is a current situation on bsd systems? I can't speak for BSD in general, but I know that for Debian kfreebsd, the hal code is not used. Do you know, who/which mailing list we could contact to ask about this, the distributor-list mailing list maybe? Should this HAL code in gvfs be treated differently then the ConsoleKit code, which was removed from various packages and non-Linux ports are supposed to maintain them downstream? See https://blogs.gnome.org/ovitters/2015/02/24/consolekit-in-gnome-3-16-and-beyond/
fwiw, I've posted to https://mail.gnome.org/archives/distributor-list/2016-September/msg00000.html to get some feedback
Thanks! Sounds like FreeBSD uses it, see: https://svnweb.freebsd.org/ports/head/devel/gvfs/pkg-plist?view=markup Maybe worth to contact them: https://www.freebsd.org/gnome/contact.html But makes sense to me to remove it from here and let maintain it in downstream... will take a look at the patch later (it is hard code freeze anyway right now).
No hurry, I'm aware that there is a hard code freeze for 3.22 and this can land for 3.23 at the earliest
Review of attachment 334551 [details] [review]: Looks good to me apart from some minor details: ::: common/Makefile.am @@ -46,3 @@ -noinst_LTLIBRARIES += libgvfscommon-hal.la -libgvfscommon_hal_la_SOURCES = \ - gvfshalutils.c gvfshalutils.h common/gvfshalutils.[c|h] should be also removed. ::: configure.ac @@ +318,3 @@ msg_hotplug_backend="gudev" +else + msg_hotplug_backend="none" Maybe we can remove this at all and the "hotplug backend:" entry in the summary, because there is only one meaningful option... ::: monitor/gphoto2/ggphoto2volumemonitor.c @@ +40,1 @@ G_LOCK_DEFINE_STATIC(hal_vm); Also would be nice to rename hal_vm to gphoto2_vm or so, although it should have been done a long time ago.
Thanks a lot for the review. Updated patch attached fixes the issues you mentioned.
Created attachment 334902 [details] [review] Drop hal volume monitor and fallback code in gphoto2 and cdda backend v2
I guess we can now also drop PKG_CHECK_MODULES([DBUS], [dbus-1]) from configure.ac? We don't use DBUS_LIBS and DBUS_CFLAGS anywhere anymore. The only reason why dbus-1.pc could be helpful is to get with_dbus_service_dir= via `pkg-config --variable=system_bus_services_dir dbus-1` But we don't do that atm. So it seems safe to drop the requirement for dbus-1.pc being installed. WDYT?
Thanks! I think that the dbus check can be safely removed...
Created attachment 334986 [details] [review] Drop hal volume monitor and fallback code in gphoto2 and cdda backend v3 Dropped the dbus-1 dependency from configure.ac as well
Comment on attachment 334986 [details] [review] Drop hal volume monitor and fallback code in gphoto2 and cdda backend v3 Thanks, let's mark it as "accepted-commit_after_freeze" for now, but we can reconsider it if something appears on distributor-list, and also it would be nice to test cdda/gphoto2 backends for sure before pushing...
(In reply to Ondrej Holy from comment #16) > Comment on attachment 334986 [details] [review] [review] > Drop hal volume monitor and fallback code in gphoto2 and cdda backend v3 > > Thanks, let's mark it as "accepted-commit_after_freeze" for now, but we can > reconsider it if something appears on distributor-list, and also it would be > nice to test cdda/gphoto2 backends for sure before pushing... Given the feedback on the distributor mailing list [1], would you be ok if for 1.30 we add big fat warning to configure if --enable-hal is used? This warning would say that hal is deprecated and support for it in gvfs will be removed in a future release? I think this would also help making affected users aware and is something which we could sneak into the upcoming gnome 3.22 release. [1] https://mail.gnome.org/archives/distributor-list/2016-September/msg00006.html
Ah, sorry, I forget on this before the hard code freeze. I think it is a good idea to add the warning to configure... can you please prepare a patch for it?
Comment on attachment 334986 [details] [review] Drop hal volume monitor and fallback code in gphoto2 and cdda backend v3 commit ec9c45d7c7671266193a93500c7477990e390fc7