Bug 236438 - [PATCH] x11-fm/thunar: add debug option
Summary: [PATCH] x11-fm/thunar: add debug option
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: easy
Depends on:
Blocks:
 
Reported: 2019-03-10 04:17 UTC by rozhuk.im
Modified: 2019-05-10 17:47 UTC (History)
2 users (show)

See Also:
madpilot: maintainer-feedback+


Attachments
add options (663 bytes, patch)
2019-03-10 04:17 UTC, rozhuk.im
no flags Details | Diff
install-strip only if no debug (796 bytes, patch)
2019-03-17 20:28 UTC, rozhuk.im
no flags Details | Diff
patch (796 bytes, patch)
2019-05-02 21:12 UTC, rozhuk.im
no flags Details | Diff
Revised patch (446 bytes, patch)
2019-05-10 13:38 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-03-10 04:17:20 UTC
Created attachment 202764 [details]
add options

Add DEBUG option
Comment 1 rozhuk.im 2019-03-17 20:28:39 UTC
Created attachment 202941 [details]
install-strip only if no debug
Comment 2 Guido Falsi freebsd_committer 2019-03-17 21:36:45 UTC
Hi,

Thanks for the patch, I had a look. While I do agree with adding DEBUG support, I think it should be done the "proper" way.

In the ports tree there is support for the "WITH_DEBUG" flag, which automatically disables optimizations, adds the -g flag and disables stripping of binaries(or at least tries to).

If a port needs further flags they should be linked to this flag.

I have also discovered that DEBUG options are not a good idea [1]

So I'd rework this patch to not add a DEBUG option to the port but use the WITH_DEBUG (which is also triggere by WITH_DEBUG_PORTS) flag to check if extrra options should be enabled.

I'd try to avoid a DEBUG option. Adding WITH_DEBUG_PORTS to make.conf is quite easy.


[1] https://reviews.freebsd.org/D15773
Comment 3 rozhuk.im 2019-03-17 21:48:57 UTC
(In reply to Guido Falsi from comment #2)

Adding WITH_DEBUG_PORTS has less usability, IMHO, but ok.
Review has no progress for a long time. :(
Same changes can be done for devel/glib20.
Comment 4 Guido Falsi freebsd_committer 2019-03-17 23:14:11 UTC
(In reply to rozhuk.im from comment #3)
> (In reply to Guido Falsi from comment #2)
> 
> Adding WITH_DEBUG_PORTS has less usability, IMHO, but ok.

Depends. If you want a bunch of ports with debugging symbols it's easier to use that than set DEBUG option in them.

Anyway, even if providing an options we need to hook with WITH_DEBUG framework.

> Review has no progress for a long time. :(

The review I referenced is abandoned, since I got negative feedback and was convinced by the arguments there.

> Same changes can be done for devel/glib20.

Yes...well...I've been debugging both thunar and glib20 using "WITH_DEBUG" as is and did get useful backtraces...


Anyway, will you be providing a revised patch? Otherwise I can cook it up, but not right away.
Comment 5 rozhuk.im 2019-05-02 21:12:28 UTC
Created attachment 204173 [details]
patch

Now it check DEBUG option and WITH_DEBUG env var.
Comment 6 Guido Falsi freebsd_committer 2019-05-03 14:37:41 UTC
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.
Comment 7 rozhuk.im 2019-05-09 00:10:19 UTC
(In reply to Guido Falsi from comment #6)
--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.
Comment 8 Guido Falsi freebsd_committer 2019-05-09 07:06:42 UTC
(In reply to rozhuk.im from comment #7)
> (In reply to Guido Falsi from comment #6)
> --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.

Same reply as in bug 237714
Comment 9 Guido Falsi freebsd_committer 2019-05-10 13:38:40 UTC
Created attachment 204313 [details]
Revised patch

Revised patch attached.

I simply removed the options knob, and leverage WITH_DEBUG only.

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 10 commit-hook freebsd_committer 2019-05-10 17:46:58 UTC
A commit references this bug:

Author: madpilot
Date: Fri May 10 17:45:55 UTC 2019
New revision: 501189
URL: https://svnweb.freebsd.org/changeset/ports/501189

Log:
  Enable extra debugging code when building binaries with debugging symbols.

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

Changes:
  head/x11-fm/thunar/Makefile
Comment 11 Guido Falsi freebsd_committer 2019-05-10 17:47:12 UTC
Committed. Thanks!