Skip to content

cmake: learn to optionally skip linking dashed built-ins #887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

cmake: learn to optionally skip linking dashed built-ins #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff7e812
cmake: support SKIP_DASHED_BUILT_INS
dscho Feb 24, 2021
69856f2
cmake(install): fix double .exe suffixes
dennisameling Jan 5, 2021
5d953a2
cmake: add a preparatory work-around to accommodate `vcpkg`
dscho Feb 24, 2021
f020cb5
cmake(install): include vcpkg dlls
dennisameling Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,11 @@ jobs:
## Unzip and remove the artifact
unzip artifacts.zip
rm artifacts.zip
- name: initialize vcpkg
uses: actions/checkout@v2
with:
repository: 'microsoft/vcpkg'
path: 'compat/vcbuild/vcpkg'
- name: download vcpkg artifacts
shell: powershell
run: |
Expand Down
26 changes: 19 additions & 7 deletions contrib/buildsystems/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ if(WIN32)

# In the vcpkg edition, we need this to be able to link to libcurl
set(CURL_NO_CURL_CMAKE ON)

# Copy the necessary vcpkg DLLs (like iconv) to the install dir
set(X_VCPKG_APPLOCAL_DEPS_INSTALL ON)
set(CMAKE_TOOLCHAIN_FILE ${VCPKG_DIR}/scripts/buildsystems/vcpkg.cmake CACHE STRING "Vcpkg toolchain file")
endif()

find_program(SH_EXE sh PATHS "C:/Program Files/Git/bin")
Expand Down Expand Up @@ -685,13 +689,17 @@ endif()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Đoàn Trần Công Danh wrote (reply to this):

On 2021-03-27 23:06:24+0000, Johannes Schindelin via GitGitGadget <[email protected]> wrote:
> From: Johannes Schindelin <[email protected]>
> 
> We are about to add support for installing the `.dll` files of Git's
> dependencies (such as libcurl) in the CMake configuration. The `vcpkg`
> ecosystem from which we get said dependencies makes that relatively
> easy: simply turn on `X_VCPKG_APPLOCAL_DEPS_INSTALL`.
> 
> However, current `vcpkg` introduces a limitation if one does that:
> While it is totally cool with CMake to specify multiple targets within
> one invocation of `install(TARGETS ...) (at least according to
> https://cmake.org/cmake/help/latest/command/install.html#command:install),
> `vcpkg`'s parser insists on a single target per `install(TARGETS ...)`
> invocation.
> 
> Well, that's easily accomplished: Let's feed the targets individually to
> the `install(TARGETS ...)` function in a `foreach()` look.
> 
> This also has the advantage that we do not have to manually cull off the
> two entries from the `${PROGRAMS_BUILT}` array before scheduling the
> remainder to be installed into `libexec/git-core`. Instead, we iterate
> through the array and decide for each entry where it wants to go.
> 
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  contrib/buildsystems/CMakeLists.txt | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index da2811ae3aad..a166be0eb1b8 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -811,15 +811,19 @@ list(TRANSFORM git_shell_scripts PREPEND "${CMAKE_BINARY_DIR}/")
>  list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")
>  
>  #install
> -install(TARGETS git git-shell
> +foreach(program ${PROGRAMS_BUILT})
> +if(${program} STREQUAL git OR ${program} STREQUAL git-shell)

Please don't use `${}` around variable inside `if()`, and quote the
string. CMake has a quirk with the `${}` inside if (expanded variable
will be treated as a variable if it is defined, or string otherwise).
Unquoted string will be seen as a variable if it's defined, string
otherwise. IOW, suggested command:

	if (program STREQUAL "git" OR program STREQUAL "git-shell")

We also have another problem with quoted arguments could be interpreted
as variable or keyword if CMP0054 policy not enabled, too.
I think it's better to have it enabled, but it's not in the scope of
this patch.

 https://cmake.org/cmake/help/latest/policy/CMP0054.html


-- 
Danh

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1876820553-1617024963=:53
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Danh,

On Sun, 28 Mar 2021, =C4=90o=C3=A0n Tr=E1=BA=A7n C=C3=B4ng Danh wrote:

> On 2021-03-27 23:06:24+0000, Johannes Schindelin via GitGitGadget <gitgi=
[email protected]> wrote:
> > From: Johannes Schindelin <[email protected]>
> >
> > We are about to add support for installing the `.dll` files of Git's
> > dependencies (such as libcurl) in the CMake configuration. The `vcpkg`
> > ecosystem from which we get said dependencies makes that relatively
> > easy: simply turn on `X_VCPKG_APPLOCAL_DEPS_INSTALL`.
> >
> > However, current `vcpkg` introduces a limitation if one does that:
> > While it is totally cool with CMake to specify multiple targets within
> > one invocation of `install(TARGETS ...) (at least according to
> > https://cmake.org/cmake/help/latest/command/install.html#command:insta=
ll),
> > `vcpkg`'s parser insists on a single target per `install(TARGETS ...)`
> > invocation.
> >
> > Well, that's easily accomplished: Let's feed the targets individually =
to
> > the `install(TARGETS ...)` function in a `foreach()` look.
> >
> > This also has the advantage that we do not have to manually cull off t=
he
> > two entries from the `${PROGRAMS_BUILT}` array before scheduling the
> > remainder to be installed into `libexec/git-core`. Instead, we iterate
> > through the array and decide for each entry where it wants to go.
> >
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> >  contrib/buildsystems/CMakeLists.txt | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystem=
s/CMakeLists.txt
> > index da2811ae3aad..a166be0eb1b8 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -811,15 +811,19 @@ list(TRANSFORM git_shell_scripts PREPEND "${CMAK=
E_BINARY_DIR}/")
> >  list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")
> >
> >  #install
> > -install(TARGETS git git-shell
> > +foreach(program ${PROGRAMS_BUILT})
> > +if(${program} STREQUAL git OR ${program} STREQUAL git-shell)
>
> Please don't use `${}` around variable inside `if()`, and quote the
> string. CMake has a quirk with the `${}` inside if (expanded variable
> will be treated as a variable if it is defined, or string otherwise).
> Unquoted string will be seen as a variable if it's defined, string
> otherwise. IOW, suggested command:
>
> 	if (program STREQUAL "git" OR program STREQUAL "git-shell")
>
> We also have another problem with quoted arguments could be interpreted
> as variable or keyword if CMP0054 policy not enabled, too.
> I think it's better to have it enabled, but it's not in the scope of
> this patch.
>
>  https://cmake.org/cmake/help/latest/policy/CMP0054.html

Thank you for this information! I've sent out v2 based on your suggestion.

Thanks,
Dscho

--8323328-1876820553-1617024963=:53--

parse_makefile_for_executables(git_builtin_extra "BUILT_INS")

option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")

#Creating hardlinks
if(NOT SKIP_DASHED_BUILT_INS)
foreach(s ${git_SOURCES} ${git_builtin_extra})
string(REPLACE "${CMAKE_SOURCE_DIR}/builtin/" "" s ${s})
string(REPLACE ".c" "" s ${s})
file(APPEND ${CMAKE_BINARY_DIR}/CreateLinks.cmake "file(CREATE_LINK git${EXE_EXTENSION} git-${s}${EXE_EXTENSION})\n")
list(APPEND git_links ${CMAKE_BINARY_DIR}/git-${s}${EXE_EXTENSION})
endforeach()
endif()

if(CURL_FOUND)
set(remote_exes
Expand Down Expand Up @@ -807,15 +815,19 @@ list(TRANSFORM git_shell_scripts PREPEND "${CMAKE_BINARY_DIR}/")
list(TRANSFORM git_perl_scripts PREPEND "${CMAKE_BINARY_DIR}/")

#install
install(TARGETS git git-shell
foreach(program ${PROGRAMS_BUILT})
if(program STREQUAL "git" OR program STREQUAL "git-shell")
install(TARGETS ${program}
RUNTIME DESTINATION bin)
else()
install(TARGETS ${program}
RUNTIME DESTINATION libexec/git-core)
endif()
endforeach()

install(PROGRAMS ${CMAKE_BINARY_DIR}/git-cvsserver
DESTINATION bin)

list(REMOVE_ITEM PROGRAMS_BUILT git git-shell)
install(TARGETS ${PROGRAMS_BUILT}
RUNTIME DESTINATION libexec/git-core)

set(bin_links
git-receive-pack git-upload-archive git-upload-pack)

Expand All @@ -828,12 +840,12 @@ install(CODE "file(CREATE_LINK ${CMAKE_INSTALL_PREFIX}/bin/git-shell${EXE_EXTENS

foreach(b ${git_links})
string(REPLACE "${CMAKE_BINARY_DIR}" "" b ${b})
install(CODE "file(CREATE_LINK ${CMAKE_INSTALL_PREFIX}/bin/git${EXE_EXTENSION} ${CMAKE_INSTALL_PREFIX}/libexec/git-core/${b}${EXE_EXTENSION})")
install(CODE "file(CREATE_LINK ${CMAKE_INSTALL_PREFIX}/bin/git${EXE_EXTENSION} ${CMAKE_INSTALL_PREFIX}/libexec/git-core/${b})")
endforeach()

foreach(b ${git_http_links})
string(REPLACE "${CMAKE_BINARY_DIR}" "" b ${b})
install(CODE "file(CREATE_LINK ${CMAKE_INSTALL_PREFIX}/libexec/git-core/git-remote-http${EXE_EXTENSION} ${CMAKE_INSTALL_PREFIX}/libexec/git-core/${b}${EXE_EXTENSION})")
install(CODE "file(CREATE_LINK ${CMAKE_INSTALL_PREFIX}/libexec/git-core/git-remote-http${EXE_EXTENSION} ${CMAKE_INSTALL_PREFIX}/libexec/git-core/${b})")
endforeach()

install(PROGRAMS ${git_shell_scripts} ${git_perl_scripts} ${CMAKE_BINARY_DIR}/git-p4
Expand Down