diff mbox series

[FFmpeg-devel] fftools/cmdutils: don't print build configuration by default

Message ID 20210806180419.8829-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] fftools/cmdutils: don't print build configuration by default
Related show

Checks

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

Commit Message

James Almer Aug. 6, 2021, 6:04 p.m. UTC
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(-)

Comments

Andreas Rheinhardt Aug. 6, 2021, 6:08 p.m. UTC | #1
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
James Almer Aug. 6, 2021, 6:27 p.m. UTC | #2
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.
Soft Works Aug. 6, 2021, 8:44 p.m. UTC | #3
> -----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
Derek Buitenhuis Aug. 7, 2021, 4:06 p.m. UTC | #4
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
James Almer Aug. 7, 2021, 4:13 p.m. UTC | #5
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".
>
Derek Buitenhuis Aug. 7, 2021, 4:17 p.m. UTC | #6
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
James Almer Aug. 7, 2021, 4:28 p.m. UTC | #7
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".
>
Carl Eugen Hoyos Aug. 8, 2021, 12:58 a.m. UTC | #8
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
Gyan Doshi Aug. 8, 2021, 4:31 a.m. UTC | #9
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
Stephen Hutchinson Aug. 8, 2021, 11:37 a.m. UTC | #10
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 mbox series

Patch

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;