diff mbox series

[1/3] cmake: add knob to disable vcpkg

Message ID 3170f78daa5fa89f04f61e24c9c64c93ea5b394f.1622828605.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Make CMake work out of the box | expand

Commit Message

Matt Rogers June 4, 2021, 5:43 p.m. UTC 
From: Matthew Rogers <mattr94@gmail.com>

When building on windows users have the option to use vcpkg to provide
the dependencies needed to compile.  Previously, this was used only when
using the Visual Studio generator which was not ideal because:

  - Not all users who want to use vcpkg use the Visual Studio
    generators.

  - Some versions of Visual Studio 2019 moved away from using the
    VS 2019 by default, making it impossible for Visual Studio to
    configure the project in the likely event that it couldn't find the
    dependencies.

  - Inexperienced users of CMake are very likely to get tripped up by
    the errors caused by a lack of vcpkg, making the above bullet point
    both annoying and hard to debug.

As such, lets make using vcpkg the default on windows.  Users who want
to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

Signed-off-by: Matthew Rogers <mattr94@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Eric Sunshine June 4, 2021, 6:03 p.m. UTC | #1 
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When building on windows users have the option to use vcpkg to provide
> the dependencies needed to compile.  Previously, this was used only when
> using the Visual Studio generator which was not ideal because:
>
>   - Not all users who want to use vcpkg use the Visual Studio
>     generators.
>
>   - Some versions of Visual Studio 2019 moved away from using the
>     VS 2019 by default, making it impossible for Visual Studio to
>     configure the project in the likely event that it couldn't find the
>     dependencies.

Is there something missing between "using the" and "VS 2019"? I'm
having a hard time trying to understand what this bullet point is
saying due to this apparent gap.

>   - Inexperienced users of CMake are very likely to get tripped up by
>     the errors caused by a lack of vcpkg, making the above bullet point
>     both annoying and hard to debug.
>
> As such, lets make using vcpkg the default on windows.  Users who want
> to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

s/lets/let's/

> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
Matt Rogers June 4, 2021, 6:34 p.m. UTC | #2 
On Fri, Jun 4, 2021 at 2:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > When building on windows users have the option to use vcpkg to provide
> > the dependencies needed to compile.  Previously, this was used only when
> > using the Visual Studio generator which was not ideal because:
> >
> >   - Not all users who want to use vcpkg use the Visual Studio
> >     generators.
> >
> >   - Some versions of Visual Studio 2019 moved away from using the
> >     VS 2019 by default, making it impossible for Visual Studio to
> >     configure the project in the likely event that it couldn't find the
> >     dependencies.
>
> Is there something missing between "using the" and "VS 2019"? I'm
> having a hard time trying to understand what this bullet point is
> saying due to this apparent gap.
>

Yeah, this should really read
- Some versions of Visual Studio 2019 moved away from using the
     VS 2019 _Generator_ by default, making it impossible for Visual Studio to
     configure the project in the likely event that it couldn't find the
     dependencies.


> >   - Inexperienced users of CMake are very likely to get tripped up by
> >     the errors caused by a lack of vcpkg, making the above bullet point
> >     both annoying and hard to debug.
> >
> > As such, lets make using vcpkg the default on windows.  Users who want
> > to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.
>
> s/lets/let's/
>
> > Signed-off-by: Matthew Rogers <mattr94@gmail.com>
Sibi Siddharthan June 4, 2021, 8:55 p.m. UTC | #3 
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
<gitgitgadget@gmail.com> wrote:

> -if(WIN32)
> +
> +if (WIN32 AND NOT NO_VCPKG)
> +       set(USING_VCPKG TRUE)
> +else()
> +       set(USING_VCPKG FALSE)
> +endif()

I think it would be better if we could have an option for this knob.
Maybe like this

option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only
applicable to Windows platforms" OFF)

I would prefer to use `USE_VCPKG`.

Thank You,
Sibi Siddharthan
Matt Rogers June 5, 2021, 10:30 p.m. UTC | #4 
On Fri, Jun 4, 2021 at 4:55 PM Sibi Siddharthan
<sibisiddharthan.github@gmail.com> wrote:
>
> On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> > -if(WIN32)
> > +
> > +if (WIN32 AND NOT NO_VCPKG)
> > +       set(USING_VCPKG TRUE)
> > +else()
> > +       set(USING_VCPKG FALSE)
> > +endif()
>
> I think it would be better if we could have an option for this knob.
> Maybe like this
>
> option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only
> applicable to Windows platforms" OFF)

Option would definitely be the better tool to use here, I just didn't
think about
it when originally writing it, so I'll send a reroll with that and the spelling
corrections suggested by Eric Sunshine.  I assume you'd prefer something
with a final form more like:

option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.
Only applicable to Windows platforms" ON)


>
> I would prefer to use `USE_VCPKG`.
>
> Thank You,
> Sibi Siddharthan
Sibi Siddharthan June 6, 2021, 4:33 a.m. UTC | #5 
On Sun, Jun 6, 2021 at 4:01 AM Matt Rogers <mattr94@gmail.com> wrote:
>
> On Fri, Jun 4, 2021 at 4:55 PM Sibi Siddharthan
> <sibisiddharthan.github@gmail.com> wrote:
> >
> > On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >
> > > -if(WIN32)
> > > +
> > > +if (WIN32 AND NOT NO_VCPKG)
> > > +       set(USING_VCPKG TRUE)
> > > +else()
> > > +       set(USING_VCPKG FALSE)
> > > +endif()
> >
> > I think it would be better if we could have an option for this knob.
> > Maybe like this
> >
> > option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only
> > applicable to Windows platforms" OFF)
>
> Option would definitely be the better tool to use here, I just didn't
> think about
> it when originally writing it, so I'll send a reroll with that and the spelling
> corrections suggested by Eric Sunshine.  I assume you'd prefer something
> with a final form more like:
>
> option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.
> Only applicable to Windows platforms" ON)
>

Yes, this would be better.

Thank You,
Sibi Siddharthan
diff mbox series

Patch

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index a87841340e6a..41320150bf66 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -43,14 +43,24 @@  NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi
 to use another tool say `ninja` add this to the command line when configuring.
 `-G Ninja`
 
+NOTE: By default CMake will install vcpkg locally to your source tree on configuration,
+to avoid this, add `-DNO_VCPKG=TRUE` to the command line when configuring.
+
 ]]
 cmake_minimum_required(VERSION 3.14)
 
 #set the source directory to root of git
 set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
-if(WIN32)
+
+if (WIN32 AND NOT NO_VCPKG)
+	set(USING_VCPKG TRUE)
+else()
+	set(USING_VCPKG FALSE)
+endif()
+
+if(USING_VCPKG)
 	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
-	if(MSVC AND NOT EXISTS ${VCPKG_DIR})
+	if(NOT EXISTS ${VCPKG_DIR})
 		message("Initializing vcpkg and building the Git's dependencies (this will take a while...)")
 		execute_process(COMMAND ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg_install.bat)
 	endif()
@@ -178,7 +188,9 @@  endif()
 
 find_program(MSGFMT_EXE msgfmt)
 if(NOT MSGFMT_EXE)
-	set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+	if (USING_VCPKG)
+		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+	endif()
 	if(NOT EXISTS ${MSGFMT_EXE})
 		message(WARNING "Text Translations won't be built")
 		unset(MSGFMT_EXE)
@@ -982,7 +994,7 @@  file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n"
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
-if(WIN32)
+if(USING_VCPKG)
 	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
 endif()