Skip to content

tests/fate: Improvements for running FATE on Windows/MSYS2 #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
softworkz wants to merge 3 commits into
base: master
Choose a base branch
from
Open

tests/fate: Improvements for running FATE on Windows/MSYS2 #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
facfadf
tests/fate: Fix subtitle fate tests on Windows
softworkz May 12, 2025
73ab2c2
tests/source-check: Fix make inclusion-guard check EOL-agnostic
softworkz May 12, 2025
c9e2157
tests/hevc: Fix concat input when running in MSYS2 shell
softworkz May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,38 @@
*.pnm -diff -text
Changelog merge=union
Changelog merge=union
tests/ref/fate/sub-aqtitle eol=lf
tests/ref/fate/sub-ass-to-ass-transcode eol=lf
tests/ref/fate/sub-cc eol=lf
tests/ref/fate/sub-cc-realtime
tests/ref/fate/sub-cc-realtime eol=lf
tests/ref/fate/sub-cc-scte20 eol=lf
tests/ref/fate/sub-charenc eol=lf
tests/ref/fate/sub-dvb eol=lf
tests/ref/fate/sub-jacosub eol=lf
tests/ref/fate/sub-lrc-remux eol=lf
tests/ref/fate/sub-microdvd eol=lf
tests/ref/fate/sub-microdvd-remux eol=lf
tests/ref/fate/sub-movtext eol=lf
tests/ref/fate/sub-mpl2 eol=lf
tests/ref/fate/sub-mpsub eol=lf
tests/ref/fate/sub-mpsub-frames eol=lf
tests/ref/fate/sub-pgs-remux eol=lf
tests/ref/fate/sub-pjs eol=lf
tests/ref/fate/sub-realtext eol=lf
tests/ref/fate/sub-sami eol=lf
tests/ref/fate/sub-sami2 eol=lf
tests/ref/fate/sub-scc-remux eol=lf
tests/ref/fate/sub-srt eol=lf
tests/ref/fate/sub-srt-badsyntax eol=lf
tests/ref/fate/sub-srt-empty-events eol=lf
tests/ref/fate/sub-srt-madness-timeshift eol=lf
tests/ref/fate/sub-srt-rrn-remux eol=lf
tests/ref/fate/sub-ssa-to-ass-remux eol=lf
tests/ref/fate/sub-stl eol=lf
tests/ref/fate/sub-subviewer eol=lf
tests/ref/fate/sub-subviewer1 eol=lf
tests/ref/fate/sub-ttmlenc eol=lf
tests/ref/fate/sub-vplayer eol=lf
tests/ref/fate/sub-webvtt eol=lf
tests/ref/fate/sub-webvtt2 eol=lf
tests/ref/fate/sub-webvttenc eol=lf
2 changes: 1 addition & 1 deletion tests/fate/hevc.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ $(TARGET_SAMPLES)/hevc-conformance/LS_A_Orange_2.bit|$\
$(TARGET_SAMPLES)/hevc/mv_nuh_layer_id.bit|$\
$(TARGET_SAMPLES)/hevc-conformance/NoOutPrior_B_Qualcomm_1.bit|$\
$(TARGET_SAMPLES)/hevc-conformance/MVHEVCS_A.bit
fate-hevc-mv-switch: CMD = framecrc -i "concat:$(INPUT)" -fps_mode passthrough -map 0:vidx:0 -map 0:vidx:1 -sws_flags +accurate_rnd+bitexact
fate-hevc-mv-switch: CMD = MSYS2_ARG_CONV_EXCL=concat: framecrc -i "concat:$(INPUT)" -fps_mode passthrough -map 0:vidx:0 -map 0:vidx:1 -sws_flags +accurate_rnd+bitexact
FATE_HEVC-$(call FRAMECRC, HEVC, HEVC, SCALE_FILTER CONCAT_PROTOCOL) += fate-hevc-mv-switch

# multiview stream, select view by position
Expand Down
2 changes: 1 addition & 1 deletion tests/fate/source-check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ for f in `git ls-files | grep '\.h$'` ; do
-e 's/_vaf_/_/' \
Copy link

@ffmpeg-codebot ffmpeg-codebot bot May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the FFmpeg mailing list, Andreas Rheinhardt wrote (reply to this):

softworkz:
> From: softworkz <[email protected]>
> 
> ..to make it work when checked out with autocrlf=on,
> which is Git default on Windows.
> 
> Signed-off-by: softworkz <[email protected]>
> ---
>  tests/fate/source-check.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/fate/source-check.sh b/tests/fate/source-check.sh
> index 4d7e175784..99e869e869 100755
> --- a/tests/fate/source-check.sh
> +++ b/tests/fate/source-check.sh
> @@ -28,7 +28,7 @@ for f in `git ls-files | grep '\.h$'` ; do
>          -e 's/_vaf_/_/' \
>      | tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ`"
>  
> -    git grep -L "^#define $macro$" $f
> +    git grep -L "^#define $macro\>" $f

This makes the test less strict. Why don't we instead just specify that
the repo should be checked out with lf only? Would this break something?

>  done
>  
>  echo "Use of av_clip() where av_clip_uintp2() could be used:"

_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".

Copy link

@ffmpeg-codebot ffmpeg-codebot bot May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the FFmpeg mailing list, "softworkz ." wrote (reply to this):



> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of Andreas
> Rheinhardt
> Sent: Donnerstag, 22. Mai 2025 12:42
> To: [email protected]
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/3] tests/source-check: Fix make
> inclusion-guard check EOL-agnostic
> 
> softworkz:
> > From: softworkz <[email protected]>
> >
> > ..to make it work when checked out with autocrlf=on,
> > which is Git default on Windows.
> >
> > Signed-off-by: softworkz <[email protected]>
> > ---
> >  tests/fate/source-check.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/fate/source-check.sh b/tests/fate/source-check.sh
> > index 4d7e175784..99e869e869 100755
> > --- a/tests/fate/source-check.sh
> > +++ b/tests/fate/source-check.sh
> > @@ -28,7 +28,7 @@ for f in `git ls-files | grep '\.h$'` ; do
> >          -e 's/_vaf_/_/' \
> >      | tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ`"
> >
> > -    git grep -L "^#define $macro$" $f
> > +    git grep -L "^#define $macro\>" $f
> 
> This makes the test less strict. 

Yea, that's correct, but does that defeat the intention of the test?
It might allow whitespace at the end of the but this is something
that can happen for any line in any file, not just the guard definitions
in header files. Eventually this is guarded against by the hooks of
the Git repo when pushing. 
It might also allow more text after some whitespace, but that would 
file compilation, I think.

Do you know some regex Kung-Fu to ignore EOL and still use an end 
marker? I had found a way but that requires switching to Perl matching
(-E), but from what I've read, we cannot assume this to be available
on all platforms.


> Why don't we instead just specify that
> the repo should be checked out with lf only? Would this break something?

From my experience it can cause a lot of trouble. The following 
discussions from last year may give you an idea of these pitfalls,
even though not everything might apply to FFmpeg:

https://github.com/ffmpeginteropx/FFmpegInteropX/pull/433
https://github.com/ffmpeginteropx/FFmpegInteropX/pull/431
https://github.com/ffmpeginteropx/FFmpegInteropX/pull/430

The risk is that it causes more trouble than the problems it might
solve.
What stands on the other side is that these two patches is all that
is needed to successfully run FATE tests on Windows.

When a new subtitle test is added, the entry in .gitattributes may
be forgotten, but with the new CI builds on Windows it would also
be discovered immediately.


Thanks a lot for looking at this,
sw





_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".

Copy link

@ffmpeg-codebot ffmpeg-codebot bot May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the FFmpeg mailing list, "softworkz ." wrote (reply to this):



> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of softworkz .
> Sent: Donnerstag, 22. Mai 2025 13:12
> To: FFmpeg development discussions and patches <[email protected]>
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/3] tests/source-check: Fix make
> inclusion-guard check EOL-agnostic
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <[email protected]> On Behalf Of Andreas
> > Rheinhardt
> > Sent: Donnerstag, 22. Mai 2025 12:42
> > To: [email protected]
> > Subject: Re: [FFmpeg-devel] [PATCH v2 2/3] tests/source-check: Fix make
> > inclusion-guard check EOL-agnostic
> >
> > softworkz:
> > > From: softworkz <[email protected]>
> > >
> > > ..to make it work when checked out with autocrlf=on,
> > > which is Git default on Windows.
> > >
> > > Signed-off-by: softworkz <[email protected]>
> > > ---
> > >  tests/fate/source-check.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/fate/source-check.sh b/tests/fate/source-check.sh
> > > index 4d7e175784..99e869e869 100755
> > > --- a/tests/fate/source-check.sh
> > > +++ b/tests/fate/source-check.sh
> > > @@ -28,7 +28,7 @@ for f in `git ls-files | grep '\.h$'` ; do
> > >          -e 's/_vaf_/_/' \
> > >      | tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ`"
> > >
> > > -    git grep -L "^#define $macro$" $f
> > > +    git grep -L "^#define $macro\>" $f
> >
> > This makes the test less strict.
> 
> Yea, that's correct, but does that defeat the intention of the test?
> It might allow whitespace at the end of the but this is something
> that can happen for any line in any file, not just the guard definitions
> in header files. Eventually this is guarded against by the hooks of
> the Git repo when pushing.
> It might also allow more text after some whitespace, but that would
> file compilation, I think.
> 
> Do you know some regex Kung-Fu to ignore EOL and still use an end
> marker? I had found a way but that requires switching to Perl matching
> (-E), but from what I've read, we cannot assume this to be available
> on all platforms.
> 
> 
> > Why don't we instead just specify that
> > the repo should be checked out with lf only? Would this break something?
> 
> From my experience it can cause a lot of trouble. The following
> discussions from last year may give you an idea of these pitfalls,
> even though not everything might apply to FFmpeg:
> 
> https://github.com/ffmpeginteropx/FFmpegInteropX/pull/433
> https://github.com/ffmpeginteropx/FFmpegInteropX/pull/431
> https://github.com/ffmpeginteropx/FFmpegInteropX/pull/430
> 
> The risk is that it causes more trouble than the problems it might
> solve.
> What stands on the other side is that these two patches is all that
> is needed to successfully run FATE tests on Windows.
> 
> When a new subtitle test is added, the entry in .gitattributes may
> be forgotten, but with the new CI builds on Windows it would also
> be discovered immediately.
> _______________________________________________

Oh, and I totally forgot this: There are other tests which are 
failing when declaring EOL=LF in .gitattributes - even a few
subtitle tests, that's why I haven't added all of them.

sw


_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".

| tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ`"

git grep -L "^#define $macro$" $f
git grep -L "^#define $macro\>" $f
done

echo "Use of av_clip() where av_clip_uintp2() could be used:"
Expand Down