| Message ID | 20210806180419.8829-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] fftools/cmdutils: don't print build configuration by default | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
James Almer: > From: Matthieu Patou <mpatou@fb.com> > > Suggested-by: ffmpeg@fb.com > Signed-off-by: James Almer <jamrial@gmail.com> > --- > fftools/cmdutils.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 912e881174..fc58277df7 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1160,7 +1160,8 @@ static void print_program_info(int flags, int level) > av_log(NULL, level, "\n"); > av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT); > > - av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); > + if (flags & SHOW_CONFIG) > + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); > } > > static void print_buildconf(int flags, int level) > @@ -1203,7 +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef *options) > int show_version(void *optctx, const char *opt, const char *arg) > { > av_log_set_callback(log_callback_help); > - print_program_info (SHOW_COPYRIGHT, AV_LOG_INFO); > + print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, AV_LOG_INFO); > print_all_libs_info(SHOW_VERSION, AV_LOG_INFO); > > return 0; > This ensures that the configuration will be missing from most bugreports. - Andreas
On 8/6/2021 3:08 PM, Andreas Rheinhardt wrote: > James Almer: >> From: Matthieu Patou <mpatou@fb.com> >> >> Suggested-by: ffmpeg@fb.com >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> fftools/cmdutils.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c >> index 912e881174..fc58277df7 100644 >> --- a/fftools/cmdutils.c >> +++ b/fftools/cmdutils.c >> @@ -1160,7 +1160,8 @@ static void print_program_info(int flags, int level) >> av_log(NULL, level, "\n"); >> av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT); >> >> - av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); >> + if (flags & SHOW_CONFIG) >> + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); >> } >> >> static void print_buildconf(int flags, int level) >> @@ -1203,7 +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef *options) >> int show_version(void *optctx, const char *opt, const char *arg) >> { >> av_log_set_callback(log_callback_help); >> - print_program_info (SHOW_COPYRIGHT, AV_LOG_INFO); >> + print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, AV_LOG_INFO); >> print_all_libs_info(SHOW_VERSION, AV_LOG_INFO); >> >> return 0; >> > This ensures that the configuration will be missing from most bugreports. Yes, that was my main concern as well. But ffmpeg -version will still show the build configuration, in any case.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Friday, 6 August 2021 20:27 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build > configuration by default > > On 8/6/2021 3:08 PM, Andreas Rheinhardt wrote: > > James Almer: > >> From: Matthieu Patou <mpatou@fb.com> > >> > >> Suggested-by: ffmpeg@fb.com > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> fftools/cmdutils.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index > >> 912e881174..fc58277df7 100644 > >> --- a/fftools/cmdutils.c > >> +++ b/fftools/cmdutils.c > >> @@ -1160,7 +1160,8 @@ static void print_program_info(int flags, int > level) > >> av_log(NULL, level, "\n"); > >> av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT); > >> > >> - av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION > "\n", indent); > >> + if (flags & SHOW_CONFIG) > >> + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION > >> + "\n", indent); > >> } > >> > >> static void print_buildconf(int flags, int level) @@ -1203,7 > >> +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef > *options) > >> int show_version(void *optctx, const char *opt, const char *arg) > >> { > >> av_log_set_callback(log_callback_help); > >> - print_program_info (SHOW_COPYRIGHT, AV_LOG_INFO); > >> + print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, > AV_LOG_INFO); > >> print_all_libs_info(SHOW_VERSION, AV_LOG_INFO); > >> > >> return 0; > >> > > This ensures that the configuration will be missing from most bugreports. > > Yes, that was my main concern as well. But ffmpeg -version will still show the > build configuration, in any case. In my fork, I have a similar change, but I'm also printing the build config when loglevel >= VERBOSE. This largely addresses the bugreport concern as for those cases it will usually require a higher loglevel than INFO anyway. softworkz
On 8/6/2021 7:04 PM, James Almer wrote: > From: Matthieu Patou <mpatou@fb.com> > > Suggested-by: ffmpeg@fb.com > Signed-off-by: James Almer <jamrial@gmail.com> > --- > fftools/cmdutils.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) What exactly is the point of this? You've provided a patch with no explanation for its value. If it's "security", then that's kind of dumb, since if you have access to run ffmpeg on something, you have access to `strings $(which ffmpeg)` too. - Derek
On 8/7/2021 1:06 PM, Derek Buitenhuis wrote: > On 8/6/2021 7:04 PM, James Almer wrote: >> From: Matthieu Patou <mpatou@fb.com> >> >> Suggested-by: ffmpeg@fb.com >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> fftools/cmdutils.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) > > What exactly is the point of this? > > You've provided a patch with no explanation for its value. > > If it's "security", then that's kind of dumb, since if you > have access to run ffmpeg on something, you have access to > `strings $(which ffmpeg)` too. It's just to reduce the amount of information printed by default. Most command line utilities don't bother showing build time configuration options outside of --version or --help output, if at all. And the string is obviously in the binary (duplicated on every library at that), so it's not about hiding anything. I made the latest version print it on verbose level or higher (and always with --version), which may be preferable. But i nonetheless don't have a strong opinion about this change. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 8/7/2021 5:13 PM, James Almer wrote: > It's just to reduce the amount of information printed by default. Most > command line utilities don't bother showing build time configuration > options outside of --version or --help output, if at all. > And the string is obviously in the binary (duplicated on every library > at that), so it's not about hiding anything. I'm going to have to say this is actually negative value, and say I dislike it. I find it quite useful to print by default, in terms of debugging, finding issues in logs, etc. It also means (as noted elsewhere) that bug reports no longer include this info by default. So, I'm going to have to say this is not a good enough reason, in my opinion. > I made the latest version print it on verbose level or higher (and > always with --version), which may be preferable. But i nonetheless don't > have a strong opinion about this change. See above. - Derek
On 8/7/2021 1:17 PM, Derek Buitenhuis wrote: > On 8/7/2021 5:13 PM, James Almer wrote: >> It's just to reduce the amount of information printed by default. Most >> command line utilities don't bother showing build time configuration >> options outside of --version or --help output, if at all. >> And the string is obviously in the binary (duplicated on every library >> at that), so it's not about hiding anything. > > I'm going to have to say this is actually negative value, and say I dislike it. > > I find it quite useful to print by default, in terms of debugging, finding > issues in logs, etc. > > It also means (as noted elsewhere) that bug reports no longer include this > info by default. Bug reports are usually required to use debug loglevel, so they should not be affected with the latest version. > > So, I'm going to have to say this is not a good enough reason, in my opinion. Alright. > >> I made the latest version print it on verbose level or higher (and >> always with --version), which may be preferable. But i nonetheless don't >> have a strong opinion about this change. > > See above. > > - Derek > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Am Sa., 7. Aug. 2021 um 18:14 Uhr schrieb James Almer <jamrial@gmail.com>: > > On 8/7/2021 1:06 PM, Derek Buitenhuis wrote: > > On 8/6/2021 7:04 PM, James Almer wrote: > >> From: Matthieu Patou <mpatou@fb.com> > >> > >> Suggested-by: ffmpeg@fb.com > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> fftools/cmdutils.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > > > > What exactly is the point of this? > > > > You've provided a patch with no explanation for its value. > > > > If it's "security", then that's kind of dumb, since if you > > have access to run ffmpeg on something, you have access to > > `strings $(which ffmpeg)` too. > > It's just to reduce the amount of information printed by default. Most > command line utilities don't bother showing build time configuration > options outside of --version or --help output, if at all. > And the string is obviously in the binary (duplicated on every library > at that), so it's not about hiding anything. > > I made the latest version print it on verbose level or higher (and > always with --version), which may be preferable. But i nonetheless > don't have a strong opinion about this change. FWIW, the change is a very bad idea imo. Carl Eugen
On 2021-08-06 11:34 pm, James Almer wrote: > From: Matthieu Patou <mpatou@fb.com> > > Suggested-by: ffmpeg@fb.com > Signed-off-by: James Almer <jamrial@gmail.com> > --- > fftools/cmdutils.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 912e881174..fc58277df7 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -1160,7 +1160,8 @@ static void print_program_info(int flags, int level) > av_log(NULL, level, "\n"); > av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT); > > - av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); > + if (flags & SHOW_CONFIG) > + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); I like it. If most users pasted the full log on their own initiative in bug reports or Q&As then this would have been a negative but they don't. I suggest to add an else clause that prints something like "Add -buildconf to view configuration". Regards, Gyan
On 8/8/2021 12:31 AM, Gyan Doshi wrote: > > > On 2021-08-06 11:34 pm, James Almer wrote: >> From: Matthieu Patou <mpatou@fb.com> >> >> Suggested-by: ffmpeg@fb.com >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> fftools/cmdutils.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c >> index 912e881174..fc58277df7 100644 >> --- a/fftools/cmdutils.c >> +++ b/fftools/cmdutils.c >> @@ -1160,7 +1160,8 @@ static void print_program_info(int flags, int >> level) >> av_log(NULL, level, "\n"); >> av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT); >> - av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION >> "\n", indent); >> + if (flags & SHOW_CONFIG) >> + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION >> "\n", indent); > > I like it. If most users pasted the full log on their own initiative in > bug reports or Q&As then this would have been a negative but they don't. > > I suggest to add an else clause that prints something like "Add > -buildconf to view configuration". > Fun fact, the original -buildconf patchset included¹ that as an option, by letting users opt to disable the configuration: line entirely, whereby it would notify them to use -buildconf. I have kept it rebased over the years in my github repo². ¹http://ffmpeg.org/pipermail/ffmpeg-devel/2013-November/151075.html ²https://github.com/qyot27/FFmpeg/commit/ce087ae0b72cf70fff29300b528122b7451cb4b7
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 912e881174..fc58277df7 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1160,7 +1160,8 @@ static void print_program_info(int flags, int level) av_log(NULL, level, "\n"); av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT); - av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); + if (flags & SHOW_CONFIG) + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); } static void print_buildconf(int flags, int level) @@ -1203,7 +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef *options) int show_version(void *optctx, const char *opt, const char *arg) { av_log_set_callback(log_callback_help); - print_program_info (SHOW_COPYRIGHT, AV_LOG_INFO); + print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, AV_LOG_INFO); print_all_libs_info(SHOW_VERSION, AV_LOG_INFO); return 0;