| Message ID | 20191025025129.250049-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
Hi Emily, On Thu, 24 Oct 2019, Emily Shaffer wrote: > diff --git a/bugreport.c b/bugreport.c > new file mode 100644 > index 0000000000..ada54fe583 > --- /dev/null > +++ b/bugreport.c > @@ -0,0 +1,55 @@ > +#include "cache.h" > + > +#include "bugreport.h" > +#include "help.h" > +#include "run-command.h" > +#include "version.h" > + > +void get_system_info(struct strbuf *sys_info) > +{ > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf std_out = STRBUF_INIT; > + > + strbuf_reset(sys_info); > + > + // get git version from native cmd Please use C-style comments instead of C++ ones. > + strbuf_addstr(sys_info, "git version: "); > + strbuf_addstr(sys_info, git_version_string); > + strbuf_complete_line(sys_info); > + > + // system call for other version info > + argv_array_push(&cp.args, "uname"); > + argv_array_push(&cp.args, "-a"); > + capture_command(&cp, &std_out, 0); Mmmkay. I am not too much of a fan of relying on the `uname` executable, as it can very well be a 32-bit `uname.exe` on Windows, which obviously would _not_ report the architecture of the machine, but something misleading. Why not use the `uname()` function (that we even define in `compat/mingw.c`)? Also, why not include the same information as `git version --build-options`, most importantly `GIT_HOST_CPU`? In any case, if you are capturing the output of `uname -a`, you definitely will want to pass the `RUN_COMMAND_STDOUT_TO_STDERR` flag (in case, say, the MSYS2 `uname.exe` fails with those dreaded Cygwin error messages like "*** fatal error - add_item ("\??\D:\git\installation\Git", "/", ...) failed, errno 1"). > + > + strbuf_addstr(sys_info, "uname -a: "); > + strbuf_addbuf(sys_info, &std_out); > + strbuf_complete_line(sys_info); > + > + argv_array_clear(&cp.args); > + strbuf_reset(&std_out); > + > + > + argv_array_push(&cp.args, "curl-config"); > + argv_array_push(&cp.args, "--version"); > + capture_command(&cp, &std_out, 0); This will be almost certainly be incorrect, as most _regular_ users won't have `curl-config`. I know, it is easy to forget that most Git users are no hard-core C developers ;-) A better alternative would be to use `curl_version()`, guarded, of course, by `#ifndef NO_CURL`... > + > + strbuf_addstr(sys_info, "curl-config --version: "); > + strbuf_addbuf(sys_info, &std_out); > + strbuf_complete_line(sys_info); > + > + argv_array_clear(&cp.args); > + strbuf_reset(&std_out); > + > + > + argv_array_push(&cp.args, "ldd"); > + argv_array_push(&cp.args, "--version"); > + capture_command(&cp, &std_out, 0); Again, this command will only be present in few setups. I am not actually sure that the output of this is interesting to begin with. What I _do_ think is that a much more interesting piece of information would be the exact GLIBC version, the OS name and the OS version. > + > + strbuf_addstr(sys_info, "ldd --version: "); > + strbuf_addbuf(sys_info, &std_out); > + strbuf_complete_line(sys_info); > + > + argv_array_clear(&cp.args); > + strbuf_reset(&std_out); > +} > diff --git a/bugreport.h b/bugreport.h > new file mode 100644 > index 0000000000..ba216acf3f > --- /dev/null > +++ b/bugreport.h > @@ -0,0 +1,7 @@ > +#include "strbuf.h" > + > +/** > + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info. > + * The previous contents of sys_info will be discarded. > + */ > +void get_system_info(struct strbuf *sys_info); > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > index 2ef16440a0..7232d31be7 100644 > --- a/builtin/bugreport.c > +++ b/builtin/bugreport.c > @@ -1,4 +1,5 @@ > #include "builtin.h" > +#include "bugreport.h" > #include "stdio.h" > #include "strbuf.h" > #include "time.h" > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template) > return 0; > } > > +void add_header(FILE *report, const char *title) > +{ > + struct strbuf buffer = STRBUF_INIT; > + strbuf_addf(&buffer, "\n\n[%s]\n", title); > + strbuf_write(&buffer, report); This leaks `buffer`. Why not write into `report` via `fprintf()` directly? Ciao, Dscho > +} > + > int cmd_bugreport(int argc, const char **argv, const char *prefix) > { > struct strbuf buffer = STRBUF_INIT; > @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > get_bug_template(&buffer); > strbuf_write(&buffer, report); > > + // add other contents > + add_header(report, "System Info"); > + get_system_info(&buffer); > + strbuf_write(&buffer, report); > + > fclose(report); > > launch_editor(report_path.buf, NULL, NULL); > -- > 2.24.0.rc0.303.g954a862665-goog > >
On 2019.10.24 19:51, Emily Shaffer wrote: [snip] > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > index 2ef16440a0..7232d31be7 100644 > --- a/builtin/bugreport.c > +++ b/builtin/bugreport.c > @@ -1,4 +1,5 @@ > #include "builtin.h" > +#include "bugreport.h" > #include "stdio.h" > #include "strbuf.h" > #include "time.h" > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template) > return 0; > } > > +void add_header(FILE *report, const char *title) I get the same compilation error here, can you make add_header() static as well?
On Mon, Oct 28, 2019 at 02:49:29PM +0100, Johannes Schindelin wrote: > Hi Emily, > > On Thu, 24 Oct 2019, Emily Shaffer wrote: > > > diff --git a/bugreport.c b/bugreport.c > > new file mode 100644 > > index 0000000000..ada54fe583 > > --- /dev/null > > +++ b/bugreport.c > > @@ -0,0 +1,55 @@ > > +#include "cache.h" > > + > > +#include "bugreport.h" > > +#include "help.h" > > +#include "run-command.h" > > +#include "version.h" > > + > > +void get_system_info(struct strbuf *sys_info) > > +{ > > + struct child_process cp = CHILD_PROCESS_INIT; > > + struct strbuf std_out = STRBUF_INIT; > > + > > + strbuf_reset(sys_info); > > + > > + // get git version from native cmd > > Please use C-style comments instead of C++ ones. > > > + strbuf_addstr(sys_info, "git version: "); > > + strbuf_addstr(sys_info, git_version_string); > > + strbuf_complete_line(sys_info); > > + > > + // system call for other version info > > + argv_array_push(&cp.args, "uname"); > > + argv_array_push(&cp.args, "-a"); > > + capture_command(&cp, &std_out, 0); > > Mmmkay. I am not too much of a fan of relying on the `uname` executable, > as it can very well be a 32-bit `uname.exe` on Windows, which obviously > would _not_ report the architecture of the machine, but something > misleading. > > Why not use the `uname()` function (that we even define in > `compat/mingw.c`)? Very glad to have your review and point this kind of thing out. :) It simplifies the code, too. I wasn't sure about these system calls, so I appreciate you suggesting alternatives. > > Also, why not include the same information as `git version > --build-options`, most importantly `GIT_HOST_CPU`? > > In any case, if you are capturing the output of `uname -a`, you > definitely will want to pass the `RUN_COMMAND_STDOUT_TO_STDERR` flag (in > case, say, the MSYS2 `uname.exe` fails with those dreaded Cygwin error > messages like "*** fatal error - add_item > ("\??\D:\git\installation\Git", "/", ...) failed, errno 1"). > > > + > > + strbuf_addstr(sys_info, "uname -a: "); > > + strbuf_addbuf(sys_info, &std_out); > > + strbuf_complete_line(sys_info); > > + > > + argv_array_clear(&cp.args); > > + strbuf_reset(&std_out); > > + > > + > > + argv_array_push(&cp.args, "curl-config"); > > + argv_array_push(&cp.args, "--version"); > > + capture_command(&cp, &std_out, 0); > > This will be almost certainly be incorrect, as most _regular_ users > won't have `curl-config`. I know, it is easy to forget that most Git > users are no hard-core C developers ;-) Heresy! :) > > A better alternative would be to use `curl_version()`, guarded, of > course, by `#ifndef NO_CURL`... > > > + > > + strbuf_addstr(sys_info, "curl-config --version: "); > > + strbuf_addbuf(sys_info, &std_out); > > + strbuf_complete_line(sys_info); > > + > > + argv_array_clear(&cp.args); > > + strbuf_reset(&std_out); > > + > > + > > + argv_array_push(&cp.args, "ldd"); > > + argv_array_push(&cp.args, "--version"); > > + capture_command(&cp, &std_out, 0); > > Again, this command will only be present in few setups. I am not > actually sure that the output of this is interesting to begin with. It was a suggestion, I believe, from Jonathan Nieder. > > What I _do_ think is that a much more interesting piece of information > would be the exact GLIBC version, the OS name and the OS version. The glibc version is easy; I've done that. It certainly looks nicer than the ldd call. I guess I may be missing something, because as I start to look into how to the OS info, I fall down a hole of many, many preprocessor defines to check. If that's the approach you want me to take, just say the word, but it will be ugly :) I suppose I had hoped the uname info would give us a close enough idea that full OS info isn't necessary. > > > + > > + strbuf_addstr(sys_info, "ldd --version: "); > > + strbuf_addbuf(sys_info, &std_out); > > + strbuf_complete_line(sys_info); > > + > > + argv_array_clear(&cp.args); > > + strbuf_reset(&std_out); > > +} > > diff --git a/bugreport.h b/bugreport.h > > new file mode 100644 > > index 0000000000..ba216acf3f > > --- /dev/null > > +++ b/bugreport.h > > @@ -0,0 +1,7 @@ > > +#include "strbuf.h" > > + > > +/** > > + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info. > > + * The previous contents of sys_info will be discarded. > > + */ > > +void get_system_info(struct strbuf *sys_info); > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > > index 2ef16440a0..7232d31be7 100644 > > --- a/builtin/bugreport.c > > +++ b/builtin/bugreport.c > > @@ -1,4 +1,5 @@ > > #include "builtin.h" > > +#include "bugreport.h" > > #include "stdio.h" > > #include "strbuf.h" > > #include "time.h" > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template) > > return 0; > > } > > > > +void add_header(FILE *report, const char *title) > > +{ > > + struct strbuf buffer = STRBUF_INIT; > > + strbuf_addf(&buffer, "\n\n[%s]\n", title); > > + strbuf_write(&buffer, report); > > This leaks `buffer`. Why not write into `report` via `fprintf()` > directly? Rather, to match the style of the rest of the builtin, modified get_header to add the header to a passed-in strbuf instead of modifying the file directly. > > Ciao, > Dscho > > > +} > > + > > int cmd_bugreport(int argc, const char **argv, const char *prefix) > > { > > struct strbuf buffer = STRBUF_INIT; > > @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > > get_bug_template(&buffer); > > strbuf_write(&buffer, report); > > > > + // add other contents > > + add_header(report, "System Info"); > > + get_system_info(&buffer); > > + strbuf_write(&buffer, report); > > + > > fclose(report); > > > > launch_editor(report_path.buf, NULL, NULL); > > -- > > 2.24.0.rc0.303.g954a862665-goog > > > > Based on Dscho's comments, each piece of system info is gathered differently enough that I will do each in an independent commit now.
Hi Emily, On Fri, 8 Nov 2019, Emily Shaffer wrote: > On Mon, Oct 28, 2019 at 02:49:29PM +0100, Johannes Schindelin wrote: > > > On Thu, 24 Oct 2019, Emily Shaffer wrote: > > > > > diff --git a/bugreport.c b/bugreport.c > > > new file mode 100644 > > > index 0000000000..ada54fe583 > > > --- /dev/null > > > +++ b/bugreport.c > > > [...] > > > + strbuf_addstr(sys_info, "curl-config --version: "); > > > + strbuf_addbuf(sys_info, &std_out); > > > + strbuf_complete_line(sys_info); > > > + > > > + argv_array_clear(&cp.args); > > > + strbuf_reset(&std_out); > > > + > > > + > > > + argv_array_push(&cp.args, "ldd"); > > > + argv_array_push(&cp.args, "--version"); > > > + capture_command(&cp, &std_out, 0); > > > > Again, this command will only be present in few setups. I am not > > actually sure that the output of this is interesting to begin with. > > It was a suggestion, I believe, from Jonathan Nieder. Yes, I guess Jonathan builds their Git locally, too. It _is_ easy to forget that most users find this too involved to even try. Nothing like reading through a bug tracker quite frequently to learn about the actual troubles actual users have :-) > > What I _do_ think is that a much more interesting piece of information > > would be the exact GLIBC version, the OS name and the OS version. > > The glibc version is easy; I've done that. It certainly looks nicer than > the ldd call. > > I guess I may be missing something, because as I start to look into how > to the OS info, I fall down a hole of many, many preprocessor defines to > check. If that's the approach you want me to take, just say the word, > but it will be ugly :) I suppose I had hoped the uname info would give us > a close enough idea that full OS info isn't necessary. We could go down the pre-processor route, but that would give us the OS name and version at build time, not at run time. I think we will be mostly interested in the latter, though. We might need to enhance our `uname()` emulation in `compat/mingw.c`, but I think we already have enough information there. When it comes to glibc, I think `gnu_get_libc_version()` would get us what we want. A trickier thing might be to determine whether we're actually linking against glibc; I do not want to break musl builds again, I already did that inadvertently when requiring `REG_STARTEND` back in the days. > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > > > index 2ef16440a0..7232d31be7 100644 > > > --- a/builtin/bugreport.c > > > +++ b/builtin/bugreport.c > > > @@ -1,4 +1,5 @@ > > > #include "builtin.h" > > > +#include "bugreport.h" > > > #include "stdio.h" > > > #include "strbuf.h" > > > #include "time.h" > > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template) > > > return 0; > > > } > > > > > > +void add_header(FILE *report, const char *title) > > > +{ > > > + struct strbuf buffer = STRBUF_INIT; > > > + strbuf_addf(&buffer, "\n\n[%s]\n", title); > > > + strbuf_write(&buffer, report); > > > > This leaks `buffer`. Why not write into `report` via `fprintf()` > > directly? > > Rather, to match the style of the rest of the builtin, modified > get_header to add the header to a passed-in strbuf instead of > modifying the file directly. Hmm. It makes the code less elegant in my opinion. I would rather either render the entire bug report into a single `strbuf` and then write it via `write_in_full()`, or use `fprintf()` directly. Ciao, Dscho
On Mon, Nov 11, 2019 at 02:48:13PM +0100, Johannes Schindelin wrote: > > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > > > > index 2ef16440a0..7232d31be7 100644 > > > > --- a/builtin/bugreport.c > > > > +++ b/builtin/bugreport.c > > > > @@ -1,4 +1,5 @@ > > > > #include "builtin.h" > > > > +#include "bugreport.h" > > > > #include "stdio.h" > > > > #include "strbuf.h" > > > > #include "time.h" > > > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template) > > > > return 0; > > > > } > > > > > > > > +void add_header(FILE *report, const char *title) > > > > +{ > > > > + struct strbuf buffer = STRBUF_INIT; > > > > + strbuf_addf(&buffer, "\n\n[%s]\n", title); > > > > + strbuf_write(&buffer, report); > > > > > > This leaks `buffer`. Why not write into `report` via `fprintf()` > > > directly? > > > > Rather, to match the style of the rest of the builtin, modified > > get_header to add the header to a passed-in strbuf instead of > > modifying the file directly. > > Hmm. It makes the code less elegant in my opinion. I would rather either > render the entire bug report into a single `strbuf` and then write it > via `write_in_full()`, or use `fprintf()` directly. Yeah, that sounds good. I will do that. :)
diff --git a/Makefile b/Makefile index 78767ecdde..045b22cb67 100644 --- a/Makefile +++ b/Makefile @@ -844,6 +844,7 @@ LIB_OBJS += bisect.o LIB_OBJS += blame.o LIB_OBJS += blob.o LIB_OBJS += branch.o +LIB_OBJS += bugreport.o LIB_OBJS += bulk-checkin.o LIB_OBJS += bundle.o LIB_OBJS += cache-tree.o diff --git a/bugreport.c b/bugreport.c new file mode 100644 index 0000000000..ada54fe583 --- /dev/null +++ b/bugreport.c @@ -0,0 +1,55 @@ +#include "cache.h" + +#include "bugreport.h" +#include "help.h" +#include "run-command.h" +#include "version.h" + +void get_system_info(struct strbuf *sys_info) +{ + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf std_out = STRBUF_INIT; + + strbuf_reset(sys_info); + + // get git version from native cmd + strbuf_addstr(sys_info, "git version: "); + strbuf_addstr(sys_info, git_version_string); + strbuf_complete_line(sys_info); + + // system call for other version info + argv_array_push(&cp.args, "uname"); + argv_array_push(&cp.args, "-a"); + capture_command(&cp, &std_out, 0); + + strbuf_addstr(sys_info, "uname -a: "); + strbuf_addbuf(sys_info, &std_out); + strbuf_complete_line(sys_info); + + argv_array_clear(&cp.args); + strbuf_reset(&std_out); + + + argv_array_push(&cp.args, "curl-config"); + argv_array_push(&cp.args, "--version"); + capture_command(&cp, &std_out, 0); + + strbuf_addstr(sys_info, "curl-config --version: "); + strbuf_addbuf(sys_info, &std_out); + strbuf_complete_line(sys_info); + + argv_array_clear(&cp.args); + strbuf_reset(&std_out); + + + argv_array_push(&cp.args, "ldd"); + argv_array_push(&cp.args, "--version"); + capture_command(&cp, &std_out, 0); + + strbuf_addstr(sys_info, "ldd --version: "); + strbuf_addbuf(sys_info, &std_out); + strbuf_complete_line(sys_info); + + argv_array_clear(&cp.args); + strbuf_reset(&std_out); +} diff --git a/bugreport.h b/bugreport.h new file mode 100644 index 0000000000..ba216acf3f --- /dev/null +++ b/bugreport.h @@ -0,0 +1,7 @@ +#include "strbuf.h" + +/** + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info. + * The previous contents of sys_info will be discarded. + */ +void get_system_info(struct strbuf *sys_info); diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 2ef16440a0..7232d31be7 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -1,4 +1,5 @@ #include "builtin.h" +#include "bugreport.h" #include "stdio.h" #include "strbuf.h" #include "time.h" @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template) return 0; } +void add_header(FILE *report, const char *title) +{ + struct strbuf buffer = STRBUF_INIT; + strbuf_addf(&buffer, "\n\n[%s]\n", title); + strbuf_write(&buffer, report); +} + int cmd_bugreport(int argc, const char **argv, const char *prefix) { struct strbuf buffer = STRBUF_INIT; @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) get_bug_template(&buffer); strbuf_write(&buffer, report); + // add other contents + add_header(report, "System Info"); + get_system_info(&buffer); + strbuf_write(&buffer, report); + fclose(report); launch_editor(report_path.buf, NULL, NULL);
Teach bugreport how to collect the Git, curl, and ldd versions, as well as the uname string. Learning the uname and versions in the user's environment will enable us to reproduce behavior reported by the user; the versions will also give us a good starting place while bisecting a newly introduced bug. Put this functionality in a library, so that other commands can easily include this debug info if necessary. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Makefile | 1 + bugreport.c | 55 +++++++++++++++++++++++++++++++++++++++++++++ bugreport.h | 7 ++++++ builtin/bugreport.c | 13 +++++++++++ 4 files changed, 76 insertions(+) create mode 100644 bugreport.c create mode 100644 bugreport.h