gitgitgadget / git

GitGitGadget's Git fork. Open Pull Requests here to submit them to the Git mailing list
https://gitgitgadget.github.io/
Other
208 stars 135 forks source link

cmake: fix build of `t-oidtree` #1761

Closed dscho closed 1 month ago

dscho commented 1 month ago

This is based on the gt/unit-test-oidtree branch.

Cc: "Ghanshyam Thakkar" shyamthakkar001@gmail.com

dscho commented 1 month ago

/submit

gitgitgadget[bot] commented 1 month ago

Submitted as pull.1761.git.1720816450344.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1761/dscho/cmake-vs-t-oidtree-v1

To fetch this version to local tag pr-1761/dscho/cmake-vs-t-oidtree-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1761/dscho/cmake-vs-t-oidtree-v1
gitgitgadget[bot] commented 1 month ago

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

Hi,

now Cc:ing Ghanshyam correctly. Sorry for that.

On Fri, 12 Jul 2024, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When the `oidtree` test helper was turned into a unit test, a new
> `lib-oid` source file was added as dependency. This was only done in the
> Makefile so far, but also needs to be done in the CMake definition.
>
> This is a companion of ed548408723d (t/: migrate helper/test-oidtree.c
> to unit-tests/t-oidtree.c, 2024-06-08).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     cmake: fix build of t-oidtree
>
>     This is based on the gt/unit-test-oidtree branch.
>
>     /cc "Ghanshyam Thakkar" shyamthakkar001@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1761%2Fdscho%2Fcmake-vs-t-oidtree-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1761/dscho/cmake-vs-t-oidtree-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1761
>
>  contrib/buildsystems/CMakeLists.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 2f9c33585c6..832f46b316b 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -976,11 +976,12 @@ list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>
>  #unit-tests
>  add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
> +add_library(unit-test-lib-oid OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/lib-oid.c)
>
>  parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
>  foreach(unit_test ${unit_test_PROGRAMS})
>   add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
> - target_link_libraries("${unit_test}" unit-test-lib common-main)
> + target_link_libraries("${unit_test}" unit-test-lib unit-test-lib-oid common-main)
>   set_target_properties("${unit_test}"
>       PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests/bin)
>   if(MSVC)
>
> base-commit: ed548408723d6e969160279398cc47f88f5700bc
> --
> gitgitgadget
>
gitgitgadget[bot] commented 1 month ago

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When the `oidtree` test helper was turned into a unit test, a new
> `lib-oid` source file was added as dependency. This was only done in the
> Makefile so far, but also needs to be done in the CMake definition.
>
> This is a companion of ed548408723d (t/: migrate helper/test-oidtree.c
> to unit-tests/t-oidtree.c, 2024-06-08).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     cmake: fix build of t-oidtree
>     
>     This is based on the gt/unit-test-oidtree branch.
>     
>     /cc "Ghanshyam Thakkar" shyamthakkar001@gmail.com

The base topic has been merged last month during this cycle, so the
fix must be merged down to the 'master' before the release.  Will
queue.

Thanks.
gitgitgadget[bot] commented 1 month ago

This patch series was integrated into seen via https://github.com/git/git/commit/bc7d7b028dce0738bd85fd2d3f3e967038ba33f2.

gitgitgadget[bot] commented 1 month ago

On the Git mailing list, "Ghanshyam Thakkar" wrote (reply to this):

Junio C Hamano <gitster@pobox.com> wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When the `oidtree` test helper was turned into a unit test, a new
> > `lib-oid` source file was added as dependency. This was only done in the
> > Makefile so far, but also needs to be done in the CMake definition.
> >
> > This is a companion of ed548408723d (t/: migrate helper/test-oidtree.c
> > to unit-tests/t-oidtree.c, 2024-06-08).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >     cmake: fix build of t-oidtree
> >     
> >     This is based on the gt/unit-test-oidtree branch.
> >     
> >     /cc "Ghanshyam Thakkar" shyamthakkar001@gmail.com
>
> The base topic has been merged last month during this cycle, so the
> fix must be merged down to the 'master' before the release. Will
> queue.

Changes look correct to me. I was not aware of the existance of
CMakeLists in the tree cause there was no mention of CMake support
in the docs about building git from source, but it was my mistake
for grepping for 'test-lib' in just the Makefile instead of project
wide. Apologies for the negligance and thanks for the patch.
gitgitgadget[bot] commented 1 month ago

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:

> Changes look correct to me. I was not aware of the existance of
> CMakeLists in the tree cause there was no mention of CMake support
> in the docs about building git from source, but it was my mistake
> for grepping for 'test-lib' in just the Makefile instead of project
> wide. Apologies for the negligance and thanks for the patch.

No need to apologize.  Nobody caught during the review, so it is not
even your fault ;-) 

And the CMakeLists is not even meant to work with anything but for
Windows, IIRC (even though it might work by accident).
gitgitgadget[bot] commented 1 month ago

This branch is now known as js/unit-test-oidtree-cmake-fix.

gitgitgadget[bot] commented 1 month ago

There was a status update in the "New Topics" section about the branch js/unit-test-oidtree-cmake-fix on the Git mailing list:

Build fix.

Will merge to 'next'.
source: <pull.1761.git.1720816450344.gitgitgadget@gmail.com>
gitgitgadget[bot] commented 1 month ago

This patch series was integrated into seen via https://github.com/git/git/commit/edeb94556a5ba4e6c03c994809dec4c32b742335.

gitgitgadget[bot] commented 1 month ago

This patch series was integrated into next via https://github.com/git/git/commit/d7aac9004ca75f52297a87454d02cc988b9ee8db.

gitgitgadget[bot] commented 1 month ago

This patch series was integrated into seen via https://github.com/git/git/commit/17df5da515a7169be3800a34d1132be2fe132004.

gitgitgadget[bot] commented 1 month ago

There was a status update in the "Cooking" section about the branch js/unit-test-oidtree-cmake-fix on the Git mailing list:

Build fix.

Will merge to 'master'.
source: <pull.1761.git.1720816450344.gitgitgadget@gmail.com>
gitgitgadget[bot] commented 1 month ago

This patch series was integrated into seen via https://github.com/git/git/commit/219719cc558334e06c129598072c9d12ca79028d.

gitgitgadget[bot] commented 1 month ago

This patch series was integrated into master via https://github.com/git/git/commit/219719cc558334e06c129598072c9d12ca79028d.

gitgitgadget[bot] commented 1 month ago

This patch series was integrated into next via https://github.com/git/git/commit/219719cc558334e06c129598072c9d12ca79028d.

gitgitgadget[bot] commented 1 month ago

Closed via 219719cc558334e06c129598072c9d12ca79028d.

gitgitgadget[bot] commented 1 month ago

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

Hi Ghanshyam,

On Sat, 13 Jul 2024, Ghanshyam Thakkar wrote:

> Junio C Hamano <gitster@pobox.com> wrote:
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > When the `oidtree` test helper was turned into a unit test, a new
> > > `lib-oid` source file was added as dependency. This was only done in the
> > > Makefile so far, but also needs to be done in the CMake definition.
> > >
> > > This is a companion of ed548408723d (t/: migrate helper/test-oidtree.c
> > > to unit-tests/t-oidtree.c, 2024-06-08).
> > >
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >     cmake: fix build of t-oidtree
> > >
> > >     This is based on the gt/unit-test-oidtree branch.
> > >
> > >     /cc "Ghanshyam Thakkar" shyamthakkar001@gmail.com
> >
> > The base topic has been merged last month during this cycle, so the
> > fix must be merged down to the 'master' before the release. Will
> > queue.
>
> Changes look correct to me.

Thank you for confirming.

For what it's worth, it was this confirmation by the domain expert (i.e.
you) that I was looking for when I Cc:ed you, not to spread blame.

The CMakeLists.txt file exists primarily to support building Git in Visual
Studio, and I removed that part from CI builds specifically so that
contributors on the Git mailing list need not worry about that support.

Ciao,
Johannes