Re: [PATCH v3 3/9] bugreport: add version and system information

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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