Re: [FFmpeg-devel] [PATCH 0/3] tests/fate: Improvements for running FATE on Windows/MSYS2

[Kacper Michajlow <kasper93-at-gmail.com@ffmpeg.org>]

On Tue, 17 Jun 2025 at 03:46, softworkz . <softworkz@hotmail.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Kacper Michajlow <kasper93@gmail.com>
> > Sent: Tuesday, June 17, 2025 3:00 AM
> > To: softworkz . <softworkz@hotmail.com>
> > Cc: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 0/3] tests/fate: Improvements for running
> > FATE on Windows/MSYS2
> >
> > On Tue, 17 Jun 2025 at 01:05, softworkz . <softworkz@hotmail.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kacper Michajlow <kasper93@gmail.com>
> > > > Sent: Tuesday, June 17, 2025 12:44 AM
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@ffmpeg.org>
> > > > Cc: softworkz <softworkz@hotmail.com>
> > > > Subject: Re: [FFmpeg-devel] [PATCH 0/3] tests/fate: Improvements for
> > > > running FATE on Windows/MSYS2
> > > >
> > > > On Mon, 12 May 2025 at 12:00, ffmpegagent <ffmpegagent@gmail.com>
> > > > wrote:
> > > > >
> > > > > When setting up the new Patchword builders I noticed some issues
> > > > > when running FATE tests on Windows. Initially I had them
> > > > > suppressed on the builders, but this patchset should finally fix it.
> > > > >
> > > > > softworkz (3):
> > > > >   tests/fate: Fix subtitle fate tests on Windows
> > > > >   tests/source-check: Fix make inclusion-guard check EOL-agnostic
> > > >
> > > > I think ffmpeg repositories should always be checked out with LF
> > > > line endings, there is nothing that expects those sources to have
> > > > CRLF. If you like you can set attributes to all files to LF (not
> > > > only subs)
> > >
> > > FATE already fails when setting LF for all subtitle ref files.
> > >
> >
> > What do you mean? Everything is LF based. I don't see any failures.
>
> Hi Kasper,
>
> A common misconception is that autocrlf=off would mean that you are
> dealing just with LF line endings in Git - but that's not the case, even
> the opposite is true: Disabling autocrlf is what actually enables check-in
> of files with CRLF - often accidentally. But it's not always accidental. Some
> of the FATE reference files for subtitle tests actually _do_ have CRLF line
> endings. By specifying LF in .gitattributes, CRLF would get replaced by LF
> and the tests will fail.
> Setting LF in .gitattributes is something totally different from autocrlf=off.
> The latter means "do not change line endings back-and-forth when checking
> In and out", but what you set in .gitattributes trumps autocrlf - i.e. autocrlf
> doesn't act on files with a .gitattributes entry about line endings.

I know how autocrlf works. I only said that imposing default logic
that "Windows always must use CRLF" and git implicitly will break your
committed line endings by default was a short sighted design decision.
It's good to have options to do the conversion IF the user wants that
or is configured as needed for a certain repository/platform.

> The patch may look like as if I had forgotten some of the subtitle ref files,
> but no: I had to carefully choose the ones who need it and which must not
> be changed.

Could you be more specific? Which ones? I think rcombs and astiob
changed all remaining CRLF sometime in last year or even earlier.

Either way, files are committed in the proper way in the repository,
so just make your git client not break that on checkout.

> > >
> > > >, but essentially this should already  be done by the user when
> > > >checking the repository.
> > > >
> > > > (autocrlf should be considered harmful, the was bad idea to make git
> > > > tooling smarter for its own good)
> > >
> > > While this might be true, autocrlf is on by default and it's harmful
> > > to switch It off globally as that would screw things in many other projects.
> > >
> > > Checking out only FFmpeg with autocrlf=off is non-trivial. Nobody
> > > knows how to do this properly and even less people will know how to
> > > properly change it after checking out, in a way that all files get changed as
> > well.
> > >
> > > I do know both, but I work with FFmpeg, having autocrlf=oni (the
> > > default on
> > > Windows) for more than 10 years, and I think it's more than valid to
> > > expect That FATE tests are running successfully also under these conditions.
> > >
> > > > >   tests/hevc: Fix concat input when running in MSYS2 shell
> > > >
> > > > This is more tricky, but frankly, I don't like injecting platform
> > > > specific workarounds into makefile files like that. Either maintain
> > > > it yourself or do it in a more generic way, not just in one hevc
> > > > test, because what if someone else adds a concat test? Do you expect
> > > > them to know that some MSYS2 specific handling is needed? It shouldn't
> > be.
> > > > Also if you like to fix "fate paths", it should be done fully.
> > > > Currently only relative paths are working, because some tests are
> > > > doing things like "$(input)[bla]" which also trips patch conversion,
> > > > so full unix path doesn't work, because it won't get converted to
> > > > Windows one, Windows path doesn't work, because it would be mangled
> > because of not escaped `\`.
> > >
> > > The fix I'm proposing is in fact working with relative paths only. If
> > > you know a Better way, that works in all cases, please feel free to tell.
> > >
> > > It's clearly just a "better-than-nothing" fix - but it's still better
> > > than nothing. 😊
> >
> > I'm confused though, because concat tests input is not converted. What
> > exactly are we fixing here?
>
> Please see here for background:
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/520f9af365f550aefc3e9abfacab83cfc8817b8e.1747043988.git.ffmpegagent@gmail.com/
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/c9e21574c0c8be252b3ff83133a004e3eef6803c.1747146207.git.ffmpegagent@gmail.com/
>
> Zhao had reported this issue before and while I tried to get FATE tests working on MSYS2 I found a solution. Unfortunately, it only works when the FATE samples path is specified relative to the FFmpeg dir, but at least it opens up one way where it is working, after there wasn't any way before. It's the best I could find, unfortunately.
>
>
>
> > 2025-06-16T23:27:19.8284085Z TEST    hevc-mv-switch
> > 2025-06-16T23:27:19.8286651Z /c/a/FFmpeg/FFmpeg/tests/fate-run.sh
> > fate-hevc-mv-switch "../samples" ""
> > "/c/a/FFmpeg/FFmpeg/.github/fate/build" 'framecrc -i
> > "concat:../samples/hevc-
> > conformance/LS_A_Orange_2.bit|../samples/hevc/mv_nuh_layer_id.bit|../sa
> > mples/hevc-conformance/NoOutPrior_B_Qualcomm_1.bit|../samples/hevc-
> > conformance/MVHEVCS_A.bit"
> > -fps_mode passthrough -map 0:vidx:0 -map 0:vidx:1 -sws_flags
> > +accurate_rnd+bitexact' '' '' '' '1' '' '' '' '' '' '' '' '' '' ''
> >
> > It works fine as is, MSYS2 won't convert this because it's separated with |.
>
> The fix is only needed when you are running make fate interactively from a
> command line in an MSYS2 shell.

Hmm, I see.

- Kacper
_______________________________________________
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".