Bug 237714 - [PATCH] sysutils/xfce4-power-manager: fix crashes, improve FreeBSD support, add DEBUG option
Summary: [PATCH] sysutils/xfce4-power-manager: fix crashes, improve FreeBSD support, a...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-xfce mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-02 21:05 UTC by rozhuk.im
Modified: 2019-05-15 09:49 UTC (History)
2 users (show)

See Also:
madpilot: maintainer-feedback+


Attachments
patch (5.21 KB, patch)
2019-05-02 21:05 UTC, rozhuk.im
no flags Details | Diff
revised patch (6.71 KB, patch)
2019-05-10 13:35 UTC, Guido Falsi
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rozhuk.im 2019-05-02 21:05:58 UTC
Created attachment 204172 [details]
patch

- add DEBUG option
- fix craches: then tray menu opened and charge level changed or power source changed
- improve freebsd support: more correct display enumeration
- fix few warnings
Comment 1 Guido Falsi freebsd_committer 2019-05-03 14:36:32 UTC
Hi,

Thanks for the patch.

I have some questions:

- Why add DEBUG as an option? WITH_DEBUG is not enough to activate it, just adding --enable-debug as needed when WITH_DEBUG is on.

While there is no strict rule about this and some ports are adding a DEBUG option, it is not a good practice, and using WITH_DEBUG should be the only way to enable it. it is not a user flag, it's a developer flag anyway.


- Regarding the source changes, I need some clearifications.

the g_object_ref change should be upstreamed? Have you already sent a bug report? It looks like a bug to me. Would you propose the patch upstream?

I don't get why you need the "return g_strdup (_("Unknown"));" patch. Looking at the code I'd say the intention is for that to never be reached, but maybe there is a bug if 59.0 < value < 60.0. The upstream repository has fixed this in a different way. I'd be more comfortable importing the newer upstream code [1], to avoid diverging. Could you modify your patch to match upstream code?


Fir the xfpm-backlight-helper.c patch, before committing it I'd like to know where the "8" number comes from, and exactly what the patch fixes.

This last hunk also contains some gratuitous changes which would make upstreaming it difficult. You should not make whitespace changes (they can be there for upstream code style rules) and I'd also avoid changing the return logic, and leave it matching what the upstream used. This is because this kind of change could be refused by upstream if proposed.

Also, while upstream allocating only one gchar looks definitely too little, 64 is definitely too much, since your code never uses more than 5 (including terminating \0 ). The second argument to g_snprintf could also be calculated correctly.

As before, should this be upstreamed? Are you wiling to propose it in an upstream bug report?


[1] https://github.com/xfce-mirror/xfce4-power-manager/blob/xfce-4.12/settings/xfpm-settings.c#L1567
Comment 2 rozhuk.im 2019-05-08 23:55:05 UTC
--enable-debug does not affect to INSTALL_TARGET=	install-strip, and as result binary without debug symbols, gdb bt show nothink usable.
I see no other good ways to set INSTALL_TARGET=install, remove -O* and add -g to CFLAGS.

I will submit changes to upstream a bit later.

"return g_strdup (_("Unknown"));" - makes compiller happy and silence warning.
I will try to submit all changes to upstream.

8 - my magic number.
I assume that this is max reasonable index count for lcd0...lcd7.

I will change 64 to something like 16 in future.
Comment 3 Guido Falsi freebsd_committer 2019-05-09 07:06:08 UTC
(In reply to rozhuk.im from comment #2)
> --enable-debug does not affect to INSTALL_TARGET=	install-strip, and as
> result binary without debug symbols, gdb bt show nothink usable.
> I see no other good ways to set INSTALL_TARGET=install, remove -O* and add
> -g to CFLAGS.

Maybe my question was not clear. The standard way to build ports with debugging symbols in FreeBSD is to use the WITH_DEBUG flag, which you use correctly.

WITH_DEBUG causes the ports framework to unset the STRIP variable and set STRIP_CMD to true (the noop command), also automatically removes optimization flags from cflags and replaces INSTALL_TARGET. You can find the code doing all this in bsd.port.mk [1]


What I was criticizing is adding a "DEBUG" option using the options framework. It's better to only check for the WITH_DEBUG flag and act based on that, no need for an option in the option framework. Should every single port have a DEBUG option then?

I'd rather avoid polluting port options by adding DEBUG to OPTIONS_DEFINE. Some ports are doing this but this is not a good idea.

> 
> I will submit changes to upstream a bit later.

In general I think that, for changes which are not just porting changes, but actual development, submitting them upstream should happen before including in the ports tree. This to avoid diverging.

Anyway most of these changes look quite reasonable so I will be testing them for inclusion.

> 
> "return g_strdup (_("Unknown"));" - makes compiller happy and silence
> warning.
> I will try to submit all changes to upstream.

I see.

> 
> 8 - my magic number.
> I assume that this is max reasonable index count for lcd0...lcd7.
> 
> I will change 64 to something like 16 in future.

This could be difficult to upstream. Better put it in a define anyway.


I'll followup with a modified patch.


[1] https://svnweb.freebsd.org/ports/head/Mk/bsd.port.mk?revision=500731&view=markup#l1758
Comment 4 Guido Falsi freebsd_committer 2019-05-09 08:01:24 UTC
(In reply to Guido Falsi from comment #3)

> What I was criticizing is adding a "DEBUG" option using the options
> framework. It's better to only check for the WITH_DEBUG flag and act based
> on that, no need for an option in the option framework. Should every single
> port have a DEBUG option then?

Ti make this point clearer.

WITH_DEBUG should not be set by a port option, but should be manually set in make.conf.

Some time ago I sent a review proposing to retire WITH_DEBUG and use a proper silent option for that. I was discouraged, exactly for the same reasons I'm stating I got convinced about that reasoning.

Rif: https://reviews.freebsd.org/D15773
Comment 5 Guido Falsi freebsd_committer 2019-05-10 13:35:42 UTC
Created attachment 204312 [details]
revised patch

I have uploaded a revised patch.

This patch does not add an option, but leverages the WITH_DEBUG framework

I kept your other changes, but reworked the patch to xfpm-settingsc basing it on newer upstream code.

I also slightly changed the xfpm-backlight_helper.c patch to use a define for your magic number, use a smaller allocation and have less gratuitous changes to the code (white space for example).

I'm gong to commit this patch in the next days unless you have strong objections.

Please understand I don't feel comfortable adding a DEBUG knob to port options, so I'll not do that. The rest of the patch is almost the same as what you proposed with only minor changes.
Comment 6 rozhuk.im 2019-05-10 14:36:16 UTC
if (NULL != sleep_mode && !g_strcmp0 (sleep_mode, "Standby"))

was important, IMHO.
Im not sure is g_strcmp0() check to NULL, and this was point of crash.

I undestand that you dont like DEBUG option, but I dont know another way to build only some ports with debug and keep them with debug after portmaster upgrades.
You may not apply and not commit it, it is ok for me.
Comment 7 Guido Falsi freebsd_committer 2019-05-10 14:51:41 UTC
(In reply to rozhuk.im from comment #6)
> if (NULL != sleep_mode && !g_strcmp0 (sleep_mode, "Standby"))
> 
> was important, IMHO.
> Im not sure is g_strcmp0() check to NULL, and this was point of crash.

I forgot to mention that change in my description.

I removed that because AFAIK g_strcmp0() does check for NULL. [1]

Although, if you tell me you experienced crashes there I agree it's required, so I'll add that back before committing.

> 
> I undestand that you dont like DEBUG option, but I dont know another way to
> build only some ports with debug and keep them with debug after portmaster
> upgrades.

My suggestion is to add the port origin to WITH_DEBUG_PORTS in make.conf like:

WITH_DEBUG_PORTS=   sysutils/xfce4-power-manager \
                    x11-fm/thunar

> You may not apply and not commit it, it is ok for me.

Thanks!


[1] https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strcmp0
Comment 8 commit-hook freebsd_committer 2019-05-10 17:40:51 UTC
A commit references this bug:

Author: madpilot
Date: Fri May 10 17:40:03 UTC 2019
New revision: 501188
URL: https://svnweb.freebsd.org/changeset/ports/501188

Log:
  - Fix reported sporadic crashes
  - Enumerate all displays for backlight support
  - Fix some compile warnings
  - When building debug binaries enable extra debugging code too

  PR:		237714
  Submitted by:	rozhuk.im@gmail.com
  MFH:		2019Q2

Changes:
  head/sysutils/xfce4-power-manager/Makefile
  head/sysutils/xfce4-power-manager/files/
  head/sysutils/xfce4-power-manager/files/patch-panel-plugins_power-manager-plugin_power-manager-button.c
  head/sysutils/xfce4-power-manager/files/patch-settings_xfpm-settings-app.c
  head/sysutils/xfce4-power-manager/files/patch-settings_xfpm-settings.c
  head/sysutils/xfce4-power-manager/files/patch-src_xfpm-backlight-helper.c
  head/sysutils/xfce4-power-manager/files/patch-src_xfpm-dpms.c
Comment 9 Guido Falsi freebsd_committer 2019-05-10 17:43:13 UTC
Patch committed.

Thanks!

I'm keeping this bug open so that it can be used to track upstreaming of these changes.

Since you intend to upstream these, could you also be so kind as to update this bug accordingly, adding links to the upstream bug report so that can be tracked?

Thanks again!
Comment 10 commit-hook freebsd_committer 2019-05-11 07:49:48 UTC
A commit references this bug:

Author: madpilot
Date: Sat May 11 07:49:23 UTC 2019
New revision: 501229
URL: https://svnweb.freebsd.org/changeset/ports/501229

Log:
  MFH: r501188

  - Fix reported sporadic crashes
  - Enumerate all displays for backlight support
  - Fix some compile warnings
  - When building debug binaries enable extra debugging code too

  PR:		237714
  Submitted by:	rozhuk.im@gmail.com

  Approved by:	ports-secteam (miwi)

Changes:
_U  branches/2019Q2/
  branches/2019Q2/sysutils/xfce4-power-manager/Makefile
  branches/2019Q2/sysutils/xfce4-power-manager/files/
Comment 11 Guido Falsi freebsd_committer 2019-05-15 09:49:33 UTC
Changes committed and merged to quarterly.

Thanks!