[FFmpeg-devel] Added additional note to fate.texi to describe the importance of not checking out files from git with core.autocrlf set to true

Submitted by Aaron Levinson on April 21, 2017, 11:48 p.m.

Details

Message ID 06919b3e-86f4-8821-e3cc-e800c2c521d9@aracnet.com
State New
Headers show

Commit Message

Aaron Levinson April 21, 2017, 11:48 p.m.
On 4/21/2017 4:17 PM, Lou Logan wrote:
> On Fri, 21 Apr 2017 16:05:16 -0700
> Aaron Levinson <alevinsn@aracnet.com> wrote:
> 
>> From 7ea6455cd52ffe561f94bf03f48c4c2cf61b33c5 Mon Sep 17 00:00:00 2001
>> From: Aaron Levinson <alevinsn@aracnet.com>
>> Date: Fri, 21 Apr 2017 15:55:11 -0700
>> Subject: [PATCH] Added additional note to fate.texi to describe the importance
>>  of not checking out files from git with core.autocrlf set to true
>>
>> Purpose: Added additional note to fate.texi to describe the importance
>> of not checking out files from git with core.autocrlf set to true.
>> When CRLF is used for line endings, this causes the source FATE test,
>> which uses check-source.sh, to fail.  The issue has to do with the use
>> of git grep, which doesn't appear to be able to handle CRLF line
>> endings.  Older versions of check-source.sh used grep directly, which
>> seems to do a better job of handling CRLF line endings.
>>
>> Note: thanks to Hendrik Leppkes for suggestion that the issue with
>> FATE failing was caused by CRLF line endings.
>> ---
>>  doc/fate.texi | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/doc/fate.texi b/doc/fate.texi
>> index 7a96c25..80fac0a 100644
>> --- a/doc/fate.texi
>> +++ b/doc/fate.texi
>> @@ -77,6 +77,15 @@ FATE_SAMPLES=fate-suite/ make fate
>>  @float NOTE
>>  Do not put a '~' character in the samples path to indicate a home
>>  directory. Because of shell nuances, this will cause FATE to fail.
>> +
>> +In addition, FATE will fail if files are checked out from git such
>> +that CRLF is used for line endings.  This will occur on Windows when
>> +git is installed using the default options, because that causes git's
>> +core.autocrlf setting to be set to 'true'.  Make sure to set
>> +core.autocrlf to 'input' or 'false', or, in the case that the
>> +repository has already been cloned, it is possible to get past this by
>> +executing the following command in the top-level ffmpeg directory:
>> +@command{find -name '*.h' -type f | xargs dos2unix}.
>>  @end float
> 
> This is a non-blocking minor comment, but it would be nice to add some
> additional markup to core.autocrlf and true/false. For a list see:
> 
> <https://www.gnu.org/software/texinfo/manual/texinfo/html_node/Indicating.html#Indicating>

How about the following?

Thanks,
Aaron Levinson

------------------------------------------------------------------------------------

From 707b61cfcc648eb228a157ce2c9ed0f61a2ba5ca Mon Sep 17 00:00:00 2001
From: Aaron Levinson <alevinsn@aracnet.com>
Date: Fri, 21 Apr 2017 16:45:28 -0700
Subject: [PATCH] Added additional note to fate.texi to describe the importance
 of not checking out files from git with core.autocrlf set to true

Purpose: Added additional note to fate.texi to describe the importance
of not checking out files from git with core.autocrlf set to true.
When CRLF is used for line endings, this causes the source FATE test,
which uses check-source.sh, to fail.  The issue has to do with the use
of git grep, which doesn't appear to be able to handle CRLF line
endings.  Older versions of check-source.sh used grep directly, which
seems to do a better job of handling CRLF line endings.

Note: thanks to Hendrik Leppkes for suggestion that the issue with
FATE failing was caused by CRLF line endings.
---
 doc/fate.texi | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Aaron Levinson April 22, 2017, 2:20 a.m.
Note that in case it wasn't clear, I confirmed that this patch works by 
running make on Linux and examining both the generated text file and the 
generated HTML file in a Web browser.

Aaron

On 4/21/2017 4:48 PM, Aaron Levinson wrote:
> On 4/21/2017 4:17 PM, Lou Logan wrote:
>> On Fri, 21 Apr 2017 16:05:16 -0700
>> Aaron Levinson <alevinsn@aracnet.com> wrote:
>>
>>> From 7ea6455cd52ffe561f94bf03f48c4c2cf61b33c5 Mon Sep 17 00:00:00 2001
>>> From: Aaron Levinson <alevinsn@aracnet.com>
>>> Date: Fri, 21 Apr 2017 15:55:11 -0700
>>> Subject: [PATCH] Added additional note to fate.texi to describe the importance
>>>  of not checking out files from git with core.autocrlf set to true
>>>
>>> Purpose: Added additional note to fate.texi to describe the importance
>>> of not checking out files from git with core.autocrlf set to true.
>>> When CRLF is used for line endings, this causes the source FATE test,
>>> which uses check-source.sh, to fail.  The issue has to do with the use
>>> of git grep, which doesn't appear to be able to handle CRLF line
>>> endings.  Older versions of check-source.sh used grep directly, which
>>> seems to do a better job of handling CRLF line endings.
>>>
>>> Note: thanks to Hendrik Leppkes for suggestion that the issue with
>>> FATE failing was caused by CRLF line endings.
>>> ---
>>>  doc/fate.texi | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/doc/fate.texi b/doc/fate.texi
>>> index 7a96c25..80fac0a 100644
>>> --- a/doc/fate.texi
>>> +++ b/doc/fate.texi
>>> @@ -77,6 +77,15 @@ FATE_SAMPLES=fate-suite/ make fate
>>>  @float NOTE
>>>  Do not put a '~' character in the samples path to indicate a home
>>>  directory. Because of shell nuances, this will cause FATE to fail.
>>> +
>>> +In addition, FATE will fail if files are checked out from git such
>>> +that CRLF is used for line endings.  This will occur on Windows when
>>> +git is installed using the default options, because that causes git's
>>> +core.autocrlf setting to be set to 'true'.  Make sure to set
>>> +core.autocrlf to 'input' or 'false', or, in the case that the
>>> +repository has already been cloned, it is possible to get past this by
>>> +executing the following command in the top-level ffmpeg directory:
>>> +@command{find -name '*.h' -type f | xargs dos2unix}.
>>>  @end float
>>
>> This is a non-blocking minor comment, but it would be nice to add some
>> additional markup to core.autocrlf and true/false. For a list see:
>>
>> <https://www.gnu.org/software/texinfo/manual/texinfo/html_node/Indicating.html#Indicating>
>
> How about the following?
>
> Thanks,
> Aaron Levinson
>
> ------------------------------------------------------------------------------------
>
> From 707b61cfcc648eb228a157ce2c9ed0f61a2ba5ca Mon Sep 17 00:00:00 2001
> From: Aaron Levinson <alevinsn@aracnet.com>
> Date: Fri, 21 Apr 2017 16:45:28 -0700
> Subject: [PATCH] Added additional note to fate.texi to describe the importance
>  of not checking out files from git with core.autocrlf set to true
>
> Purpose: Added additional note to fate.texi to describe the importance
> of not checking out files from git with core.autocrlf set to true.
> When CRLF is used for line endings, this causes the source FATE test,
> which uses check-source.sh, to fail.  The issue has to do with the use
> of git grep, which doesn't appear to be able to handle CRLF line
> endings.  Older versions of check-source.sh used grep directly, which
> seems to do a better job of handling CRLF line endings.
>
> Note: thanks to Hendrik Leppkes for suggestion that the issue with
> FATE failing was caused by CRLF line endings.
> ---
>  doc/fate.texi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/doc/fate.texi b/doc/fate.texi
> index 7a96c25..f3b8c0c8 100644
> --- a/doc/fate.texi
> +++ b/doc/fate.texi
> @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate
>  @float NOTE
>  Do not put a '~' character in the samples path to indicate a home
>  directory. Because of shell nuances, this will cause FATE to fail.
> +
> +In addition, FATE will fail if files are checked out from git such
> +that @kbd{@key{CR}@key{LF}} is used for line endings.  This will occur
> +on Windows when git is installed using the default options, because
> +that causes git's @var{core.autocrlf} setting to be set to
> +@option{true}.  Make sure to set @var{core.autocrlf} to @option{input}
> +or @option{false}, or, in the case that the repository has already
> +been cloned, it is possible to get past this by executing the
> +following command in the top-level ffmpeg directory: @command{find
> +-name '*.h' -type f | xargs dos2unix}.
>  @end float
>
>  To use a custom wrapper to run the test, pass @option{--target-exec} to
>
Clément Bœsch April 22, 2017, 9:16 a.m.
On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote:
[...]
> diff --git a/doc/fate.texi b/doc/fate.texi
> index 7a96c25..f3b8c0c8 100644
> --- a/doc/fate.texi
> +++ b/doc/fate.texi
> @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate
>  @float NOTE
>  Do not put a '~' character in the samples path to indicate a home
>  directory. Because of shell nuances, this will cause FATE to fail.
> +
> +In addition, FATE will fail if files are checked out from git such
> +that @kbd{@key{CR}@key{LF}} is used for line endings.  This will occur
> +on Windows when git is installed using the default options, because
> +that causes git's @var{core.autocrlf} setting to be set to
> +@option{true}.  Make sure to set @var{core.autocrlf} to @option{input}
> +or @option{false}, or, in the case that the repository has already
> +been cloned, it is possible to get past this by executing the
> +following command in the top-level ffmpeg directory: @command{find
> +-name '*.h' -type f | xargs dos2unix}.
>  @end float
>  

This is documented in doc/git-howto.texi, isn't it?
Aaron Levinson April 22, 2017, 7:07 p.m.
On 4/22/2017 2:16 AM, Clément Bœsch wrote:
> On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote:
> [...]
>> diff --git a/doc/fate.texi b/doc/fate.texi
>> index 7a96c25..f3b8c0c8 100644
>> --- a/doc/fate.texi
>> +++ b/doc/fate.texi
>> @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate
>>  @float NOTE
>>  Do not put a '~' character in the samples path to indicate a home
>>  directory. Because of shell nuances, this will cause FATE to fail.
>> +
>> +In addition, FATE will fail if files are checked out from git such
>> +that @kbd{@key{CR}@key{LF}} is used for line endings.  This will occur
>> +on Windows when git is installed using the default options, because
>> +that causes git's @var{core.autocrlf} setting to be set to
>> +@option{true}.  Make sure to set @var{core.autocrlf} to @option{input}
>> +or @option{false}, or, in the case that the repository has already
>> +been cloned, it is possible to get past this by executing the
>> +following command in the top-level ffmpeg directory: @command{find
>> +-name '*.h' -type f | xargs dos2unix}.
>>  @end float
>>
>
> This is documented in doc/git-howto.texi, isn't it?

Yes it is, but just because it can be found elsewhere in the 
documentation doesn't mean there isn't benefit to having a fallback 
elsewhere in the documentation.

Aaron Levinson
Hendrik Leppkes April 22, 2017, 7:09 p.m.
On Sat, Apr 22, 2017 at 9:07 PM, Aaron Levinson <alevinsn@aracnet.com> wrote:
>
>
> On 4/22/2017 2:16 AM, Clément Bœsch wrote:
>>
>> On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote:
>> [...]
>>>
>>> diff --git a/doc/fate.texi b/doc/fate.texi
>>> index 7a96c25..f3b8c0c8 100644
>>> --- a/doc/fate.texi
>>> +++ b/doc/fate.texi
>>> @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate
>>>  @float NOTE
>>>  Do not put a '~' character in the samples path to indicate a home
>>>  directory. Because of shell nuances, this will cause FATE to fail.
>>> +
>>> +In addition, FATE will fail if files are checked out from git such
>>> +that @kbd{@key{CR}@key{LF}} is used for line endings.  This will occur
>>> +on Windows when git is installed using the default options, because
>>> +that causes git's @var{core.autocrlf} setting to be set to
>>> +@option{true}.  Make sure to set @var{core.autocrlf} to @option{input}
>>> +or @option{false}, or, in the case that the repository has already
>>> +been cloned, it is possible to get past this by executing the
>>> +following command in the top-level ffmpeg directory: @command{find
>>> +-name '*.h' -type f | xargs dos2unix}.
>>>  @end float
>>>
>>
>> This is documented in doc/git-howto.texi, isn't it?
>
>
> Yes it is, but just because it can be found elsewhere in the documentation
> doesn't mean there isn't benefit to having a fallback elsewhere in the
> documentation.

Can't we just make it point there, instead of explaining it again?

- Hendrik
Aaron Levinson April 22, 2017, 7:22 p.m.
On 4/22/2017 12:09 PM, Hendrik Leppkes wrote:
> On Sat, Apr 22, 2017 at 9:07 PM, Aaron Levinson <alevinsn@aracnet.com> wrote:
>>
>>
>> On 4/22/2017 2:16 AM, Clément Bœsch wrote:
>>>
>>> On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote:
>>> [...]
>>>>
>>>> diff --git a/doc/fate.texi b/doc/fate.texi
>>>> index 7a96c25..f3b8c0c8 100644
>>>> --- a/doc/fate.texi
>>>> +++ b/doc/fate.texi
>>>> @@ -77,6 +77,16 @@ FATE_SAMPLES=fate-suite/ make fate
>>>>  @float NOTE
>>>>  Do not put a '~' character in the samples path to indicate a home
>>>>  directory. Because of shell nuances, this will cause FATE to fail.
>>>> +
>>>> +In addition, FATE will fail if files are checked out from git such
>>>> +that @kbd{@key{CR}@key{LF}} is used for line endings.  This will occur
>>>> +on Windows when git is installed using the default options, because
>>>> +that causes git's @var{core.autocrlf} setting to be set to
>>>> +@option{true}.  Make sure to set @var{core.autocrlf} to @option{input}
>>>> +or @option{false}, or, in the case that the repository has already
>>>> +been cloned, it is possible to get past this by executing the
>>>> +following command in the top-level ffmpeg directory: @command{find
>>>> +-name '*.h' -type f | xargs dos2unix}.
>>>>  @end float
>>>>
>>>
>>> This is documented in doc/git-howto.texi, isn't it?
>>
>>
>> Yes it is, but just because it can be found elsewhere in the documentation
>> doesn't mean there isn't benefit to having a fallback elsewhere in the
>> documentation.
>
> Can't we just make it point there, instead of explaining it again?

OK, I'll do that.
Aaron Levinson May 3, 2017, 7:45 p.m.
On 4/22/2017 12:22 PM, Aaron Levinson wrote:
> On 4/22/2017 12:09 PM, Hendrik Leppkes wrote:
>> On Sat, Apr 22, 2017 at 9:07 PM, Aaron Levinson <alevinsn@aracnet.com>
>> wrote:
>>>
>>>
>>> On 4/22/2017 2:16 AM, Clément Bœsch wrote:
>>>>
>>>> On Fri, Apr 21, 2017 at 04:48:27PM -0700, Aaron Levinson wrote:
>>>>> [...]
>>>>
>>>> This is documented in doc/git-howto.texi, isn't it?
>>>
>>>
>>> Yes it is, but just because it can be found elsewhere in the
>>> documentation
>>> doesn't mean there isn't benefit to having a fallback elsewhere in the
>>> documentation.
>>
>> Can't we just make it point there, instead of explaining it again?
>
> OK, I'll do that.

I'm abandoning this patch and will submit a new one shortly with a new 
title.

Aaron Levinson

Patch hide | download patch | download mbox

diff --git a/doc/fate.texi b/doc/fate.texi
index 7a96c25..f3b8c0c8 100644
--- a/doc/fate.texi
+++ b/doc/fate.texi
@@ -77,6 +77,16 @@  FATE_SAMPLES=fate-suite/ make fate
 @float NOTE
 Do not put a '~' character in the samples path to indicate a home
 directory. Because of shell nuances, this will cause FATE to fail.
+
+In addition, FATE will fail if files are checked out from git such
+that @kbd{@key{CR}@key{LF}} is used for line endings.  This will occur
+on Windows when git is installed using the default options, because
+that causes git's @var{core.autocrlf} setting to be set to
+@option{true}.  Make sure to set @var{core.autocrlf} to @option{input}
+or @option{false}, or, in the case that the repository has already
+been cloned, it is possible to get past this by executing the
+following command in the top-level ffmpeg directory: @command{find
+-name '*.h' -type f | xargs dos2unix}.
 @end float
 
 To use a custom wrapper to run the test, pass @option{--target-exec} to