Skip to content
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.

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

Fix use of Visual Studio compiler values "-w..." or "-FS" #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlwinEsch
Copy link

This was needed by a Kodi addon project and ffmpeg Windows ARM compile,
where the flag "-FS" was needed, to become AppVeyor and Jenkins
more happy and the "-w..." to prevent one warning, which was given by
config.h on every compile file.

Without comes on configure:

BEGIN ./ffconf.KEHnLLrW/test.S
    1
    2   .macro m n, y:vararg=0
    3   \n: .int \y
    4   .endm
    5   m x
END ./ffconf.KEHnLLrW/test.S
gas-preprocessor.pl -arch aarch64 -as-type armasm -- armasm64.exe -I/d/Dev/Kodi64/NEW/inputstream.ffmpegdirect/build2/build/depends/include -D_ISOC99_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_USE_MATH_DEFINES -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -nologo -ignore 4509 -DWIN32 -D_WINDOWS -W3 -DUNICODE -D_UNICODE -DTARGET_WINDOWS -DTARGET_WINDOWS_STORE -DNOMINMAX -D_CRT_SECURE_NO_WARNINGS -D_WINSOCKAPI_ -ID:/Dev/Kodi64/NEW/inputstream.ffmpegdirect/build2/build/depends/include -ID:/Dev/Kodi64/NEW/inputstream.ffmpegdirect/build2/build/depends/include/libxml2 -FS -DWINAPI_FAMILY=WINAPI_FAMILY_APP -D_WIN32_WINNT=0x0A00 -wd4828 -mcpu=aarch64 -c -o ./ffconf.KEHnLLrW/test.o ./ffconf.KEHnLLrW/test.S
cpp.exe: error: unrecognized command line option '-wd4828'
GNU assembler not found, install/update gas-preprocessor

I'm not sure if OK, just thought that if I bring a patch into the project (https://github.com/AlwinEsch/inputstream.ffmpegdirect/tree/build-fixes/depends/common/gas-preprocessor), I could also ask and try to bring it in directly.

This was needed by a Kodi addon project and ffmpeg Windows ARM compile,
where the flag "-FS" was needed, to become AppVeyor and Jenkins
more happy and the "-w..." to prevent one warning, which was given by
config.h on every compile file.

Without comes on configure:
```bash
BEGIN ./ffconf.KEHnLLrW/test.S
    1
    2   .macro m n, y:vararg=0
    3   \n: .int \y
    4   .endm
    5   m x
END ./ffconf.KEHnLLrW/test.S
gas-preprocessor.pl -arch aarch64 -as-type armasm -- armasm64.exe -I/d/Dev/Kodi64/NEW/inputstream.ffmpegdirect/build2/build/depends/include -D_ISOC99_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_USE_MATH_DEFINES -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -nologo -ignore 4509 -DWIN32 -D_WINDOWS -W3 -DUNICODE -D_UNICODE -DTARGET_WINDOWS -DTARGET_WINDOWS_STORE -DNOMINMAX -D_CRT_SECURE_NO_WARNINGS -D_WINSOCKAPI_ -ID:/Dev/Kodi64/NEW/inputstream.ffmpegdirect/build2/build/depends/include -ID:/Dev/Kodi64/NEW/inputstream.ffmpegdirect/build2/build/depends/include/libxml2 -FS -DWINAPI_FAMILY=WINAPI_FAMILY_APP -D_WIN32_WINNT=0x0A00 -wd4828 -mcpu=aarch64 -c -o ./ffconf.KEHnLLrW/test.o ./ffconf.KEHnLLrW/test.S
cpp.exe: error: unrecognized command line option '-wd4828'
GNU assembler not found, install/update gas-preprocessor
```
@phunkyfish
Copy link

@mstorsjo what do you think?

@mstorsjo
Copy link

In general, the change probably looks good - but I wonder why this is needed - the normal build also uses a number of -wd parameters, and they are only passed when building C sources, not when building assembly files.

What does the configure line look like, how do you pass the -wd4828 and -FS options to the build? If they would be passed via --extra-cflags, they wouldn't end up passed to the assembler at all.

@AlwinEsch
Copy link
Author

AlwinEsch commented Apr 19, 2020

The related flags are set for here https://github.com/xbmc/inputstream.ffmpegdirect/blob/Matrix/depends/common/ffmpeg/CMakeLists.txt#L215-L235, what we use to create the ffmpeg for our Kodi addon


About the -FS was needed for here to have the multiprocess build fixed.

Here his build without value: https://jenkins.kodi.tv/blue/organizations/jenkins/xbmc%2Finputstream.ffmpegdirect/detail/PR-36/5/pipeline/307

libavfilter/af_acontrast.c: fatal error C1041: cannot open program database 'F:\builds\workspace\binary-addons\kodi-windows-x86_64-Matrix\cmake\addons\build\ffmpeg\src\ffmpeg\vc140.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FS
libavfilter/aeval.c: fatal error C1041: cannot open program database 'F:\builds\workspace\binary-addons\kodi-windows-x86_64-Matrix\cmake\addons\build\ffmpeg\src\ffmpeg\vc140.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FS

This with the gas-preprocessor comes then by the Windows UWP ARM build, where it bring, without patch:
https://dev.azure.com/teamkodi/binary-addons/_build/results?buildId=936&view=logs&j=f1a74db1-652b-553f-2cc0-a946453fa7a5&t=07a7f212-cab5-52f8-bce6-fbf845a5d175&l=1868

   GNU assembler not found, install/update gas-preprocessor

Seems by a test build inside configure.


The other for -w without the previous needed -wd4828, was currently not reproducable. But was coming by VS where it has a problem to handle a string inside config.h (if I'm not wrong now by this line):

#define CC_IDENT "Microsoft (R) C/C++-Optimierungscompiler Version 19.16.27034 f.r x64"

The german für was there on ü wrong and has generated on every compiled file a 4828 warning, which was by ffmpeg amount of source files, much, much warning messages.
On begin I seen this warning also on a test build, like Azure, where the amount of messages is limited to 5 MByte, where then goes over it.

Also can there other flags on CMAKE_C_FLAGS given from parent.

There was then on configure this, too:

   GNU assembler not found, install/update gas-preprocessor

@mstorsjo
Copy link

Right, thanks for the links.

After rechecking - options added via --extra-cflags do end up passed to armasm - the reason why I didn't think that happened was because the one flag I was passing to --extra-cflags, -MD, is being filtered out by configure here: https://github.com/FFmpeg/FFmpeg/blob/master/configure#L4397-L4406

One could in theory add these flags there as well, but it's probably better and more robust (and useful to more projects) to add the flags in gas-preprocessor instead.

So the patch looks fine, but development of gas-preprocessor happens over at https://github.com/ffmpeg/gas-preprocessor nowadays, and contributions should be sent to the ffmpeg-devel mailing list - once sent there I'll acknowledge and merge it.

@AlwinEsch
Copy link
Author

Thank you very much, had overlooked the fact that it is there now.

Will then bring the request there.

@phunkyfish
Copy link

@mstorsjo I submitted the patch on Alwin's behalf here: https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/

But appears to fail on applying it in patchwork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants