Closed Bug 1328153 Opened 5 years ago Closed 5 years ago

Replace the dbus notifications with the notify-send system command

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set
normal

Tracking

(firefox53 affected)

RESOLVED FIXED
Tracking Status
firefox53 --- affected

People

(Reporter: wgianopoulos, Assigned: wgianopoulos)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 981146 added dbus notifications to mozbuild for Linux builds.  The problem with this is that if you do not run the freedesktop, the attempt at nofication fails with:

Notification center failed: The name org.freedesktop.Notifications was not provided by any .service files

It seems that in this case you need to install the package xfce4-notifyd, which is not installed by the bootstrap.py that is provided to install all the required build tools.

However, installing that only makes the issue worse.  Because it now can attempt the notification, this adds a new issue.  If there is no x desktop session running the notification waits for a 2 minute timeout trying to connect to a non-existent service.

It seems to me all of this can be avoided by using the notify-send system command which is part of libnotify.  libnotify is a required package for building and is therefore installed by bootstrap.py and it also does not suffer from the long timeout if no xsession is running.

I propose replacing the dbus notification code with just using the notify-send command.
I am working on, and currently testing a patch for this.
Assignee: nobody → wgianopoulos
Status: NEW → ASSIGNED
This fixes all of the issues for me under fedora25 using GNOME3 desktop under Wayland.  Besides the review I am looking for feedback from :evilpie to make sure this still addresses his original issue.
Attachment #8823117 - Flags: review?(gps)
Attachment #8823117 - Flags: feedback?(evilpies)
Fix typos
Attachment #8823117 - Attachment is obsolete: true
Attachment #8823117 - Flags: review?(gps)
Attachment #8823117 - Flags: feedback?(evilpies)
Attachment #8823119 - Flags: review?(gps)
Attachment #8823119 - Flags: feedback?(evilpies)
By the way, this patch addresses the concerns form Bug 981146 comment #1.  Perhaps we should have looked more seriously at those.
(In reply to Bill Gianopoulos [:WG9s] from comment #4)
> By the way, this patch addresses the concerns form Bug 981146 comment #1. 
> Perhaps we should have looked more seriously at those.

Well actually it is a similar concern.  The issue is not that the Python dbus package might not be installed, but that the XFCE Notify Daemon is probably not installed.
Make it clearer that notify-send is usually provided by libnotify in the error message that appears it if is absent.
Attachment #8823119 - Attachment is obsolete: true
Attachment #8823119 - Flags: review?(gps)
Attachment #8823119 - Flags: feedback?(evilpies)
Attachment #8823120 - Flags: review?(gps)
Attachment #8823120 - Flags: feedback?(evilpies)
I am surprised, I assumed notify-send would use the same freedesktop API.
(In reply to Tom Schuster [:evilpie] from comment #7)
> I am surprised, I assumed notify-send would use the same freedesktop API.

I suspect it is cleverer than that and uses whatever the native interface is for your Operating System and desktop environment.   It certainly works for me without me installing the freedesktop notify daemon.

So my feedback request was kind of more like does the command:

notify-send --app-name 'Mozilla Build System' 'Mozilla Build System' 'Build complete'

work in the original environment that you did not see notifications under?
Comment on attachment 8823120 [details] [diff] [review]
Use notify-send for build notifications

Works for me right now, but this is not the same system, but still Linux Mint. I super happy about this approach if it supports more stuff.
Attachment #8823120 - Flags: feedback?(evilpies) → feedback+
OH, and for those who think the 2 instances of 'Mozilla Build System' are redundant, the first one sets the application name, which is used on the lock screen to say <app-name> has x notification(s).  The second one sets the title for the actual notification pop-up which is 2 lines the first has the title in bold and the message on the second line in normal font.
(In reply to Tom Schuster [:evilpie] from comment #9)
> Comment on attachment 8823120 [details] [diff] [review]
> Use notify-send for build notifications
> 
> Works for me right now, but this is not the same system, but still Linux
> Mint. I super happy about this approach if it supports more stuff.

Great otherwise i was going to come up with a way more complex patch to try the dbus if notify-send did not work and then figure out what to do about what to tell people to do to make notifications work if neither suceeded.
By the way I filed this Python issue upstream on the fact that the resultant log entry created by this run process is an ill formatted command.

http://bugs.python.org/issue29135
(In reply to Bill Gianopoulos [:WG9s] from comment #12)
> By the way I filed this Python issue upstream on the fact that the resultant
> log entry created by this run process is an ill formatted command.
> 
> http://bugs.python.org/issue29135

It turns out that run_process is actually part of the mach mixins, so I will be filing a Mozilla bug on this issues.
Not sure why the previous version of this patch worked.  This version fixes the specification of the --app-name parameter to conform to the command documentation.
Attachment #8823120 - Attachment is obsolete: true
Attachment #8823120 - Flags: review?(gps)
Attachment #8823275 - Flags: review?(gps)
(In reply to Bill Gianopoulos [:WG9s] from comment #13)
> (In reply to Bill Gianopoulos [:WG9s] from comment #12)
> > By the way I filed this Python issue upstream on the fact that the resultant
> > log entry created by this run process is an ill formatted command.
> > 
> > http://bugs.python.org/issue29135
> 
> It turns out that run_process is actually part of the mach mixins, so I will
> be filing a Mozilla bug on this issues.

I filled bug 1328393 on this issue.
Comment on attachment 8823275 [details] [diff] [review]
Use notify-send for build notifications

Review of attachment 8823275 [details] [diff] [review]:
-----------------------------------------------------------------

This patch on its own is fine.

There should be additional work to python/mozboot to install the package providing `notify-send` (which on Debian/Ubuntu appears to be libnotify-bin). Also, we can remove occurrences of "python-dbus" from installed packages, since we no longer need that Python package to send notifications. I'd prefer the bootstrap changes land at the same time. But I don't want to hold up your forward progress. So land this if you want.
Attachment #8823275 - Flags: review?(gps) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e179dbf1d095
Change Linux notify to use the system command notify-send to avoid freedesktop dependency, r=gps
Keywords: checkin-needed
Blocks: 1330023
I guess we don't need to leave this open. Greg filed a followup.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Core → Firefox Build System
Blocks: 1445193
You need to log in before you can comment on or make changes to this bug.