cmake: learn to optionally skip linking dashed built-ins #887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just like the Makefile-based build learned to skip hard-linking the dashed built-ins in 179227d (Optionally skip linking/copying the built-ins, 2020-09-21), this patch teaches the CMake-based build the same trick. Note: In contrast to the Makefile-based process, the built-ins would only be linked during installation, not already when Git is built. Therefore, the CMake-based build that we use in our CI builds _already_ does not link those built-ins (because the files are not installed anywhere, they are used to run the test suite in-place). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
By mistake, the `.exe` extension is appended _twice_ when installing the dashed executables into `libexec/git-core/` on Windows (the extension is already appended when adding items to the `git_links` list in the `#Creating hardlinks` section). Signed-off-by: Dennis Ameling <dennis@dennisameling.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
skip-dashed-built-ins-in-cmake
branch
from
eb02398 to
4b183c7
Compare
|
/submit |
|
Submitted as pull.887.git.1616886386.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -685,13 +685,17 @@ endif() | |||
|
|
|||
There was a problem hiding this comment.
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 <gitgitgadget@gmail.com> wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
> 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
There was a problem hiding this comment.
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=
tgadget@gmail.com> wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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 <johannes.schindelin@gmx.de>
> > ---
> > 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--
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@6a27d68. |
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 <johannes.schindelin@gmx.de>
Our CMake configuration generates not only build definitions, but also install definitions: After building Git using `msbuild git.sln`, the built artifacts can be installed via `msbuild INSTALL.vcxproj`. To specify _where_ the files should be installed, the `-DCMAKE_INSTALL_PREFIX=<path>` option can be used when running CMake. However, this process would really only install the files that were just built. On Windows, we need more than that: We also need the `.dll` files of the dependencies (such as libcurl). The `vcpkg` ecosystem, which we use to obtain those dependencies, can be asked to install said `.dll` files really easily, so let's do that. This requires more than just the built `vcpkg` artifacts in the CI build definition; We now clone the `vcpkg` repository so that the relevant CMake scripts are available, in particular the ones related to defining the toolchain. Signed-off-by: Dennis Ameling <dennis@dennisameling.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
skip-dashed-built-ins-in-cmake
branch
from
4b183c7 to
f020cb5
Compare
|
/submit |
|
Submitted as pull.887.v2.git.1617021705.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@c43488f. |
|
This patch series was integrated into seen via git@408f151. |
|
There was a status update about the branch CMake update for vsbuild. Will merge to 'next'. |
|
This patch series was integrated into seen via git@6c91a26. |
|
This patch series was integrated into next via git@e0c4369. |
|
This patch series was integrated into seen via git@3a4101b. |
|
This patch series was integrated into seen via git@d65092b. |
|
There was a status update about the branch CMake update for vsbuild. Will merge to 'master'. |
|
This patch series was integrated into seen via git@a548f3e. |
|
This patch series was integrated into next via git@a548f3e. |
|
This patch series was integrated into master via git@a548f3e. |
|
Closed via a548f3e. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In Git for Windows, we would like to make use of the fact that our CMake-based build can also install the files into their final location. This patch series helps with that.
Changes since v1:
cc: Đoàn Trần Công Danh congdanhqx@gmail.com