[FFmpeg-devel] [PATCHv2] fate: Add an option for disabling the 2k/4k tests

[James Almer]

On 12/13/2019 8:07 PM, Martin Storsjö wrote:
> On Fri, 13 Dec 2019, James Almer wrote:
> 
>> On 12/13/2019 7:22 PM, Martin Storsjö wrote:
>>> On Fri, 13 Dec 2019, Carl Eugen Hoyos wrote:
>>>
>>>>
>>>>
>>>>> Am 13.12.2019 um 09:34 schrieb Martin Storsjö <martin at martin.st>:
>>>>>
>>>>>> On Thu, 12 Dec 2019, James Almer wrote:
>>>>>>
>>>>>>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>>>>>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>>>>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>>>>>>
>>>>>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>>>>>> <martin at martin.st>:
>>>>>>>>>>
>>>>>>>>>> When testing on a memory limited system, these tests consume a
>>>>>>>>>> significant amount of memory and can often fail if testing by
>>>>>>>>>> running
>>>>>>>>>> multiple processes in parallel.
>>>>>>>>>> ---
>>>>>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>>>>>>
>>>>>>>>>> Also moved the 2k tests to the same option.
>>>>>>>>>> ---
>>>>>>>>>>  configure             | 3 +++
>>>>>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/configure b/configure
>>>>>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>>>>>> --- a/configure
>>>>>>>>>> +++ b/configure
>>>>>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on
>>>>>>>>>> FFmpeg 
>>>>>>>> itself):
>>>>>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>>>>>> prefix
>>>>>>>>>>                             in the name) of tests whose result is
>>>>>>>>>> ignored
>>>>>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>>>>>> +  --disable-large-tests    disable tests that use a large
>>>>>>>>>> amount of 
>>>>>>>> memory
>>>>>>>>>
>>>>>>>>> I would have suggested to control this when running the tests, if
>>>>>>>>> the 
>>>>>>>> configure
>>>>>>>>> setting makes sense, it should at least be possible to change the
>>>>>>>>> setting 
>>>>>>>> when
>>>>>>>>> calling make.
>>>>>>>>> Or is that possible anyway?
>>>>>>>>
>>>>>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>>>>>> var=value assignment on the make command line overrides any
>>>>>>>> var=othervalue assignment within the makefiles themselves, but that
>>>>>>>> doesn't seem very convenient.
>>>>>>>>
>>>>>>>> But I'd like to have it as a configure option, to easily be able to
>>>>>>>> set it e.g. in a fate setup.
>>>>>>> Any further opinions on this one - is it ok to go ahead with it in
>>>>>>> this
>>>>>>> form, or are changes requested?
>>>>>>
>>>>>> Configure option is fine if it can also be overridden from the
>>>>>> command
>>>>>> line at runtime (like --samples and SAMPLES).
>>>>>
>>>>> Well, it can be overridden at runtime, but it's not with a very
>>>>> convenient name (the CONFIG_* variable). Is that ok?
>>>>
>>>> Ideally, this would be possible with:
>>>> make BIG=no fate
>>>
>>> That requires a bit more extra intermediate variables.
>>>
>>> One way of doing it would be this:
>>>
>>> # Default, overriden by any "make BIG=no fate"
>>> BIG=$(CONFIG_LARGE_TESTS)
>>> ...
>>> TESTS-$(ENCDEC components)-$(BIG) += some-tests
>>> TESTS += TESTS-yes-yes
>>>
>>> While it earlier was requested to use $(ALLYES ...) instead of the
>>> -yes-yes construct.
>>>
>>> Or to keep using ALLYES, we'd need yet another intermediate variable:
>>>
>>> # Default, overriden by any "make BIG=no fate"
>>> BIG=$(CONFIG_LARGE_TESTS)
>>> # The same as BIG above, but with a CONFIG_ prefix
>>> CONFIG_BIG=$(BIG)
>>> ...
>>> TESTS-$(ALLYES components BIG) += some-tests
>>> TESTS += TESTS-yes
>>>
>>>
>>>
>>> James, what's your opinion on these two alternatives, if it should be
>>> configurable with a different variable name?
>>
>> BIG is too generic and could be used for anything. LARGE_TESTS would be
>> better, and would get rid of the need to add a new custom CONFIG_
>> variable for the second example using ALLYES.
> 
> I intentionally meant to use a different variable for that, to
> differentiate between the configure-generated CONFIG_LARGE_TESTS from
> config.mak and the one that is set to pick up a potential user-supplied
> value from e.g. LARGE_TESTS, otherwise falling back on the configure
> generated value - I'm not sure if that really works if the first and
> last variable are the same, or if that ends up as an infinite recursion
> otherwise (CONFIG_LARGE_TESTS expands to LARGE_TESTS which expands to
> CONFIG_LARGE_TESTS).

I still think just overriding using CONFIG_LARGE_TESTS from the command
line is more than enough for a developer debug option, but if others
want something shorter then your suggestion above for ALLYES is fine.