[02/10] cmake: do find Git for Windows' shell interpreter

Message ID	05b4b69fee2b8c32769dd72dea182cfb72a14876.1601044118.git.gitgitgadget@gmail.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <SRS0=RFRG=DC=vger.kernel.org=git-owner@kernel.org> Message-Id: <05b4b69fee2b8c32769dd72dea182cfb72a14876.1601044118.git.gitgitgadget@gmail.com> In-Reply-To: <pull.738.git.1601044118.gitgitgadget@gmail.com> References: <pull.738.git.1601044118.gitgitgadget@gmail.com> From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> Date: Fri, 25 Sep 2020 14:28:30 +0000 Subject: [PATCH 02/10] cmake: do find Git for Windows' shell interpreter Fcc: Sent Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MIME-Version: 1.0 To: git@vger.kernel.org Cc: Sibi Siddharthan <sibisiddharthan.github@gmail.com>, =?utf-8?b?xJBvw6Bu?= =?utf-8?b?IFRy4bqnbiBDw7RuZw==?= Danh <congdanhqx@gmail.com>, SZEDER =?utf-8?b?R8OhYm9y?= <szeder.dev@gmail.com>, Johannes Schindelin <johannes.schindelin@gmx.de>, Johannes Schindelin <johannes.schindelin@gmx.de> Precedence: bulk
Series	CMake and Visual Studio \| expand [00/10] CMake and Visual Studio [01/10] cmake: ignore files generated by CMake as run in Visual Studio [02/10] cmake: do find Git for Windows' shell interpreter [03/10] cmake: ensure that the `vcpkg` packages are found on Windows [04/10] cmake: fall back to using `vcpkg`'s `msgfmt.exe` on Windows [05/10] cmake: quote the path accurately when editing `test-lib.sh` [06/10] cmake (Windows): let the `.dll` files are found when running the tests [07/10] cmake (Windows): complain when encountering an unknown compiler [08/10] cmake (Windows): initialize vcpkg/build dependencies automatically [09/10] cmake (Windows): recommend using Visual Studio's built-in CMake support [10/10] hashmap_for_each_entry(): work around MSVC's run-time check failure #3

Linus Arver via GitGitGadget Sept. 25, 2020, 2:28 p.m. UTC

From: Johannes Schindelin <johannes.schindelin@gmx.de>

By default, Git for Windows does not install its `sh.exe` into the
`PATH`. However, our current `CMakeLists.txt` expects to find a shell
interpreter in the `PATH`.

So let's fall back to looking in the default location where Git for
Windows _does_ install a relatively convenient `sh.exe`:
`C:\Program Files\Git\bin\sh.exe`

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Sibi Siddharthan Sept. 25, 2020, 4:25 p.m. UTC | #1

> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 5007f173f1..d14fa4f3dc 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -43,8 +43,11 @@ set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
>
>  find_program(SH_EXE sh)
>  if(NOT SH_EXE)
> -       message(FATAL_ERROR "sh: shell interpreter was not found in your path, please install one."
> -                       "On Windows, you can get it as part of 'Git for Windows' install at https://gitforwindows.org/")
> +       set(SH_EXE "C:/Program Files/Git/bin/sh.exe")
> +       if(NOT EXISTS ${SH_EXE})
> +               message(FATAL_ERROR "sh: shell interpreter was not found in your path, please install one."
> +                               "On Windows, you can get it as part of 'Git for Windows' install at https://gitforwindows.org/")
> +       endif()
>  endif()
>
Let's not point SH_EXE to the 'program files' directory.
find_program() searches 'PATH' for sh.
Since the Git-for-windows installer does append
'your_installation_directory'/Git/bin to the PATH variable, it should
be fine.

I personally don't install my dev tools(except Visual Studio) to
Program Files(because of the _space_), it messes up the Makefiles.

Thank You,
Sibi Siddharthan

Johannes Schindelin Sept. 26, 2020, 8:32 p.m. UTC | #2

Hi Sibi,

On Fri, 25 Sep 2020, Sibi Siddharthan wrote:

> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index 5007f173f1..d14fa4f3dc 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -43,8 +43,11 @@ set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
> >
> >  find_program(SH_EXE sh)
> >  if(NOT SH_EXE)
> > -       message(FATAL_ERROR "sh: shell interpreter was not found in your path, please install one."
> > -                       "On Windows, you can get it as part of 'Git for Windows' install at https://gitforwindows.org/")
> > +       set(SH_EXE "C:/Program Files/Git/bin/sh.exe")
> > +       if(NOT EXISTS ${SH_EXE})
> > +               message(FATAL_ERROR "sh: shell interpreter was not found in your path, please install one."
> > +                               "On Windows, you can get it as part of 'Git for Windows' install at https://gitforwindows.org/")
> > +       endif()
> >  endif()
> >
> Let's not point SH_EXE to the 'program files' directory.

It is not doing that, except as a fall-back in case the `sh` program
was not found in the `PATH`.

> find_program() searches 'PATH' for sh.

Right.

> Since the Git-for-windows installer does append
> 'your_installation_directory'/Git/bin to the PATH variable, it should
> be fine.

No, it does not. Quite purposefully so.

The `/bin/` directory is kind of a hack to reinstate _some_ level of
support for use cases that relied on Git for Windows v1.x installing its
binaries into that directory (v2.x distributes them between `/usr/bin/`
and `/mingw64/bin/`).

What _does_ get appended, at least by default, is the `/cmd/` directory
(which does _not_ contain `sh.exe`).

Now, there _is_ an option in the Git for Windows installer to append all
of its Unix tools to the `PATH`, but it is highly discouraged to do so.

> I personally don't install my dev tools(except Visual Studio) to
> Program Files(because of the _space_), it messes up the Makefiles.

Sure, and that's your prerogative. There's unfortunately no good way to
support your use case.

Luckily, the vast majority of Git for Windows' users do not change the
default location, and this patch is for them. (And "them" in this case
includes me, personally ;-))

Ciao,
Dscho

Đoàn Trần Công Danh Sept. 27, 2020, 2:25 a.m. UTC | #3

On 2020-09-26 22:32:25+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Since the Git-for-windows installer does append
> > 'your_installation_directory'/Git/bin to the PATH variable, it should
> > be fine.
> 
> No, it does not. Quite purposefully so.
> 
> The `/bin/` directory is kind of a hack to reinstate _some_ level of
> support for use cases that relied on Git for Windows v1.x installing its
> binaries into that directory (v2.x distributes them between `/usr/bin/`
> and `/mingw64/bin/`).
> 
> What _does_ get appended, at least by default, is the `/cmd/` directory
> (which does _not_ contain `sh.exe`).
> 
> Now, there _is_ an option in the Git for Windows installer to append all
> of its Unix tools to the `PATH`, but it is highly discouraged to do so.

I agree with this decision.

> 
> > I personally don't install my dev tools(except Visual Studio) to
> > Program Files(because of the _space_), it messes up the Makefiles.
> 
> Sure, and that's your prerogative. There's unfortunately no good way to
> support your use case.
> 
> Luckily, the vast majority of Git for Windows' users do not change the
> default location, and this patch is for them. (And "them" in this case
> includes me, personally ;-))

This doesn't fit into my view of Git for Windows' users
For some users that have the Administrator right, it's the default
location if they grant the Administrator right for the installer.

For those poor souls that works for enterprise companies, and thoses
that not feel comfortable give Administrator right to _another_
installer, the installer will install into (hopeful, I type it right):

	%USERPROFILE%/AppData/Local/Programs/Git

I think it's better to offer SH_EXE as an OPTION, let user specify it
as will. And we'll search in PATH if it's not specified, fallback to
2 default value if not found.

Johannes Schindelin Sept. 28, 2020, 1:56 p.m. UTC | #4

Hi Danh,

On Sun, 27 Sep 2020, Đoàn Trần Công Danh wrote:

> On 2020-09-26 22:32:25+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > > I personally don't install my dev tools(except Visual Studio) to
> > > Program Files(because of the _space_), it messes up the Makefiles.
> >
> > Sure, and that's your prerogative. There's unfortunately no good way to
> > support your use case.
> >
> > Luckily, the vast majority of Git for Windows' users do not change the
> > default location, and this patch is for them. (And "them" in this case
> > includes me, personally ;-))
>
> This doesn't fit into my view of Git for Windows' users
> For some users that have the Administrator right, it's the default
> location if they grant the Administrator right for the installer.
>
> For those poor souls that works for enterprise companies, and thoses
> that not feel comfortable give Administrator right to _another_
> installer, the installer will install into (hopeful, I type it right):
>
> 	%USERPROFILE%/AppData/Local/Programs/Git

Those poor souls that work for enterprise companies often have Git for
Windows installed by default. And of course, that default would be in
`C:\Program Files\Git`.

> I think it's better to offer SH_EXE as an OPTION, let user specify it
> as will. And we'll search in PATH if it's not specified, fallback to
> 2 default value if not found.

That's exactly as it is right now. You can specify `SH_EXE` (but only if
running CMake manually, not via Visual Studio). If you don't, it searches
`PATH`, and with my patch it then falls back to trying to find `sh.exe` in
Git for Windows' default location.

So I think we're in agreement here?

Ciao,
Dscho

Đoàn Trần Công Danh Sept. 29, 2020, 2:04 p.m. UTC | #5

On 2020-09-28 15:56:13+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi Danh,
> 
> On Sun, 27 Sep 2020, Đoàn Trần Công Danh wrote:
> 
> > On 2020-09-26 22:32:25+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > > > I personally don't install my dev tools(except Visual Studio) to
> > > > Program Files(because of the _space_), it messes up the Makefiles.
> > >
> > > Sure, and that's your prerogative. There's unfortunately no good way to
> > > support your use case.
> > >
> > > Luckily, the vast majority of Git for Windows' users do not change the
> > > default location, and this patch is for them. (And "them" in this case
> > > includes me, personally ;-))
> >
> > This doesn't fit into my view of Git for Windows' users
> > For some users that have the Administrator right, it's the default
> > location if they grant the Administrator right for the installer.
> >
> > For those poor souls that works for enterprise companies, and thoses
> > that not feel comfortable give Administrator right to _another_
> > installer, the installer will install into (hopeful, I type it right):
> >
> > 	%USERPROFILE%/AppData/Local/Programs/Git
> 
> Those poor souls that work for enterprise companies often have Git for
> Windows installed by default. And of course, that default would be in
> `C:\Program Files\Git`.

Yes, that's correct, but that Git is usually very old, and I'm not
sure about its layout. Obviously, you know better in this regard :-p

> 
> > I think it's better to offer SH_EXE as an OPTION, let user specify it
> > as will. And we'll search in PATH if it's not specified, fallback to
> > 2 default value if not found.
> 
> That's exactly as it is right now. You can specify `SH_EXE` (but only if
> running CMake manually, not via Visual Studio). If you don't, it searches
> `PATH`, and with my patch it then falls back to trying to find `sh.exe` in
> Git for Windows' default location.
> 
> So I think we're in agreement here?

Yes, seems good.

Johannes Schindelin Sept. 29, 2020, 6:42 p.m. UTC | #6

Hi Danh,

On Tue, 29 Sep 2020, Đoàn Trần Công Danh wrote:

> On 2020-09-28 15:56:13+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > On Sun, 27 Sep 2020, Đoàn Trần Công Danh wrote:
> >
> > > On 2020-09-26 22:32:25+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > > > I personally don't install my dev tools(except Visual Studio) to
> > > > > Program Files(because of the _space_), it messes up the Makefiles.
> > > >
> > > > Sure, and that's your prerogative. There's unfortunately no good way to
> > > > support your use case.
> > > >
> > > > Luckily, the vast majority of Git for Windows' users do not change the
> > > > default location, and this patch is for them. (And "them" in this case
> > > > includes me, personally ;-))
> > >
> > > This doesn't fit into my view of Git for Windows' users
> > > For some users that have the Administrator right, it's the default
> > > location if they grant the Administrator right for the installer.
> > >
> > > For those poor souls that works for enterprise companies, and thoses
> > > that not feel comfortable give Administrator right to _another_
> > > installer, the installer will install into (hopeful, I type it right):
> > >
> > > 	%USERPROFILE%/AppData/Local/Programs/Git
> >
> > Those poor souls that work for enterprise companies often have Git for
> > Windows installed by default. And of course, that default would be in
> > `C:\Program Files\Git`.
>
> Yes, that's correct, but that Git is usually very old, and I'm not
> sure about its layout. Obviously, you know better in this regard :-p

In Git for Windows v1.x, it would have contained the real Bash (at least
in 32-bit Windows; in 64-bit Windows, it would have been `C:\Program Files
(x86)\Git\bin`). But v1.x is long deprecated, it's over 5 years old.

The `C:\Program Files\Git\bin\sh.exe` stand-in is actually not _all_ that
old, I only reintroduced it relatively recently. Nevertheless, it is the
best bet for a default fall-back that I can think of.

> > > I think it's better to offer SH_EXE as an OPTION, let user specify it
> > > as will. And we'll search in PATH if it's not specified, fallback to
> > > 2 default value if not found.
> >
> > That's exactly as it is right now. You can specify `SH_EXE` (but only if
> > running CMake manually, not via Visual Studio). If you don't, it searches
> > `PATH`, and with my patch it then falls back to trying to find `sh.exe` in
> > Git for Windows' default location.
> >
> > So I think we're in agreement here?
>
> Yes, seems good.

Excellent!
Dscho

[02/10] cmake: do find Git for Windows' shell interpreter

Commit Message

Comments

Patch