cmake: generalize the handling of the UNIT_TEST_OBJS list
#1797
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
In a15d446 (cmake: also build unit tests, 2023-09-25), I accommodated the CMake definition. Seeing that a `UNIT_TEST_OBJS` list was introduced that was built by transforming the `UNIT_TEST_PROGRAMS` list and then adding a single, hard-coded file ("t/unit-tests/test-lib.c"), I decided to hard-code that in the CMake definition, too. The reason why I hard-coded it instead of imitating the `parse_makefile_for_sources()` paradigm that was used elsewhere when using the `Makefile` as source of truth for given lists of files: This function expects _only_ hard-coded values, and that transformed `UNIT_TEST_PROGRAMS` list complicated everything. In 8727215 (cmake: fix build of `t-oidtree`, 2024-07-12), I accommodated the CMake definition again, after seeing that the `UNIT_TEST_OBJS` was still defined via that transformed list but now appending _two_ hard-coded files ("t/unit-tests/lib-oid.c" joined the fray). In 428672a (Makefile: stop listing test library objects twice, 2024-09-16), the `Makefile` was changed so that `UNIT_TEST_OBJS` is finally only constructed using hard-coded file names just like the other `*_OBJS` variables. I missed that and therefore did not adjust the CMake definition. Besides, the code was working, so there was no real need to adjust it. With a4f50bb (t/unit-tests: introduce reftable library, 2024-09-16), however, the `UNIT_TEST_OBJS` list became a trio, and the CMake definition has to be adjusted again. Now that we can use the `parse_makefile_for_sources()` function without many complications, let's do that. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
/submit |
|
Submitted as pull.1797.git.1726687769585.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@35b7d19. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@2c4e8a8. |
|
This patch series was integrated into next via git@d892dcd. |
|
This patch series was integrated into seen via git@a452b34. |
|
There was a status update in the "Cooking" section about the branch CMake adjustments for recent changes around unit tests. Will merge to 'master'. source: <pull.1797.git.1726687769585.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9a970bd. |
|
There was a status update in the "Cooking" section about the branch CMake adjustments for recent changes around unit tests. Will merge to 'master'. source: <pull.1797.git.1726687769585.gitgitgadget@gmail.com> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> With a4f50bb1e9b (t/unit-tests: introduce reftable library, 2024-09-16),
> however, the `UNIT_TEST_OBJS` list became a trio, and the CMake
> definition has to be adjusted again. Now that we can use the
> `parse_makefile_for_sources()` function without many complications,
> let's do that.
Am I correct to understand that it is not "trio"-ness (has three
things) that makes the approach feasible, but the fact that all
three things are concrete values?
The longer-term aspiration is to migrate everything to clar-based
unit tests, so the list will hopefully keep getting shorter and then
become empty (https://lore.kernel.org/git/Zt_lLsnylKJ9uoqj@pks.im/).
Will queue. Thanks.
|
|
This patch series was integrated into seen via git@1050870. |
|
This patch series was integrated into seen via git@cbb5b53. |
|
This patch series was integrated into master via git@cbb5b53. |
|
This patch series was integrated into next via git@cbb5b53. |
|
Closed via cbb5b53. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi,
On Mon, 23 Sep 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > With a4f50bb1e9b (t/unit-tests: introduce reftable library, 2024-09-16),
> > however, the `UNIT_TEST_OBJS` list became a trio, and the CMake
> > definition has to be adjusted again. Now that we can use the
> > `parse_makefile_for_sources()` function without many complications,
> > let's do that.
>
> Am I correct to understand that it is not "trio"-ness (has three
> things) that makes the approach feasible, but the fact that all
> three things are concrete values?
No, the fact that a third entry was added made it obvious that adding
hard-coded equivalents to the CMake definition is too much of a
whac-a-mole thing and that the `Makefile` parsing approach would be
preferable. With just two items, it was still kind of okay to duplicate
the list, but three is a trend.
> The longer-term aspiration is to migrate everything to clar-based
> unit tests, so the list will hopefully keep getting shorter and then
> become empty (https://lore.kernel.org/git/Zt_lLsnylKJ9uoqj@pks.im/).
Once it becomes empty, I will have to send another CMake-related patch
similar to when the `REFTABLE_TEST_OBJS` list went away, as the CMake
definition cannot handle such empty lists.
Ciao,
Johannes |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> The longer-term aspiration is to migrate everything to clar-based
>> unit tests, so the list will hopefully keep getting shorter and then
>> become empty (https://lore.kernel.org/git/Zt_lLsnylKJ9uoqj@pks.im/).
>
> Once it becomes empty, I will have to send another CMake-related patch
> similar to when the `REFTABLE_TEST_OBJS` list went away, as the CMake
> definition cannot handle such empty lists.
It may be ideal if all contributors learned and updated inputs to
all the build process options like cmake, make, and autoconf at the
same time as they futz with the code they build (in other words, a
change that makes such a list to empty includes cmake update), but
different people chipping in with help in their strong areas and
polishing the system as a whole as a collaborative effort is
probably the best practical alternative.
Thanks. |
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.
This is an add-on for
ps/reftable-excludeto let it build with CMake and Visual C.This patch on its own does not actually fix
vs-build; The patch "cmake: stop looking for REFTABLE_TEST_OBJS in the Makefile" which I submitted in https://lore.kernel.org/git/pull.1796.git.1726687400286.gitgitgadget@gmail.com is also needed for a successfulvs-build.