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

[Zhao Zhili]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-contrib-support-CMAKE_GENERATOR-other-than-makefile.patch
Type: application/octet-stream
Size: 13561 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20201103/60a3f6ff/attachment-0001.obj>
-------------- next part --------------


> 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