Opened 11 years ago

Closed 11 years ago

Last modified 10 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 by Carl Eugen Hoyos, 11 years ago

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

comment:2 by burek, 11 years ago

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 by dlongest, 11 years ago

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.

in reply to:  3 ; comment:4 by Carl Eugen Hoyos, 11 years ago

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 11 years ago by Carl Eugen Hoyos (previous) (diff)

in reply to:  4 comment:5 by dlongest, 11 years ago

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 by Carl Eugen Hoyos, 11 years ago

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

in reply to:  6 comment:7 by obucinac, 11 years ago

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 0.7 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
Last edited 11 years ago by obucinac (previous) (diff)

comment:8 by Carl Eugen Hoyos, 11 years ago

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 by Cigaes, 11 years ago

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

in reply to:  9 comment:10 by Stefano Sabatini, 11 years ago

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 by reimar, 11 years ago

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.

in reply to:  11 comment:12 by Carl Eugen Hoyos, 11 years ago

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 by obucinac, 11 years ago

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

comment:15 by Granjow, 11 years ago

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 by Elon Musk, 11 years ago

Resolution: wontfix
Status: newclosed

comment:17 by Stefano Sabatini, 11 years ago

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

comment:18 by llogan, 10 years ago

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.