Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#1783 closed defect (wontfix)

libavutil contains file with the same name as a system header

Reported by: obucinac Owned by:
Priority: normal Component: undetermined
Version: unspecified Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Since 1.0 release, there is a file named time.h in libavutil, which is the same name as a system header. This may break custom builds, because (depending on -I flags) #include <time.h> in libavutils/parseutils.c may include:

libavutil/time.h

instead of

/usr/include/time.h

I am working with Android NDK and Android build system and this is probably out of your fixing scope, since Android NDK build is not officially supported (and one can always set invalid compiler flags). However, I would appreciate if you consider renaming a header with the same name as a system header. The problem is that Android build system adds additional -I flags beside those defined in Android makefile, and this cant be prevented.

How to reproduce:

It is reproducible with any compiler

cd ffmpeg
./configure

This works
gcc -I . -c libavutil/parseutils.c

This fails
gcc -I . -I libavutil -c libavutil/parseutils.c

libavutil/parseutils.h:158:64: warning: ‘struct tm’ declared inside parameter list
libavutil/parseutils.h:171:8: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘attribute’ before ‘av_timegm’
libavutil/parseutils.c:441:64: warning: ‘struct tm’ declared inside parameter list
libavutil/parseutils.c:441:7: error: conflicting types for ‘av_small_strptime’
libavutil/parseutils.h:158:7: note: previous declaration of ‘av_small_strptime’ was here

Change History (18)

comment:1 Changed 4 years ago by cehoyos

Does it work if you remove "-I libavutil"?
I believe this should never be added to a compile command.

comment:2 Changed 4 years ago by burek

I believe he (obucinac) explained why he used "-I libavutil" as an example and why it creates problems, so it's pretty much the point of his message.

comment:3 follow-up: Changed 4 years ago by dlongest

I am also building FFmpeg for use on Android. When using Android's build system, it will automatically add the libavutil folder to the include directories as part of the build process.

Correct me if I am wrong, but giving a header the same file name as one in the ANSI C standards may not fall under best practices anyways. Possibly "avtime.h" would be a better fit in this case.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by cehoyos

Replying to dlongest:

Correct me if I am wrong, but giving a header the same file name as one in the ANSI C standards may not fall under best practices anyways. Possibly "avtime.h" would be a better fit in this case.

So if I understand you correctly you suggest that instead of you using the correct "#include <libavutil/avutil.h> (or whatever the use case might be), we should break API?

Or to try it differently:
Please provide the C program and (the Makefile or the gcc command line that fail). As you found out, neither the Makefile nor the gcc command line must contain "-I/usr/include/libavutil".

Or am I missing something?

Last edited 4 years ago by cehoyos (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 years ago by dlongest

Well this is partially an Android build system issue, however it is an issue that could easily pop up somewhere else.

The Android NDK allows you to set up Android specific Makefiles (usually titled Android.mk or Application.mk) in order to simplify development, their build tools automatically set up include directories, handle dependencies, handle proper linking, and more. Part of the process causes the GCC call to have an extra -I flag for the directory the c/cpp file is located. This causes a file like "parseutils.c" to be compiled with something like the following.

arm-linux-androideabi-gcc -MMD -MP -MF ./obj/local/armeabi-v7a/objs-debug/avutil/parseutils.o.d -Ijni/ffmpeg/libavutil -c  jni/ffmpeg/libavutil/parseutils.c -o ./obj/local/armeabi-v7a/objs-debug/avutil/parseutils.o

Since parseutils.c (as well as parseutils.h) includes <time.h>, the compiler will first find the time.h from the avutil folder since it has been added as an include path by Android's build system. This prevents the compiler from finding time.h in the ANSI C library. While at the core this is really a fault of Android's build system, it is much more easily fixed by renaming a file compared to creating a new build system specifically for FFmpeg on Android or any other build systems that set include directories in this fashion.

Replying to cehoyos:

Replying to dlongest:

Correct me if I am wrong, but giving a header the same file name as one in the ANSI C standards may not fall under best practices anyways. Possibly "avtime.h" would be a better fit in this case.

So if I understand you correctly you suggest that instead of you using the correct "#include <libavutil/avutil.h> (or whatever the use case might be), we should break API?

Or to try it differently:
Please provide the C program and (the Makefile or the gcc command line that fail). As you found out, neither the Makefile nor the gcc command line must contain "-I/usr/include/libavutil".

Or am I missing something?

comment:6 follow-up: Changed 4 years ago by cehoyos

Is the incorrect -I flag (should be -Ijni/ffmpeg) also added with http://sourceforge.net/projects/ffmpeg4android/ ?

comment:7 in reply to: ↑ 6 Changed 4 years ago by obucinac

Replying to cehoyos:

Is the incorrect -I flag (should be -Ijni/ffmpeg) also added with http://sourceforge.net/projects/ffmpeg4android/ ?

Actually, thats my project, and this is the reason why I cant release makefiles for 1.0 and latest HEAD branch. I even got 7.0 build, but I still cant build most important branches - 1.0 and HEAD.
"-I libavutil" is inserted into command line by Android build system and I cant prevent that.

EDIT:

Example
Android.mk have two include folders

LOCAL_C_INCLUDES := \
    $(FFMPEG_ROOT_DIR)/$(FFMPEG_CONFIG_DIR) \
    $(FFMPEG_ROOT_DIR)

Thats first two -I parameters in the command line Android build system creates. Other -I parameters are included by build system.

prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/arm-linux-androideabi-gcc -I external/ffmpeg-HEAD-14fd34d.android/android/full-eng -I external/ffmpeg-HEAD-14fd34d.android -I external/ffmpeg-HEAD-14fd34d.android/libavutil -I out/target/product/generic/obj/SHARED_LIBRARIES/libavutil-HEAD-1.0_intermediates -I libnativehelper/include/nativehelper  -isystem system/core/include -isystem hardware/libhardware/include -isystem hardware/libhardware_legacy/include -isystem hardware/ril/include -isystem libnativehelper/include -isystem frameworks/native/include -isystem frameworks/native/opengl/include -isystem frameworks/av/include -isystem frameworks/base/include -isystem frameworks/base/opengl/include -isystem external/skia/include -isystem out/target/product/generic/obj/include -isystem bionic/libc/arch-arm/include -isystem bionic/libc/include -isystem bionic/libstdc++/include -isystem bionic/libc/kernel/common -isystem bionic/libc/kernel/arch-arm -isystem bionic/libm/include -isystem bionic/libm/include/arm -isystem bionic/libthread_db/include -c  -fno-exceptions -Wno-multichar -msoft-float -fpic -fPIE -ffunction-sections -fdata-sections -funwind-tables -fstack-protector -Wa,--noexecstack -Werror=format-security -fno-short-enums -march=armv7-a -mfloat-abi=softfp -mfpu=vfpv3-d16 -include system/core/include/arch/linux-arm/AndroidConfig.h -I system/core/include/arch/linux-arm/ -Wno-unused-but-set-variable -fno-builtin-sin -fno-strict-volatile-bitfields -Wno-psabi -mthumb-interwork -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -DNDEBUG -g -Wstrict-aliasing=2 -fgcse-after-reload -frerun-cse-after-loop -frename-registers -DNDEBUG -UDEBUG -mthumb -Os -fomit-frame-pointer -fno-strict-aliasing   -DHAVE_AV_CONFIG_H -march=armv7-a -mfloat-abi=softfp -mfpu=vfpv3-d16  -std=c99 -fomit-frame-pointer -fPIC -marm -g -Wdeclaration-after-statement -Wall -Wno-parentheses -Wno-switch -Wno-format-zero-length -Wdisabled-optimization -Wpointer-arith -Wredundant-decls -Wno-pointer-sign -Wwrite-strings -Wtype-limits -Wundef -Wmissing-prototypes -Wno-pointer-to-int-cast -Wstrict-prototypes -O3 -fno-math-errno -fno-signed-zeros -fno-tree-vectorize -Werror=implicit-function-declaration -Werror=missing-prototypes      -MD -MF out/target/product/generic/obj/SHARED_LIBRARIES/libavutil-HEAD-1.0_intermediates/parseutils.d -o out/target/product/generic/obj/SHARED_LIBRARIES/libavutil-HEAD-1.0_intermediates/parseutils.o external/ffmpeg-HEAD-14fd34d.android/libavutil/parseutils.c
Version 1, edited 4 years ago by obucinac (previous) (next) (diff)

comment:8 Changed 4 years ago by cehoyos

If you are right and you cannot remove the wrong include path from the gcc call (I don't know), then please follow my original suggestion and patch the FFmpeg build system so it also works with the Android NDK.

comment:9 follow-up: Changed 4 years ago by Cigaes

Can you point us to the bug report against the Android build system?

comment:10 in reply to: ↑ 9 Changed 4 years ago by saste

Replying to Cigaes:

Can you point us to the bug report against the Android build system?

Yes, I think it should be better to get this properly fixed in the Android build system. Asking every different project not to duplicate an header name used in another random project (being it the standard C lib or whatever else) is going to be an hell, this should be really fixed at the Android build system level.

In other words the library prefix, assuming it is unique, provides the header namespace for the given library and using the correct include path (rather than assuming that all headers are in a flat namespace as done by the Android build system) would prevent the kind of failure that you're observing.

I'm not against renaming time.h -> avtime.h, but this would break API compatibility so you're trading a failure with another failure.

comment:11 follow-up: Changed 4 years ago by reimar

I think there is a patch for the rename on the list.
But I'd really like to know why you have to use that obfuscated piece of crap that is the Android build system?
I've compiled many different programs just fine using the Android toolchain without ever using it, instead using each projects own build system, usually without needing any modifications.

comment:12 in reply to: ↑ 11 Changed 4 years ago by cehoyos

Replying to reimar:

I've compiled many different programs just fine using the Android toolchain without ever using it, instead using each projects own build system, usually without needing any modifications.

Iirc, the claim is that this does not work for FFmpeg (ie that some modifications are necessary). Could you comment on that?

comment:13 Changed 4 years ago by obucinac

I managed to resolve initial problem by rewriting Android makefiles, and this looks like best solution.

comment:15 Changed 4 years ago by Granjow

If someone else finds error messages like those and uses CMake for finding ffmpeg (which automatically returns an absolute path):

In file included from /data/cworkspace/phenoCam/src/lib/videoTools/avi-timeextract.h:17:0,

from /data/cworkspace/phenoCam/src/lib/videoTools/avi-timeextract.cpp:12:

/usr/include/c++/4.7/ctime:62:11: error: ‘::clock_t’ has not been declared
/usr/include/c++/4.7/ctime:63:11: error: ‘::time_t’ has not been declared
/usr/include/c++/4.7/ctime:66:11: error: ‘::clock’ has not been declared
/usr/include/c++/4.7/ctime:67:11: error: ‘::difftime’ has not been declared
/usr/include/c++/4.7/ctime:68:11: error: ‘::mktime’ has not been declared
/usr/include/c++/4.7/ctime:69:11: error: ‘::time’ has not been declared
/usr/include/c++/4.7/ctime:70:11: error: ‘::asctime’ has not been declared
/usr/include/c++/4.7/ctime:71:11: error: ‘::ctime’ has not been declared
/usr/include/c++/4.7/ctime:72:11: error: ‘::gmtime’ has not been declared
/usr/include/c++/4.7/ctime:73:11: error: ‘::localtime’ has not been declared
/usr/include/c++/4.7/ctime:74:11: error: ‘::strftime’ has not been declared

Use something like this to correct the path:

string(REPLACE "/usr/include/" "" FFMPEG_INCLUDE_DIR "${FFMPEG_INCLUDE_DIR}")

(Ugly hack but works)

comment:16 Changed 4 years ago by richardpl

  • Resolution set to wontfix
  • Status changed from new to closed

comment:17 Changed 4 years ago by saste

Proper fix should be at the Android build system level, so it is technically unrelated to FFmpeg.

comment:18 Changed 2 years ago by llogan

Note to Xcode users experiencing a similar issue: uncheck the "Recursive" box under "Header Search Path". This is unchecked by default.

Note: See TracTickets for help on using tickets.