tests/fate: Improvements for running FATE on Windows/MSYS2 #77
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
..when checked out with autocrlf=on, which is Git default on Windows. Signed-off-by: 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]>
softworkz
force-pushed
the
submit_fate_subs_eol
branch
from
1cabca4 to
520f9af
Compare
Collaborator
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![ffmpeg-codebot[bot]](https://avatars.githubusercontent.com/in/158557?s=60&v=4){kind=small}
MSYS2 considers the colon (:) as path separator (=separating multiple paths) and thinks it needs to convert it to a Windows path separator (;). Setting the MSYS2_ARG_CONV_EXCL environment variable keeps MSYS2 from doing this replacement. NOTE: This makes it possible to run the test from an MSYS2 shell, but it only works when using a relative path like --samples=../fate-suite (in configure) or SAMPLES=../fate-suite (in make fate) Reviewed-by: Zhao Zhili <[email protected]> Signed-off-by: softworkz <[email protected]>
softworkz
force-pushed
the
submit_fate_subs_eol
branch
from
520f9af to
c9e2157
Compare
Collaborator
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the FFmpeg mailing list, "softworkz ." wrote (reply to this): |
| -e 's/_af_/_/' \ | ||
| -e 's/_vf_/_/' \ | ||
| -e 's/_avf_/_/' \ | ||
| -e 's/_vaf_/_/' \ |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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".
|
On the FFmpeg mailing list, Kacper Michajlow wrote (reply to this): |
|
On the FFmpeg mailing list, "softworkz ." wrote (reply to this): |
|
On the FFmpeg mailing list, Kacper Michajlow wrote (reply to this): |
|
On the FFmpeg mailing list, "softworkz ." wrote (reply to this): |
|
On the FFmpeg mailing list, Kacper Michajlow wrote (reply to this): |
|
On the FFmpeg mailing list, "softworkz ." wrote (reply to this): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Version V2
.