[vlc-devel] [PATCH 5/6] contrib: protobuf: build protoc from contribs

[Rémi Denis-Courmont]

Le tiistaina 24. maaliskuuta 2020, 11.32.09 EET Steve Lhomme a écrit :
> On 2020-03-23 17:23, Rémi Denis-Courmont wrote:
> > Le maanantaina 23. maaliskuuta 2020, 17.27.26 EET Steve Lhomme a écrit :
> >> Similar to how it's done for luac: we have a triplet-protoc in
> >> contrib/bin.
> >> ---
> >> 
> >>   configure.ac                   | 10 +++++++++-
> >>   contrib/src/protobuf/rules.mak | 19 +++++++++++++++++--
> >>   2 files changed, 26 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/configure.ac b/configure.ac
> >> index bc1874cb97c..8af142d48bd 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -502,6 +502,12 @@ AS_IF([test -n "${CONTRIB_DIR}"], [
> >> 
> >>       ])
> >>     
> >>     ])
> >> 
> >> +  AS_IF([test -z "$PROTOC"], [
> >> +    AS_IF([test -x
> >> "${CONTRIB_DIR}/../bin/${host_alias}-protoc${BUILDEXEEXT}"], [ +
> >> PROTOC="${CONTRIB_DIR}/../bin/${host_alias}-protoc${BUILDEXEEXT}" +    ])
> >> +  ])
> >> +
> >> 
> >>     AS_IF([test "${SYS}" = "darwin"], [
> >>     
> >>       export LD_LIBRARY_PATH="${CONTRIB_DIR}/lib:$LD_LIBRARY_PATH"
> >>       export DYLD_LIBRARY_PATH="${CONTRIB_DIR}/lib:$DYLD_LIBRARY_PATH"
> >> 
> >> @@ -3870,7 +3876,9 @@ dnl  Chromecast streaming support
> >> 
> >>   dnl
> >>   m4_pushdef([protobuf_lite_version], 2.5.0)
> >>   AC_ARG_VAR(PROTOC, [protobuf compiler])
> >> 
> >> -AC_CHECK_PROGS(PROTOC, protoc, no)
> >> +AS_IF([test -z "$PROTOC"], [
> >> +   AC_CHECK_TOOL(PROTOC, protoc, no)
> >> +])
> >> 
> >>   PKG_WITH_MODULES([CHROMECAST],[protobuf-lite >= protobuf_lite_version],
> >>   [
> >>   
> >>       AS_IF([test "x${PROTOC}" != "xno"], [
> >>       
> >>           build_chromecast="yes"
> > 
> > I don't know if this is right or wrong, but it belongs in a separate
> > patch.
> 
> If you mean the lines above, it's related as in the case he host-protoc
> is found we use that and don't test it (like luac). Otherwise we revert
> to the system detection. No reason to change from AC_CHECK_PROGS to
> AC_CHECK_TOOL. I'll change that.

Again, not saying it's wrong per se, but it's preferable not to patch contrib 
and main at the same time.

> 
> >> diff --git a/contrib/src/protobuf/rules.mak
> >> b/contrib/src/protobuf/rules.mak index c505ef7548b..28f38445566 100644
> >> --- a/contrib/src/protobuf/rules.mak
> >> +++ b/contrib/src/protobuf/rules.mak
> >> @@ -2,9 +2,10 @@
> >> 
> >>   PROTOBUF_VERSION := 3.1.0
> >>   PROTOBUF_URL :=
> >> 
> >> https://github.com/google/protobuf/releases/download/v$(PROTOBUF_VERSION)
> >> /p
> >> rotobuf-cpp-$(PROTOBUF_VERSION).tar.gz
> >> 
> >> -PKGS += protobuf
> >> +PKGS += protobuf protoc
> >> +PKGS_PROGS += protoc
> >> 
> >>   ifeq ($(call need_pkg, "protobuf-lite >= 3.1.0 protobuf-lite <
> >>   3.2.0"),)
> >> 
> >> -PKGS_FOUND += protobuf
> >> +PKGS_FOUND += protobuf protoc
> >> 
> >>   else
> >>   # check we have a matching protoc to use
> >>   PROTOC_ABSPATH = $(shell PATH="$(PATH)" which protoc)
> > 
> > Won't work. See how luac does it.
> 
> You mean adding protoc to PKGS_ALL ?

at least

> >> @@ -44,3 +45,17 @@ protobuf: protobuf-$(PROTOBUF_VERSION)-cpp.tar.gz
> >> .sum-protobuf cd $< && $(HOSTVARS) ./configure $(HOSTCONF)
> >> --with-protoc="$(PROTOC)" cd $< && $(MAKE) && $(MAKE) install
> >> 
> >>   	touch $@
> >> 
> >> +
> >> +.sum-protoc: .sum-protobuf
> >> +	touch $@
> >> +
> >> +protoc: protobuf-$(PROTOBUF_VERSION)-cpp.tar.gz .sum-protoc
> >> +	$(UNPACK)
> >> +	mv protobuf-$(PROTOBUF_VERSION) protobuf-$(PROTOBUF_VERSION)-cpp
> >> +	$(APPLY) $(SRC)/protobuf/protobuf-win32.patch
> >> +	$(MOVE)
> >> +
> >> +.protoc: protoc
> >> +	cd $< && $(BUILDVARS) ./configure $(BUILDCONF)
> >> +	cd $< && $(MAKE) && $(MAKE) install
> >> +	touch $@
> > 
> > That will install plenty of crap that we most probably don't want to
> > install.
> As the great philosopher once said: storage is cheap.
 > And it's not that much.

I'm not worried about disk space here. I'm worried about two things:
1) unexpected/unhandled new directories directly under contrib/
  (currently, we only handle bin/)
2) filename conflicts between multiple platforms in the same contrib/ directory
  (currently, we prefix everything with the host triplet)



-- 
Rémi Denis-Courmont
http://www.remlab.net/