brndnmtthws / conky

Light-weight system monitor for X, Wayland (sort of), and other things, too
https://conky.cc
GNU General Public License v3.0
7.17k stars 615 forks source link

1.10.0 - "execgraph" not work #153

Closed akorop closed 6 years ago

akorop commented 8 years ago

"execgraph" command always fail with error message: conky: reading exec value failed (perhaps it's not the correct format?) For example: ${execgraph echo 50}. The same command work fine with conky 1.9

akorop commented 8 years ago

Wow! It's long been known bug. See http://sourceforge.net/p/conky/bugs/395/

vyrus714 commented 8 years ago

I've had this ever since upgrading to the newer version. Would be nice to get my ping graph back, i've had to settle for an execbar instead - but that hardly tells me anything.

akorop commented 8 years ago

IMHO the original patch from sourceforge is a bit destructive (it drop the error message). I propose more delicate patch:

diff --git a/src/exec.cc b/src/exec.cc
index 10f47d8..2eaf6fd 100644
--- a/src/exec.cc
+++ b/src/exec.cc
@@ -220,7 +220,7 @@ void scan_execi_gauge_arg(struct text_object *obj, const char *arg)
        return;
    }
    ed->cmd = buf;
-   obj->data.opaque = ed;
+   obj->data.s = strndup(arg ? arg : "", text_buffer_size.get(*state));
 }
 #endif /* BUILD_X11 */
vyrus714 commented 8 years ago

Have you tested that? If it does work, i'll probably go and compile a copy for myself until its fixed in the main branch. There doesnt seem to be a whole lot of activity though.

akorop commented 8 years ago

Yes, of course. I use it. And the patch from issue 158 I use also.

Ceipheed commented 8 years ago

Please excuse me if it seems silly, but I'm having the same issue and don't know how to apply akorop's patch. Using Arch Linux.

akorop commented 8 years ago

Under Arch Linux Your may simple install the "conky-mt" AUR package

Ceipheed commented 8 years ago

tried (and failed) installing with yaourt and got this message:

CMake Error at cmake/ConkyPlatformChecks.cmake:29 (check_include_files): Unknown CMake command "check_include_files". Call Stack (most recent call first): CMakeLists.txt:37 (include)

-- Configuring incomplete, errors occurred!

akorop commented 8 years ago

Sorry, I don't track some updates. Fixed. Reinstall conky-mt, please.

Ceipheed commented 8 years ago

that did it. thanks.

txtsd commented 8 years ago

@akorop Can you create a PR for your changes?

akorop commented 8 years ago

What is PR?

txtsd commented 8 years ago

@akorop Pull Request

akorop commented 8 years ago

About official git version (brndnmtthws/conky). Now (2016 feb 12) it works strange a bit. Conky call now the specified program and pass the argument if double quoted, but it pass correctly one argument only. Exapmple: ${execgraph "echo 50"} This works fine

${execgraph "s 5 0"} where s is the script: echo $1$2 This dos not works

marcpayne commented 8 years ago

Hmm, I will take a look at this later today or tomorrow. Thanks for the report.

marcpayne commented 8 years ago

@akorop Confirmed. This has actually been an issue for quite a while, apparently. execgraph and other graph commands take height,width color1 color2 scale (and subsets of these) as numerical arguments. The command "s 5 0" is interpreted as "s followed by color1 = 5 and color2 = 0. Note the leading mismatched double-quote.

As a workaround, can you try surrounding your arguments by single-quotes?

${execgraph "s '5' '0'"}
akorop commented 8 years ago

Oh... Where is my sweet conky 1.9? :) And where I (and other users) can read about these insane tricks?

marcpayne commented 8 years ago

Yeah, a lot changed under the hood since 1.9...

The workaround, and my reasoning leading up to it, comes from reading through the scan_graph() function in specials.cc to understand how the arguments are parsed, skimming the sscanf man page, and knowing that exec objects use sh -c to run commands (see exec.cc).

There isn't any one source of such tricks that I know of. It just comes from experience reading code and doing some shell scripting. I knew that single-quotes cause whatever is inside them to be treated literally, and that quoting a number would essentially have no effect for the interpreter (sh). The sscanf calls look specifically for numbers, not numbers surrounded by quotes (I needed something that would cause all the sscanf calls to fail). These two tidbits of information came together to form the workaround.

To be honest, it took me many, many hours to learn what I know about Conky's code, and I did so by jumping in, tracing function calls, inserting tons of printf() and cout statements to see what is going on, etc. A good C++ book and a bash scripting guide can be very useful. Oh, and patience! Lots of patience. :)

marcpayne commented 8 years ago

Sidenote: ${execgraph "echo 50"} happens to partially match a sscanf call, which sets the height of the graph frame to 50px. The command echo 50 is correctly parsed at the end, so there are no errors and the graph value is a constant 50. Likewise for any integer from 0 to 100... :expressionless:

akorop commented 8 years ago

it took me many, many hours to learn what I know about Conky's code

And now each user must do it? I tried an example from "man conky": ${execgraph "date +'%S'"} - it not works. How to make conky pass parameters correctly and without side effects - it is not understandable (or impossible). IMHO such code has no right to exist. Perhaps it will be right after the writing of adequate documentation. But, perhaps, such code behavior will be unacceptable even if the documentation will be written. Conky script language must be simple and clear, otherwise it not make sense.

plikhari commented 8 years ago

I think that is a bit harsh ! We must give thanks and appreciate the time and effort people have put in to get us this fantastic program. In every program there are issues which do not get noticed even with best of minds testing and using it. We have been given a fantastic tool add-on called Lua. There are so many things that can be improved, extended or corrected. The choice is with you:

or

akorop commented 8 years ago

The choice is with you: Learn the C and C++ that is required Understand the underlying code - this is always the beast of the issue or Learn Lua and get on with it

I'm professional programmer and I understand the difference between a program (and documentation) for the normal users, program for several special users and program for authors only. Conky become the program for the authors only?

marcpayne commented 8 years ago

@akorop I understand and share your frustration. When I said I spent so many hours learning the code, I didn't mean to imply that every user should do it, and it certainly should not be necessary for users to do so in order to use Conky. My intention was to illustrate -- to anyone who happens to read it -- what kind of work is sometimes necessary to identify the source of a problem and come up with a workaround (until a proper fix is made). Nothing else was implied.

But, perhaps, such code behavior will be unacceptable even if the documentation will be written.

Yes, I agree with this statement completely. At best, workarounds like I suggested should be documented as known issues until the code is fixed. The code in scan_graph() needs to be rewritten, in my opinion. When I did my "exec refactor," I had actually started on it, but it turned out to be a bigger job than I anticipated and I didn't have the time to come up with a proper solution (I am only a hobbyist, so the learning curve is usually steep for me).

Regarding the date example in the man page, I just tried it and it works fine for me.

ldsh commented 8 years ago

Before, it was possible to have syntaxes like:

${execgraph -t echo "$(cat /tmp/dataintextfile.txt | cut -c16-20)*7"| bc -l}
${execgraph -t sensors | grep 'Core 0' | paste -s | cut -c16-21}
${execgraph -t .i7zdaemon/get-i7z-value freq --percent -c 0}

Now, none of these works any-more (currently using 1.10.1 from ubuntu repos).

I tried to add some ", but without success. Even removing the -t option does not help. The ${execgraph -t "echo 50"} is not working either.

No way to have the execgraph with this version?

The execi works like before, so there may be a need for syntax consistency?

marcpayne commented 8 years ago

@ldsh There were several issues with the exec system up to version 1.10.1, unfortunately. The fixes landed in version 1.10.2.

Until you are able to update, you can get your execgraph commands working by placing each one into a separate script and calling the script instead. I would also recommend placing the -t and -l options at the end. For example, you might do the following to run your sensors command:

${execgraph ~/bin/cputemp.sh -t}

The file ~/bin/cputemp.sh (or whatever you choose to name it) could look like this, assuming you use bash:

#!/bin/bash
sensors | grep 'Core 0' | paste -s | cut -c16-21

When you update to 1.10.2, you will not have to use separate scripts anymore. However, commands with spaces will need to be double-quoted and the -t and -l options should still come at the end.

${execgraph "echo '$(cat /tmp/dataintextfile.txt | cut -c16-20)*7'| bc" -l -t}
${execgraph "sensors | grep 'Core 0' | paste -s | cut -c16-21" -t}
${execgraph ".i7zdaemon/get-i7z-value freq --percent -c 0" -t}

Sorry for all the inconvenience. Please let us know if you have any further issues or you need some clarification. Regarding syntax consistency, there is a discussion in #246.

ldsh commented 8 years ago

@marcpayne Thank you for your support.

Good to know it will be better with next conky update.

If I run

echo '$(cat /tmp/dataintextfile.txt | cut -c16-20)*7'| bc -l

In the terminal, it does not work correctly. Seems the double quotes are required. So I doubt it will work seamlessly for that command. As I understand, I'll have to find an other workaround to retrieve that value (as the double quote will be needed by conky 1.10.2). [EDIT] I found out that the quotes are not needed inside the command, so I guess this will work:

${execgraph "echo $(cat /tmp/dataintextfile.txt | cut -c16-20)*7| bc -l" -t)

[\EDIT]

(note, the -l was an option to bc to tell to use the math routines, not the conky option to use log scale).

ldsh commented 8 years ago

As an other workaround, I used the double quotes plus execigraph which seems to behave like the execgraph will in version 1.10.2.

${execigraph "echo $(cat /tmp/dataintextfile.txt | cut -c16-20)*7| bc -l" -t 1)

This gives the graph, but the parameter default_graph_width = 190, doesn't seem to be taken into account. Instead, it fills up to the right side of the window.

marcpayne commented 8 years ago

Yeah, execigraph and execgraph did have different behavior, but this has been corrected in 1.10.2. Same for the default_graph_* parameters. :)

Regarding your command above, I didn't test before I posted my comment. The single quotes make everything inside literal, which I overlooked.

Thank you for the reports!

DanyGee commented 7 years ago

I tried your workarounds but they seem not to work.

My script to show GPU usage: nvidiagraf.sh

#!/bin/bash
nvidia-smi | grep % | cut -c 62-63

In terminal, the script works:

~/Dokumenty/Conky $ ./nvidiagraf.sh
 5

Line from my .conkyrc

${execigraph 20,85 "~/Dokumenty/Conky/nvidiagraf.sh" -1 -t}

It draws the graph border, but not the graph wave. What am I doing wrong?

lasers commented 6 years ago

Fixed via https://github.com/brndnmtthws/conky/pull/192. Many thanks @marcpayne for this work.

Merged 3 years, 9 months, 18 days after the issue on SourceForge. Merged 2 months, 18 days after this report. Closing this issue 2 years, 7 months, 7 days after the merge. Closing this issue after 2 years, 9 months, 27 days. Somebody ought to close the issue on SourceForge.