| Message ID | 20200214015343.201946-7-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,01/15] help: move list_config_help to builtin/help | expand |
Hi Emily, On Thu, 13 Feb 2020, Emily Shaffer wrote: > To help pinpoint the source of a regression, it is useful to know some > info about the compiler which the user's Git client was built with. By > adding a generic get_compiler_info() in 'compat/' we can choose which > relevant information to share per compiler; to get started, let's > demonstrate the version of glibc if the user built with 'gcc'. I agree with the need for the compiler information, but in the patch I only see information about glibc being printed out. Shouldn't we use `__GNUC__` and `__GNUC_MINOR__` here? Don't get me wrong, the glibc version is good and all, but the compiler information might be even more crucial. Git for Windows had to hold back compiling with GCC v8.x for a while, for example, because the stacksmasher was broken. Similar issues are not unheard of, and could help pinpoint compiler-related problems a lot quicker. Thanks, Dscho > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > Documentation/git-bugreport.txt | 1 + > bugreport.c | 5 +++++ > compat/compiler.h | 24 ++++++++++++++++++++++++ > 3 files changed, 30 insertions(+) > create mode 100644 compat/compiler.h > > diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt > index 4dd72c60f5..8bbc4c960c 100644 > --- a/Documentation/git-bugreport.txt > +++ b/Documentation/git-bugreport.txt > @@ -27,6 +27,7 @@ The following information is captured automatically: > > - 'git version --build-options' > - uname sysname, release, version, and machine strings > + - Compiler-specific info string > > OPTIONS > ------- > diff --git a/bugreport.c b/bugreport.c > index b76a1dfb2a..4f9101caeb 100644 > --- a/bugreport.c > +++ b/bugreport.c > @@ -4,6 +4,7 @@ > #include "strbuf.h" > #include "time.h" > #include "help.h" > +#include "compat/compiler.h" > > static void get_system_info(struct strbuf *sys_info) > { > @@ -24,6 +25,10 @@ static void get_system_info(struct strbuf *sys_info) > uname_info.release, > uname_info.version, > uname_info.machine); > + > + strbuf_addstr(sys_info, "compiler info: "); > + get_compiler_info(sys_info); > + strbuf_complete_line(sys_info); > } > > static const char * const bugreport_usage[] = { > diff --git a/compat/compiler.h b/compat/compiler.h > new file mode 100644 > index 0000000000..bda5098e1b > --- /dev/null > +++ b/compat/compiler.h > @@ -0,0 +1,24 @@ > +#ifndef COMPILER_H > +#define COMPILER_H > + > +#include "git-compat-util.h" > +#include "strbuf.h" > + > +#ifdef __GLIBC__ > +#include <gnu/libc-version.h> > + > +static inline void get_compiler_info(struct strbuf *info) > +{ > + strbuf_addf(info, "glibc: %s", gnu_get_libc_version()); > +} > + > +#else > + > +static inline void get_compiler_info(struct strbuf *info) > +{ > + strbuf_addstr(info, "get_compiler_info() not implemented"); > +} > + > +#endif > + > +#endif /* COMPILER_H */ > -- > 2.25.0.265.gbab2e86ba0-goog > >
On Wed, Feb 19, 2020 at 03:23:34PM +0100, Johannes Schindelin wrote: > Hi Emily, > > On Thu, 13 Feb 2020, Emily Shaffer wrote: > > > To help pinpoint the source of a regression, it is useful to know some > > info about the compiler which the user's Git client was built with. By > > adding a generic get_compiler_info() in 'compat/' we can choose which > > relevant information to share per compiler; to get started, let's > > demonstrate the version of glibc if the user built with 'gcc'. > > I agree with the need for the compiler information, but in the patch I > only see information about glibc being printed out. Shouldn't we use > `__GNUC__` and `__GNUC_MINOR__` here? > > Don't get me wrong, the glibc version is good and all, but the compiler > information might be even more crucial. Git for Windows had to hold back > compiling with GCC v8.x for a while, for example, because the stacksmasher > was broken. Similar issues are not unheard of, and could help pinpoint > compiler-related problems a lot quicker. Hm, sure. Good point - thanks. This does make me start to wonder, though - does it really make sense to have ifdef-gated redefinitions of the whole get_compiler_info() method like I do now? I wonder if it makes more sense to have only one definition, so we can write down everything we know regardless of which pieces are put together. My thinking is something like this - what if I am using glibc, but not a GNU compiler? (The GNU docs on __GCC__ indicate this is a situation that might occur - "a non-GCC compiler that claims to accept the GNU C dialects") Is there some striking reason not to implement this compat command thusly instead: #ifdef __GLIBC__ #include <gnu/libc-version.h> #endif static inline void get_compiler_info(struct strbuf *info) { #ifdef __GLIBC__ strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); #endif #ifdef __GNUC__ strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); #endif #ifdef _MSC_VER strbuf_addf(info, "msc runtime: %s\n", some_msc_info()); #endif } The thinking being - this way if I decide to use, say, LLVM + glibc, then I don't need to reimplement this command with all the glibc diagnostics again. Or, if someone else already wrote diagnostics for LLVM with some other libc, then it even Just Works for me and my new combination. That said, I'm reasoning about these combinations of compilers and libcs and whatever else from an inexperienced viewpoint, so maybe this isn't necessary? - Emily
Hi Emily, On Wed, 19 Feb 2020, Emily Shaffer wrote: > #ifdef __GLIBC__ > #include <gnu/libc-version.h> > #endif > > static inline void get_compiler_info(struct strbuf *info) > { > #ifdef __GLIBC__ > strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > #endif > > #ifdef __GNUC__ > strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > #endif > > #ifdef _MSC_VER > strbuf_addf(info, "msc runtime: %s\n", some_msc_info()); You could do it this way right away: strbuf_addf(info, "MSVC version: %d.%d\n", _MSC_VER / 100, _MSC_VER % 100); Note: this is _not_ the MSVC _runtime_ version, but really the compiler version. You could also use _MSC_FULL_VER, which is a bit more complete. > #endif > } > > The thinking being - this way if I decide to use, say, LLVM + glibc, > then I don't need to reimplement this command with all the glibc > diagnostics again. Or, if someone else already wrote diagnostics for > LLVM with some other libc, then it even Just Works for me and my new > combination. > > That said, I'm reasoning about these combinations of compilers and libcs > and whatever else from an inexperienced viewpoint, so maybe this isn't > necessary? I would have hoped that the argument I made earlier about the broken GCC version would have convinced you that the answer to this question is: Yes, this is necessary. Ciao, Dscho > > - Emily >
On Thu, Feb 20, 2020 at 11:33:05PM +0100, Johannes Schindelin wrote: > Hi Emily, > > On Wed, 19 Feb 2020, Emily Shaffer wrote: > > > #ifdef __GLIBC__ > > #include <gnu/libc-version.h> > > #endif > > > > static inline void get_compiler_info(struct strbuf *info) > > { > > #ifdef __GLIBC__ > > strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > > #endif > > > > #ifdef __GNUC__ > > strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > > #endif > > > > #ifdef _MSC_VER > > strbuf_addf(info, "msc runtime: %s\n", some_msc_info()); > > You could do it this way right away: > > strbuf_addf(info, "MSVC version: %d.%d\n", > _MSC_VER / 100, _MSC_VER % 100); > > Note: this is _not_ the MSVC _runtime_ version, but really the compiler > version. > > You could also use _MSC_FULL_VER, which is a bit more complete. Sorry, but I'm not comfortable sending code I can't check for myself (and already muscle-memoried into my format-patch/send-email workflow). If you send a scissors I can roll it into the series with your SOB. - Emily
Hi Emily, On Thu, 20 Feb 2020, Emily Shaffer wrote: > On Thu, Feb 20, 2020 at 11:33:05PM +0100, Johannes Schindelin wrote: > > Hi Emily, > > > > On Wed, 19 Feb 2020, Emily Shaffer wrote: > > > > > #ifdef __GLIBC__ > > > #include <gnu/libc-version.h> > > > #endif > > > > > > static inline void get_compiler_info(struct strbuf *info) > > > { > > > #ifdef __GLIBC__ > > > strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > > > #endif > > > > > > #ifdef __GNUC__ > > > strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > > > #endif > > > > > > #ifdef _MSC_VER > > > strbuf_addf(info, "msc runtime: %s\n", some_msc_info()); > > > > You could do it this way right away: > > > > strbuf_addf(info, "MSVC version: %d.%d\n", > > _MSC_VER / 100, _MSC_VER % 100); > > > > Note: this is _not_ the MSVC _runtime_ version, but really the compiler > > version. > > > > You could also use _MSC_FULL_VER, which is a bit more complete. > > Sorry, but I'm not comfortable sending code I can't check for myself > (and already muscle-memoried into my format-patch/send-email workflow). > If you send a scissors I can roll it into the series with your SOB. But you can check it yourself! I worked _really_ hard on that Azure Pipeline backing the PR builds at https://github.com/git/git. _REALLY_ hard. You might just as well reap the benefits so that I did not spend all of that time and sweat and stress in vain... Ciao, Dscho
On Fri, Feb 21, 2020 at 04:22:43PM +0100, Johannes Schindelin wrote: > Hi Emily, > > On Thu, 20 Feb 2020, Emily Shaffer wrote: > > > On Thu, Feb 20, 2020 at 11:33:05PM +0100, Johannes Schindelin wrote: > > > Hi Emily, > > > > > > On Wed, 19 Feb 2020, Emily Shaffer wrote: > > > > > > > #ifdef __GLIBC__ > > > > #include <gnu/libc-version.h> > > > > #endif > > > > > > > > static inline void get_compiler_info(struct strbuf *info) > > > > { > > > > #ifdef __GLIBC__ > > > > strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > > > > #endif > > > > > > > > #ifdef __GNUC__ > > > > strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > > > > #endif > > > > > > > > #ifdef _MSC_VER > > > > strbuf_addf(info, "msc runtime: %s\n", some_msc_info()); > > > > > > You could do it this way right away: > > > > > > strbuf_addf(info, "MSVC version: %d.%d\n", > > > _MSC_VER / 100, _MSC_VER % 100); > > > > > > Note: this is _not_ the MSVC _runtime_ version, but really the compiler > > > version. > > > > > > You could also use _MSC_FULL_VER, which is a bit more complete. > > > > Sorry, but I'm not comfortable sending code I can't check for myself > > (and already muscle-memoried into my format-patch/send-email workflow). > > If you send a scissors I can roll it into the series with your SOB. > > But you can check it yourself! I worked _really_ hard on that Azure > Pipeline backing the PR builds at https://github.com/git/git. _REALLY_ > hard. You might just as well reap the benefits so that I did not spend all > of that time and sweat and stress in vain... I thought a bit about this. From your Github-using point of view, "just check my Pipeline" sounds like "just look at one more thing". From my format-patch using point of view, "just check my Pipeline" sounds like "ugh, I have to add this remote again... I don't have a fork already? How do I make that? Or is my fork just 6 months behind? How do I open a PR again? Yeesh." So I did what any good developer who's supposed to be writing semi-annual performance reviews would, and wrote a first pass at script with this brand new fancy 'gh' thing (which is, by the way, clearly not intended for scripting): https://github.com/nasamuffin/quimby. Maybe someone will find it useful. I'll try and add your MSC snippet for the next rollup. - Emily P.S. You can thank Jonathan Nieder for the name, as I asked for ideas and he said "Wikipedia tells me Chief Quimby gives Inspector Gadget something to read that later self-destructs." :P
Emily Shaffer <emilyshaffer@google.com> writes: >> > Sorry, but I'm not comfortable sending code I can't check for myself >> > (and already muscle-memoried into my format-patch/send-email workflow). >> > If you send a scissors I can roll it into the series with your SOB. >> >> But you can check it yourself! I worked _really_ hard on that Azure >> Pipeline backing the PR builds at https://github.com/git/git. _REALLY_ >> hard. You might just as well reap the benefits so that I did not spend all >> of that time and sweat and stress in vain... > > I thought a bit about this. From your Github-using point of view, "just > check my Pipeline" sounds like "just look at one more thing". From my > format-patch using point of view, "just check my Pipeline" sounds like > "ugh, I have to add this remote again... I don't have a fork already? > How do I make that? Or is my fork just 6 months behind? How do I open a > PR again? Yeesh." Sorry, but this is how a typical conversation between two techies who are more intelligent than good for their social skills ;-) Each side is not extending its hand enough to reach the other side, expecting that what s/he knows should be obvious to the others. Dscho may be frustrated for you not being aware of what he worked on, but "I worked really hard" is much less useful thing for others, who may benefit from what he worked hard on, to hear, than "here is one pager that tells you how to use it". Quite honestly, reading the above exchange from the sidelines, it is not clear to me if it is a part of, or a completely separate from, the service offered by GGG from the end-users' point of view. Can some of you come up with a one-pager to cover the following (and related but not listed) areas, that can be added as an appendix to Documentation/SubmittingPatches? It does not have to cover the common stuff like style guidelines, writing good log messages, need for test coverage, choice of fork point, etc. "Now you have a patch or a series of patches, but you obviously do not have direct and personal access to all the platforms Git supports. But there are servies to help you test them on the more common ones." - How to cause TravisCI to run our test suite with your modification. - How to trigger the same on Azure Pipeline PR builds. There may be others. Thanks.
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index 4dd72c60f5..8bbc4c960c 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -27,6 +27,7 @@ The following information is captured automatically: - 'git version --build-options' - uname sysname, release, version, and machine strings + - Compiler-specific info string OPTIONS ------- diff --git a/bugreport.c b/bugreport.c index b76a1dfb2a..4f9101caeb 100644 --- a/bugreport.c +++ b/bugreport.c @@ -4,6 +4,7 @@ #include "strbuf.h" #include "time.h" #include "help.h" +#include "compat/compiler.h" static void get_system_info(struct strbuf *sys_info) { @@ -24,6 +25,10 @@ static void get_system_info(struct strbuf *sys_info) uname_info.release, uname_info.version, uname_info.machine); + + strbuf_addstr(sys_info, "compiler info: "); + get_compiler_info(sys_info); + strbuf_complete_line(sys_info); } static const char * const bugreport_usage[] = { diff --git a/compat/compiler.h b/compat/compiler.h new file mode 100644 index 0000000000..bda5098e1b --- /dev/null +++ b/compat/compiler.h @@ -0,0 +1,24 @@ +#ifndef COMPILER_H +#define COMPILER_H + +#include "git-compat-util.h" +#include "strbuf.h" + +#ifdef __GLIBC__ +#include <gnu/libc-version.h> + +static inline void get_compiler_info(struct strbuf *info) +{ + strbuf_addf(info, "glibc: %s", gnu_get_libc_version()); +} + +#else + +static inline void get_compiler_info(struct strbuf *info) +{ + strbuf_addstr(info, "get_compiler_info() not implemented"); +} + +#endif + +#endif /* COMPILER_H */
To help pinpoint the source of a regression, it is useful to know some info about the compiler which the user's Git client was built with. By adding a generic get_compiler_info() in 'compat/' we can choose which relevant information to share per compiler; to get started, let's demonstrate the version of glibc if the user built with 'gcc'. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/git-bugreport.txt | 1 + bugreport.c | 5 +++++ compat/compiler.h | 24 ++++++++++++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 compat/compiler.h