On Sun, Jul 22, 2012 at 01:50:03AM +0100, Mans Rullgard wrote:
> This adds a full identification probe of CC, AS, LD and HOSTCC,
> and sets up correct flags and dependency tracking for each.
This is shaping up to be a big step forward. But it's very tricky
code with a lot of subtle name mangling, not easy to review at all...
> --- a/Makefile
> +++ b/Makefile
> @@ -25,14 +25,15 @@ ALLFFLIBS = avcodec avdevice avfilter avformat avresample
> avutil swscale
> IFLAGS := -I. -I$(SRC_PATH)
> CPPFLAGS := $(IFLAGS) $(CPPFLAGS)
> CFLAGS += $(ECFLAGS)
> -CCFLAGS = $(CFLAGS)
> +CCFLAGS = $(CPPFLAGS) $(CFLAGS)
> +ASFLAGS := $(CPPFLAGS) $(ASFLAGS)
> YASMFLAGS += $(IFLAGS) -I$(SRC_PATH)/libavutil/x86/ -Pconfig.asm
> HOSTCFLAGS += $(IFLAGS)
> LDFLAGS := $(ALLFFLIBS:%=-Llib%) $(LDFLAGS)
The header compilation rule still lists both CPPFLAGS and CFLAGS
explicitly and should likely be switched to just CCFLAGS.
> define COMPILE
> - $($(1)DEP)
> - $($(1)) $(CPPFLAGS) $($(1)FLAGS) $($(1)_DEPFLAGS) -c $($(1)_O) $<
> + $(call $(1)DEP,$(1))
> + $($(1)) $($(1)FLAGS) $($(1)_DEPFLAGS) -c $($(1)_O) $<
> endef
FOOFLAGS vs. FOO_DEPFLAGS is an inconsistency. Given how involved all
of this is getting, such inconsistencies start to become a problem IMO.
What about the rules for HOSTOBJS and HOSTPROGS in common.mak? They
still have hardcoded "-o $@" syntax in them.
> --- a/configure
> +++ b/configure
> @@ -1725,7 +1725,9 @@ ldflags_filter=echo
>
> AS_O='-o $@'
> CC_O='-o $@'
> +LD_O='-o $@'
>
> +HOSTCC_O='-o $@'
> host_cflags='-D_ISOC99_SOURCE -D_XOPEN_SOURCE=600 -O3 -g'
> host_libs='-lm'
> host_cflags_filter=echo
IMO it makes more sense to keep the _O variables together, whatever ...
> @@ -1735,8 +1737,8 @@ target_path='$(CURDIR)'
>
> # since the object filename is not given with the -MM flag, the compiler
> # is only able to print the basename, and we must add the path ourselves
> -DEPEND_CMD='$(DEPCC) $(DEPFLAGS) $< | sed -e "/^\#.*/d" -e
> "s,^[[:space:]]*$(*F)\\.o,$(@D)/$(*F).o," > $(@:.o=.d)'
> -DEPFLAGS='$(CPPFLAGS) $(CFLAGS) -MM'
> +DEPEND_CMD='$(DEP$(1)) $(DEP$(1)FLAGS) $($(1)DEP_FLAGS) $< | sed -e
> "/^\#.*/d" -e "s,^[[:space:]]*$(*F)\\.o,$(@D)/$(*F).o," > $(@:.o=.d)'
> +DEPFLAGS='-MM'
Please rename DEPEND_CMD to DEPCMD for consistency.
> # find source path
> if test -f configure; then
> @@ -2036,120 +2038,150 @@ tms470_flags(){
> done
> }
>
> -if $cc -v 2>&1 | grep -q '^gcc.*LLVM'; then
> + if $_cc -v 2>&1 | grep -q '^gcc.*LLVM'; then
nit: The extra spaces here are slightly silly. Maybe you can drop them
while you're at it.
> +set_ccvars(){
> + eval ${1}_O=\${_cc_o-\${${1}_O}}
> +
> + if [ -n "$_depflags" ]; then
> + eval ${1}_DEPFLAGS=\$_depflags
> + else
> + eval ${1}DEP=\${_DEPCMD:-\$DEPEND_CMD}
> + eval ${1}DEP_FLAGS=\${_DEPFLAGS:-\$DEPFLAGS}
> + eval DEP${1}FLAGS=\$_flags
> + fi
> +}
Here's where the underscore inconsistency becomes visible.
It does get confusing.
Through uppercasing a variable you convey the information whether or
not a compiler can generate dependency information as a side-effect
of compilation. Just setting a flag and testing that would be a
more obvious way of achieving the same thing.
I also wonder if FOO_DEPFLAGS should be kept separate of the normal
FOOFLAGS at all when generating dependency information as a side-effect.
> @@ -3370,9 +3419,10 @@ SLIBPREF=$SLIBPREF
> SLIBSUF=$SLIBSUF
> EXESUF=$EXESUF
> EXTRA_VERSION=$extra_version
> -DEPFLAGS=$DEPFLAGS
> CCDEP=$CCDEP
> +CCDEP_FLAGS=$CCDEP_FLAGS
> ASDEP=$ASDEP
> +ASDEP_FLAGS=$ASDEP_FLAGS
> CC_DEPFLAGS=$CC_DEPFLAGS
> AS_DEPFLAGS=$AS_DEPFLAGS
> HOSTCC=$host_cc
CCDEP_FLAGS vs. CC_DEPFLAGS ...
Heads are starting to spin everywhere ...
I think CCDEP_FLAGS should be CCDEPFLAGS. In all other cases the flags
required when invoking FOO do not have an underscore in their name, cf
CCFLAGS, ASFLAGS and similar.
> @@ -3380,6 +3430,12 @@ HOSTCFLAGS=$host_cflags
> HOSTEXESUF=$HOSTEXESUF
> HOSTLDFLAGS=$host_ldflags
> HOSTLIBS=$host_libs
> +DEPHOSTCC=$host_cc
What about the case when deps are not generated with the host compiler?
For the target compiler we set this separately:
DEPCC=$dep_cc
> +DEPHOSTCCFLAGS=$DEPHOSTCCFLAGS \$(CPPFLAGS)
As you mentioned on IRC, these are the target CPPFLAGS.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
