[vlc-devel] [PATCH] contrib: don't use standard cmake variable name CMAKE_GENERATOR

[Alexandre Janniaux]

Hi,

On Tue, Nov 03, 2020 at 01:31:37AM +0800, Zhao Zhili wrote:
> A new patch support generator other than makefile is attached. (git send-email failed again and again).
> I have test build on macOS and Linux with ninja. More tests are needed.
>

This looks very satisfactory. :)
Well done.

Regards,
--
Alexandre Janniaux
Videolabs

>
>
> > On Nov 3, 2020, at 12:00 AM, Steve Lhomme <robux4 at ycbcr.xyz> wrote:
> >
> > On 2020-11-02 16:44, Zhao Zhili wrote:
> >> Hi Steve,
> >> The commit message is not clear enough. There are two issues the patch try to resolve:
> >> First, the conflict setting/using of CMAKE_GENERATOR. If user set CMAKE_GENERATOR as Ninja, cmake $CMAKE_GENERATOR is unexpected.
> >> Second, since we always call make after cmake configure, generator other than makefile doesn't work. So we have to reset/ignore the CMAKE_GENERATOR environment variable.
> >
> > Indeed.
> >
> >> I tried to make generator other than makefile work by run build via cmake wrapper, like
> >> $(cmake) --build . --target install.
> >> Since we put too much thing in the $(cmake) variable, it doesn't work. I think it could be fixed by add $(cmake_prog) as plain cmake wrapper.
> >
> > Building cmake contribs with Ninja could be nice. We already use it for Meson contribs.
> >
> >> CMAKE_GENERATOR is being reset to empty on most platform, because there are multiple makefile variant, I'm not sure does "Unix Makefiles" works on every platform.
> >
> > Maybe not but as you said we call "make" afterwards so we expect to have that kind of Makefile. If a target doesn't support makefiles, it will not support all the contribs we use in VLC anyway. So we don't really have to handle that case.
> >
> >> Some explanations was inlined.
> >>> On Nov 2, 2020, at 10:45 PM, Steve Lhomme <robux4 at ycbcr.xyz <mailto:robux4 at ycbcr.xyz>> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 2020-11-02 15:18, Zhao Zhili wrote:
> >>>> Fix build error when user have CMAKE_GENERATOR as environment variable,
> >>>> like CMAKE_GENERATOR=Ninja.
> >>>> ---
> >>>>  contrib/bootstrap          | 2 +-
> >>>>  contrib/src/main.mak       | 5 +++--
> >>>>  contrib/src/soxr/rules.mak | 2 +-
> >>>>  3 files changed, 5 insertions(+), 4 deletions(-)
> >>>> diff --git a/contrib/bootstrap b/contrib/bootstrap
> >>>> index 5ed5f6f25d..4da86af994 100755
> >>>> --- a/contrib/bootstrap
> >>>> +++ b/contrib/bootstrap
> >>>> @@ -256,7 +256,7 @@ test -z "$GNUV3" || add_make_enabled "GNUV3"
> >>>>  test -z "$AD_CLAUSES" || add_make_enabled "AD_CLAUSES"
> >>>>  test -z "$WITH_OPTIMIZATION" || add_make_enabled "WITH_OPTIMIZATION"
> >>>>  test -z "$ENABLE_PDB" || add_make_enabled "ENABLE_PDB"
> >>>> -test "`uname -o 2>/dev/null`" != "Msys" || add_make "CMAKE_GENERATOR := -G \"MSYS Makefiles\""
> >>>> +test "`uname -o 2>/dev/null`" != "Msys" || add_make "CONTRIB_CMAKE_GENERATOR := MSYS Makefiles"
> >>>
> >>> We only set this value when building for Msys. Is Ninja expected to work in that case ? Or whatever else is set in the environment ?
> >>>
> >>>>  #
> >>>>  # Checks
> >>>> diff --git a/contrib/src/main.mak b/contrib/src/main.mak
> >>>> index aa273ef89c..7b395cd13f 100644
> >>>> --- a/contrib/src/main.mak
> >>>> +++ b/contrib/src/main.mak
> >>>> @@ -392,8 +392,9 @@ AUTORECONF = autoreconf
> >>>>  endif
> >>>>  RECONF = mkdir -p -- $(PREFIX)/share/aclocal && \
> >>>> cd $< && $(AUTORECONF) -fiv $(ACLOCAL_AMFLAGS)
> >>>> -CMAKE = cmake . -DCMAKE_TOOLCHAIN_FILE=$(abspath toolchain.cmake) \
> >>>> --DCMAKE_INSTALL_PREFIX=$(PREFIX) $(CMAKE_GENERATOR) \
> >>>> +CMAKE = CMAKE_GENERATOR=$(CONTRIB_CMAKE_GENERATOR) cmake . \
> >>>
> >>> Here you force the value to CONTRIB_CMAKE_GENERATOR which 99.9% of the time is never set, effectively overriding the environment preferred value ?
> >> It is intended to reset CMAKE_GENERATOR.
> >
> > OK, Did you try on Msys ? (or forcing the value as it would for Msys) ? I suspect the space in the variable will break this line. It should probably be double quoted:
> > CMAKE_GENERATOR="$(CONTRIB_CMAKE_GENERATOR)"
> >
> >>>
> >>>> +-DCMAKE_TOOLCHAIN_FILE=$(abspath toolchain.cmake) \
> >>>> +-DCMAKE_INSTALL_PREFIX=$(PREFIX) \
> >>>> -DBUILD_SHARED_LIBS:BOOL=OFF
> >>>>  ifdef HAVE_WIN32
> >>>>  CMAKE += -DCMAKE_DEBUG_POSTFIX:STRING=
> >>>> diff --git a/contrib/src/soxr/rules.mak b/contrib/src/soxr/rules.mak
> >>>> index 3981271743..c68b1e40f0 100644
> >>>> --- a/contrib/src/soxr/rules.mak
> >>>> +++ b/contrib/src/soxr/rules.mak
> >>>> @@ -41,6 +41,6 @@ endif
> >>>> -DWITH_LSR_BINDINGS=OFF \
> >>>> -DWITH_OPENMP=OFF \
> >>>> -DWITH_AVFFT=ON \
> >>>> --Wno-dev $(CMAKE_GENERATOR)
> >>>> +-Wno-dev
> >>>
> >>> And here you don't use the generator at all, for example the Msys one we wanted.
> >> cmake is called via $(CMAKE), so it got the generator setting.
> >>>
> >>>> cd $< && $(MAKE) install
> >>>> touch $@
> >>>> --
> >>>> 2.28.0
> >>>> _______________________________________________
> >>>> vlc-devel mailing list
> >>>> To unsubscribe or modify your subscription options:
> >>>> https://mailman.videolan.org/listinfo/vlc-devel
> >>> _______________________________________________
> >>> vlc-devel mailing list
> >>> To unsubscribe or modify your subscription options:
> >>> https://mailman.videolan.org/listinfo/vlc-devel
> >> _______________________________________________
> >> vlc-devel mailing list
> >> To unsubscribe or modify your subscription options:
> >> https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
>

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel