| Message ID | 20170506215913.7676-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 3e295e633c36f601fb3dbee533d7b5dfc8c77bef |
| Headers | show |
On 5/6/2017 2:59 PM, James Almer wrote: > It's not used by anything, has dubious usefulness, the reasons for which > it was introduced are no longer valid, and only serves to add complexity > to the build system. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Makefile | 6 ------ > configure | 2 -- > ffbuild/library.mak | 2 +- > ffbuild/libversion.sh | 2 -- > 4 files changed, 1 insertion(+), 11 deletions(-) > > diff --git a/Makefile b/Makefile > index d414cf841e..d177311262 100644 > --- a/Makefile > +++ b/Makefile > @@ -107,12 +107,6 @@ $(1) := > $(1)-yes := > endef > > -ifdef CONFIG_RAISE_MAJOR > -RAISE_MAJOR = 100 > -else > -RAISE_MAJOR = 0 > -endif > - > define DOSUBDIR > $(foreach V,$(SUBDIR_VARS),$(eval $(call RESET,$(V)))) > SUBDIR := $(1)/ > diff --git a/configure b/configure > index b76f9ce567..e28f27a739 100755 > --- a/configure > +++ b/configure > @@ -109,7 +109,6 @@ Configuration options: > --enable-gray enable full grayscale support (slower color) > --disable-swscale-alpha disable alpha channel support in swscale > --disable-all disable building components, libraries and programs > - --enable-raise-major increase major version numbers in sonames [no] > > Program options: > --disable-programs do not build command line programs > @@ -1686,7 +1685,6 @@ CONFIG_LIST=" > neon_clobber_test > ossfuzz > pic > - raise_major > thumb > valgrind_backtrace > xmm_clobber_test > diff --git a/ffbuild/library.mak b/ffbuild/library.mak > index cfc2d36067..22f1e4c37f 100644 > --- a/ffbuild/library.mak > +++ b/ffbuild/library.mak > @@ -34,7 +34,7 @@ $(TESTPROGS) $(TOOLS): %$(EXESUF): %.o > $$(LD) $(LDFLAGS) $(LDEXEFLAGS) $$(LD_O) $$(filter %.o,$$^) $$(THISLIB) $(FFEXTRALIBS) $$(ELIBS) > > $(SUBDIR)lib$(NAME).version: $(SUBDIR)version.h | $(SUBDIR) > - $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< $(RAISE_MAJOR) > $$@ > + $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< > $$@ > > $(SUBDIR)lib$(FULLNAME).pc: $(SUBDIR)version.h | $(SUBDIR) > $$(M) $$(SRC_PATH)/ffbuild/pkgconfig_generate.sh $(NAME) "$(DESC)" > diff --git a/ffbuild/libversion.sh b/ffbuild/libversion.sh > index 687adf28bc..990ce9f640 100755 > --- a/ffbuild/libversion.sh > +++ b/ffbuild/libversion.sh > @@ -5,10 +5,8 @@ toupper(){ > name=lib$1 > ucname=$(toupper ${name}) > file=$2 > -raise_major=$3 > > eval $(awk "/#define ${ucname}_VERSION_M/ { print \$2 \"=\" \$3 }" "$file") > -eval ${ucname}_VERSION_MAJOR=$((${ucname}_VERSION_MAJOR+${raise_major})) > eval ${ucname}_VERSION=\$${ucname}_VERSION_MAJOR.\$${ucname}_VERSION_MINOR.\$${ucname}_VERSION_MICRO > eval echo "${name}_VERSION=\$${ucname}_VERSION" > eval echo "${name}_VERSION_MAJOR=\$${ucname}_VERSION_MAJOR" > LGTM. However, it seems that some documentation needs to be updated as well. In doc/developer.texi, it states: "Incrementing the third component means a noteworthy binary compatible change (e.g. encoder bug fix that matters for the decoder). The third component always starts at 100 to distinguish FFmpeg from Libav." Note that my review only covers the content of the patch, and I don't know whether or not it makes sense to discontinue this practice. Aaron Levinson
On 5/6/2017 11:19 PM, Aaron Levinson wrote: > On 5/6/2017 2:59 PM, James Almer wrote: >> It's not used by anything, has dubious usefulness, the reasons for which >> it was introduced are no longer valid, and only serves to add complexity >> to the build system. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Makefile | 6 ------ >> configure | 2 -- >> ffbuild/library.mak | 2 +- >> ffbuild/libversion.sh | 2 -- >> 4 files changed, 1 insertion(+), 11 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index d414cf841e..d177311262 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -107,12 +107,6 @@ $(1) := >> $(1)-yes := >> endef >> >> -ifdef CONFIG_RAISE_MAJOR >> -RAISE_MAJOR = 100 >> -else >> -RAISE_MAJOR = 0 >> -endif >> - >> define DOSUBDIR >> $(foreach V,$(SUBDIR_VARS),$(eval $(call RESET,$(V)))) >> SUBDIR := $(1)/ >> diff --git a/configure b/configure >> index b76f9ce567..e28f27a739 100755 >> --- a/configure >> +++ b/configure >> @@ -109,7 +109,6 @@ Configuration options: >> --enable-gray enable full grayscale support (slower color) >> --disable-swscale-alpha disable alpha channel support in swscale >> --disable-all disable building components, libraries and >> programs >> - --enable-raise-major increase major version numbers in sonames >> [no] >> >> Program options: >> --disable-programs do not build command line programs >> @@ -1686,7 +1685,6 @@ CONFIG_LIST=" >> neon_clobber_test >> ossfuzz >> pic >> - raise_major >> thumb >> valgrind_backtrace >> xmm_clobber_test >> diff --git a/ffbuild/library.mak b/ffbuild/library.mak >> index cfc2d36067..22f1e4c37f 100644 >> --- a/ffbuild/library.mak >> +++ b/ffbuild/library.mak >> @@ -34,7 +34,7 @@ $(TESTPROGS) $(TOOLS): %$(EXESUF): %.o >> $$(LD) $(LDFLAGS) $(LDEXEFLAGS) $$(LD_O) $$(filter %.o,$$^) >> $$(THISLIB) $(FFEXTRALIBS) $$(ELIBS) >> >> $(SUBDIR)lib$(NAME).version: $(SUBDIR)version.h | $(SUBDIR) >> - $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< >> $(RAISE_MAJOR) > $$@ >> + $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< > $$@ >> >> $(SUBDIR)lib$(FULLNAME).pc: $(SUBDIR)version.h | $(SUBDIR) >> $$(M) $$(SRC_PATH)/ffbuild/pkgconfig_generate.sh $(NAME) "$(DESC)" >> diff --git a/ffbuild/libversion.sh b/ffbuild/libversion.sh >> index 687adf28bc..990ce9f640 100755 >> --- a/ffbuild/libversion.sh >> +++ b/ffbuild/libversion.sh >> @@ -5,10 +5,8 @@ toupper(){ >> name=lib$1 >> ucname=$(toupper ${name}) >> file=$2 >> -raise_major=$3 >> >> eval $(awk "/#define ${ucname}_VERSION_M/ { print \$2 \"=\" \$3 }" >> "$file") >> -eval ${ucname}_VERSION_MAJOR=$((${ucname}_VERSION_MAJOR+${raise_major})) >> eval >> ${ucname}_VERSION=\$${ucname}_VERSION_MAJOR.\$${ucname}_VERSION_MINOR.\$${ucname}_VERSION_MICRO >> >> eval echo "${name}_VERSION=\$${ucname}_VERSION" >> eval echo "${name}_VERSION_MAJOR=\$${ucname}_VERSION_MAJOR" >> > > LGTM. However, it seems that some documentation needs to be updated as > well. In doc/developer.texi, it states: "Incrementing the third > component means a noteworthy binary compatible change (e.g. encoder bug > fix that matters for the decoder). The third component always starts at > 100 to distinguish FFmpeg from Libav." Note that my review only covers > the content of the patch, and I don't know whether or not it makes sense > to discontinue this practice. That line has nothing to do with this option. It's talking about the third component in a library version, that is, the micro version, and applies to every build regardless of configure options. Unlike minor versions which start at 0 and reset back to that after a major bump, micro versions start at 100 and reset back to that as well after a minor and/or major bump.
On 5/6/2017 6:59 PM, James Almer wrote: > It's not used by anything, has dubious usefulness, the reasons for which > it was introduced are no longer valid, and only serves to add complexity > to the build system. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Makefile | 6 ------ > configure | 2 -- > ffbuild/library.mak | 2 +- > ffbuild/libversion.sh | 2 -- > 4 files changed, 1 insertion(+), 11 deletions(-) Ping. Will be pushing this soon.
On 5/15/2017 8:59 PM, James Almer wrote: > On 5/6/2017 6:59 PM, James Almer wrote: >> It's not used by anything, has dubious usefulness, the reasons for which >> it was introduced are no longer valid, and only serves to add complexity >> to the build system. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Makefile | 6 ------ >> configure | 2 -- >> ffbuild/library.mak | 2 +- >> ffbuild/libversion.sh | 2 -- >> 4 files changed, 1 insertion(+), 11 deletions(-) > > Ping. Will be pushing this soon. Pushed.
diff --git a/Makefile b/Makefile index d414cf841e..d177311262 100644 --- a/Makefile +++ b/Makefile @@ -107,12 +107,6 @@ $(1) := $(1)-yes := endef -ifdef CONFIG_RAISE_MAJOR -RAISE_MAJOR = 100 -else -RAISE_MAJOR = 0 -endif - define DOSUBDIR $(foreach V,$(SUBDIR_VARS),$(eval $(call RESET,$(V)))) SUBDIR := $(1)/ diff --git a/configure b/configure index b76f9ce567..e28f27a739 100755 --- a/configure +++ b/configure @@ -109,7 +109,6 @@ Configuration options: --enable-gray enable full grayscale support (slower color) --disable-swscale-alpha disable alpha channel support in swscale --disable-all disable building components, libraries and programs - --enable-raise-major increase major version numbers in sonames [no] Program options: --disable-programs do not build command line programs @@ -1686,7 +1685,6 @@ CONFIG_LIST=" neon_clobber_test ossfuzz pic - raise_major thumb valgrind_backtrace xmm_clobber_test diff --git a/ffbuild/library.mak b/ffbuild/library.mak index cfc2d36067..22f1e4c37f 100644 --- a/ffbuild/library.mak +++ b/ffbuild/library.mak @@ -34,7 +34,7 @@ $(TESTPROGS) $(TOOLS): %$(EXESUF): %.o $$(LD) $(LDFLAGS) $(LDEXEFLAGS) $$(LD_O) $$(filter %.o,$$^) $$(THISLIB) $(FFEXTRALIBS) $$(ELIBS) $(SUBDIR)lib$(NAME).version: $(SUBDIR)version.h | $(SUBDIR) - $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< $(RAISE_MAJOR) > $$@ + $$(M) $$(SRC_PATH)/ffbuild/libversion.sh $(NAME) $$< > $$@ $(SUBDIR)lib$(FULLNAME).pc: $(SUBDIR)version.h | $(SUBDIR) $$(M) $$(SRC_PATH)/ffbuild/pkgconfig_generate.sh $(NAME) "$(DESC)" diff --git a/ffbuild/libversion.sh b/ffbuild/libversion.sh index 687adf28bc..990ce9f640 100755 --- a/ffbuild/libversion.sh +++ b/ffbuild/libversion.sh @@ -5,10 +5,8 @@ toupper(){ name=lib$1 ucname=$(toupper ${name}) file=$2 -raise_major=$3 eval $(awk "/#define ${ucname}_VERSION_M/ { print \$2 \"=\" \$3 }" "$file") -eval ${ucname}_VERSION_MAJOR=$((${ucname}_VERSION_MAJOR+${raise_major})) eval ${ucname}_VERSION=\$${ucname}_VERSION_MAJOR.\$${ucname}_VERSION_MINOR.\$${ucname}_VERSION_MICRO eval echo "${name}_VERSION=\$${ucname}_VERSION" eval echo "${name}_VERSION_MAJOR=\$${ucname}_VERSION_MAJOR"
It's not used by anything, has dubious usefulness, the reasons for which it was introduced are no longer valid, and only serves to add complexity to the build system. Signed-off-by: James Almer <jamrial@gmail.com> --- Makefile | 6 ------ configure | 2 -- ffbuild/library.mak | 2 +- ffbuild/libversion.sh | 2 -- 4 files changed, 1 insertion(+), 11 deletions(-)