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

[Steve Lhomme]

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
>