Created attachment 175908 [details] x11-toolkits/girara Hello, I have finally tracked down the cause of long-existing bug (which developers have been treating silently for about a year now, tho) with graphics/zathura randomly missing keystrokes in command mode. It does not like being compiled with Clang. Perhaps some #ifdef's are the wrong way. Regards, Mike
Created attachment 175909 [details] portlint -AC
Created attachment 175910 [details] poudriere testport
I forgot to mention that as of FreeBSD 11, if girara is compiled with clang, Zathura will ignore all input in command mode, not random keystrokes as it has been the case for a long time.
Hi, It's a strange bug and I'd like to avoid forcing gcc on this port. I'm not refusing I just need to have a look at it before accepting this change. Can you explain to me how to reproduce the original bug please? Thanks in advance.
(In reply to Michael Danilov from comment #3) > I forgot to mention that as of FreeBSD 11, if girara is compiled with clang, > Zathura will ignore all input in command mode, not random keystrokes as it > has been the case for a long time. I see this after my previous post, I'll try it and report back soon.
(In reply to Guido Falsi from comment #5) > (In reply to Michael Danilov from comment #3) > > I forgot to mention that as of FreeBSD 11, if girara is compiled with clang, > > Zathura will ignore all input in command mode, not random keystrokes as it > > has been the case for a long time. > > I see this after my previous post, I'll try it and report back soon. I have reproduced the issue on head (FreeBSD 12). I'm testing the proposed fix.
One additional detail: if you press tab after entering command mode, keystrokes aren't ignored. Just tested the fix with GCC 6.2.0. Thanks, Micahel! : )
Thanks for the investigation and the fix Michael! Are zathura/girara developers aware of this? I don't find any issue related to that on their issue tracker. It would be better to avoid adding a dependency to GCC, but I accept the fix, as this seems to be the only solution for now.
Damn, I should have known they have one! Not listed in https://pwmt.org/contact/, had to search online. I will try to create a bug there. I have appeared with the most detailed description of the problem, on their IRC, a number of times over the last -- what, 3 years or so -- and every time they either yawned out a link to some irrelevant patch or pretended I don't exist. Perhaps they have never heard of BSD anyway... And only yesterday it occured to me that I should try different compiler.
https://bugs.pwmt.org/project/girara/issue/24
Thanks for filing the bug report upstream. Let's hope they can shed some light. First of all at present I am unable to say if the bug is in clang, girara, zathura, or either any other involved library. Compiling with gcc 4.9 (since 4.8 is failing to compile girara in my tests, for now) works around the problem, and I'm testing with it as a stop gap solution, but it's not a fix for the bug, wherever it may happen to actually live. I don't like the idea of adding a dependency on gcc, but at present it's the only solution we have, so my testing is moving in that direction, please give me a little time, I'm trying to make it work with "USE_GCC=yes" instead of forcing gcc 4.9.
Can you leave an option (maybe disabled) to compile girara with clang ? I'd like to avoid gcc on my system and don't want to switch to atril-lite.
(In reply to Ivan from comment #12) > Can you leave an option (maybe disabled) to compile girara with clang ? I'd > like to avoid gcc on my system and don't want to switch to atril-lite. I will try to add an option if it does not make the port too complicated. As a side note I'd like to explain that options don't come for free, they require some testing and care too, making maintenance for a port more burdensome. In this case it's not a big burden, but please keep this in mind when asking for more options in general.
In fact, I don't even know what to call the option. Calling it "GCC" or "CLANG" is plainly wrong for two reasons: 9.x uses GCC by default anyway, The tier2 architectures are using gcc mostly (except ARM) anyway. In fact it's not easy to have such control on which compiler will be used. Naming the option "BASE" for base compiler in this case is wrong too, because due to the USES=compiler:c11 it will be using ports provided gcc in most cases. the option has to be descriptive and short, and cannot be "AVOIDGCC" (or something like that) because I cannot be really sure that some tier2 architecture will be using GCC anyway. I'm open to suggestions while I think about this. But we cannot think only about amd64 and i386 and ignore the other architectures.
(In reply to Guido Falsi from comment #14) > In fact, I don't even know what to call the option. Calling it "GCC" or > "CLANG" is plainly wrong for two reasons: I'm partially wrong here. Maybe GCC is ok, I cannot force it to not use GCC on all architectures and versions but I can force using GCC from ports everywhere, or otherwise whatever the OS has available.
Is girara compiled with gcc the only component which fixes the issue? I looked into it and girara is not about megabytes of source code, maybe it's worth to try to instrument the nature of the problem first?
(In reply to Ivan from comment #16) > Is girara compiled with gcc the only component which fixes the issue? I > looked into it and girara is not about megabytes of source code, maybe it's > worth to try to instrument the nature of the problem first? As I aid as is it could be a bug in zathura, girara clang or any other involved library. This fixes the problem and it's a good solution in the immediate. Patches are always welcome though. P.S. I'm adding options to avoid GCC whenever that is possible (on amd64 and i386 at least)
A commit references this bug: Author: madpilot Date: Wed Oct 19 21:58:13 UTC 2016 New revision: 424292 URL: https://svnweb.freebsd.org/changeset/ports/424292 Log: - Add GCC option, turned on by default, to force compilation using GCC, this fixes a problem in zathura which shows up only when girara is compiled wth clang [1] - Make the port use gnu99 C standard to allow it to compile with the default gcc 4.8 - Remove USES=compiler since it's not needed anymore - Small cosmetic change to the REINPLACE_CMD lines - While here, make the build verbose PR: 213595 Submitted by: Michael Danilov <mike.d.ft402@gmail.com> MFH: 2016Q4 Changes: head/x11-toolkits/girara/Makefile
I committed the proposed fix with some changes, also adding an option to disable the forced GCC use. I'll leave this bug report in the open state, at least while we wait for some feedback on the upstream bug report.
A commit references this bug: Author: madpilot Date: Thu Oct 20 06:44:22 UTC 2016 New revision: 424308 URL: https://svnweb.freebsd.org/changeset/ports/424308 Log: MFH: r424292 - Add GCC option, turned on by default, to force compilation using GCC, this fixes a problem in zathura which shows up only when girara is compiled wth clang [1] - Make the port use gnu99 C standard to allow it to compile with the default gcc 4.8 - Remove USES=compiler since it's not needed anymore - Small cosmetic change to the REINPLACE_CMD lines - While here, make the build verbose PR: 213595 Submitted by: Michael Danilov <mike.d.ft402@gmail.com> Approved by: ports-secteam (feld) Changes: _U branches/2016Q4/ branches/2016Q4/x11-toolkits/girara/Makefile
I recompiled girara with debug symbols and to my surprise the issue has gone. Disable optimization has positive effects as well. Just passing -O0 flag is enough. I'll look further.
Created attachment 175968 [details] disable clang optimization for girara_callback_inputbar_key_press_event Hack for tests.
(In reply to Ivan from comment #21) > I recompiled girara with debug symbols and to my surprise the issue has gone. > Disable optimization has positive effects as well. > > Just passing -O0 flag is enough. I'll look further. I'll see to add this information to the upstream bug report, maybe they can have some more insight. While you're there, please point me to the function or art of code actually involved with grabbing keystrokes in girara, if you find that.
(In reply to Guido Falsi from comment #23) Not digged so deep, I focused on callbacks first as they receive events with keys already up there. Is patch works for you?
Works for me!
(In reply to Ivan from comment #22) > Created attachment 175968 [details] > disable clang optimization for girara_callback_inputbar_key_press_event > > Hack for tests. I did not see this patch before replying. I'm going to test it too. Will report back.
(In reply to Guido Falsi from comment #26) > (In reply to Ivan from comment #22) > > Created attachment 175968 [details] > > disable clang optimization for girara_callback_inputbar_key_press_event > > > > Hack for tests. > > I did not see this patch before replying. > > I'm going to test it too. > > Will report back. I modified the patch to use #defines dependent on the compiler, I'm going to commit this and remove the strict requirement on GCC.
A commit references this bug: Author: madpilot Date: Fri Oct 21 09:11:52 UTC 2016 New revision: 424401 URL: https://svnweb.freebsd.org/changeset/ports/424401 Log: Add patch to disable optimizations on a keyboard input function when using the clang compiler. This allows removing the requirement on gcc. Patch based on suggestion from Ivan <bsd@abinet.ru>. Bug has been reported upstream. PR: 213595 Submitted by: Michael Danilov <mike.d.ft402@gmail.com> MFH: 2016Q4 Changes: head/x11-toolkits/girara/Makefile head/x11-toolkits/girara/files/patch-girara_callbacks.h head/x11-toolkits/girara/files/patch-girara_macros.h
(In reply to Guido Falsi from comment #27) > I modified the patch to use #defines dependent on the compiler, I'm going to > commit this and remove the strict requirement on GCC. Good job, Guido. Thanks.
A commit references this bug: Author: madpilot Date: Sat Oct 22 04:56:15 UTC 2016 New revision: 424444 URL: https://svnweb.freebsd.org/changeset/ports/424444 Log: MFH: r424401 Add patch to disable optimizations on a keyboard input function when using the clang compiler. This allows removing the requirement on gcc. Patch based on suggestion from Ivan <bsd@abinet.ru>. Bug has been reported upstream. PR: 213595 Submitted by: Michael Danilov <mike.d.ft402@gmail.com> Approved by: ports-secteam (feld) Changes: _U branches/2016Q4/ branches/2016Q4/x11-toolkits/girara/Makefile branches/2016Q4/x11-toolkits/girara/files/patch-girara_callbacks.h branches/2016Q4/x11-toolkits/girara/files/patch-girara_macros.h
Thank you all for this patch. Hopefully this will be fixed upstream so that other systems (in particular, Mac OS X) can benefit from it as well. Yesterday, I tried installing the *binary* package of girara and it wanted to pull-in gcc. Alternately, I built the package (without gcc) from ports and then installed this package (on a separate machine). The binary package needs updating, but in any case, why would it depend on gcc & co. once compiled? Thank you all again. Alan
(In reply to Alan Braslau from comment #31) > Thank you all for this patch. Hopefully this will be fixed upstream so that > other systems (in particular, Mac OS X) can benefit from it as well. > > Yesterday, I tried installing the *binary* package of girara and it wanted > to pull-in gcc. Alternately, I built the package (without gcc) from ports > and then installed this package (on a separate machine). > > The binary package needs updating, but in any case, why would it depend on > gcc & co. once compiled? > Regarding why a package built sith gcc requires gcc: gcc itself provides some shared libraries, depending on the compiler/language features used by the compiled software it could add a dependency on those, so packages using a ports provided gcc for compilation also have a run time dependency on it. You could check using ldd if girara actually links to any gcc provided shared library and, in case it does not, "pkg delete -f gcc". I've done this on some systems in the past. Anyway I committed the patch and also merge it in the quarterly branch. I also bumped the PORTREVISION each time. The packages are rebuilt periodically, I think once or twice a week, I don't know the details, but it could take a few days for the official repositories to get the updated package. Please try again in a few days, and you will find an official package with no gcc dependency.
Marking this bug fixed since we have a working patch in the tree and the upstream bug report was closed stating it is fixed in the head of their sources, so the next update will not show this buggy behaviour. Thanks all for reporting and help solving and debugging this problem.