Add git version info for parent commits (#597) by bartlettroscoe · Pull Request #598 · TriBITSPub/TriBITS

bartlettroscoe · 2024-01-05T00:00:18Z

WIP: This PR adds tests and functionality for adding git parent info for:

Add parents' commit hashes to repoVersionStringOut in tribits_generate_single_repo_version_string() #597

Added test TribitsHelloWorld_config_git_version that uses mockprogram.py as git to generate and check git version info. ToDo: We should also check the generated TribitsHelloWorldRepoVersion.txt file

For some reason, mockprogram.py prints an newline that raw git does not. But this should work for raw git too.

Renamed the test TribitsHelloWorld_config_git_version to TribitsHelloWorld_config_git_version_single_repo_one_parent to correctly state the scope of the test and allow for future similar tests with slightly a difference scopes (such as single_repo_two_parents, multi_repo_one_parent, etc).

…euse (#597) Moved git commit info functionality of tribits_generate_single_repo_version_string to a separate helper function named tribites_generate_commit_info_string. The core functionality and output is the same. This is in prep for reuse of that functionality inside of the original tribits_generate_single_repo_version string.

…#597) Removed trailing whitespace to keep git command outputs consistent with other functions performing git commands and for the output to be more processable.

achauphan · 2024-01-12T18:25:09Z

Forced push for a rebase on dce52b6 which had a typo in the commit message and triggered the codespell.

Make tribits_generate_commit_info_string output commit info based on a passed in commit sha1.

…est (#597) Updated mockprogram inputs and outputs to be more specific about which commit message is being outputted.

If the HEAD sha1 contains more than one parent, include each parent's commit info in the final repoVersionStringOut of tribits_generate_single_repo_version_string.

Forgot to double quote variable and caused configure failures during Github Actions testing.

achauphan · 2024-01-13T02:23:31Z

We're getting close! From the CDash results, looks like these outputs were not expected for the tests that check for multi-repo version outputs. Going to request a review anyways as I look into this next week.

Tagging because I cannot add @bartlettroscoe as he's owner and @sebrowne as he's not a collaborator

sebrowne · 2024-01-15T14:31:03Z

Looks fine to me

bartlettroscoe · 2024-01-15T15:33:11Z

@achauphan, I will add a more detailed review later but I think the issue with the failure of the tests:

Site	Build Name	Test Name	Status	Time	Details	Build Time
ubuntu-latest	pr-598_tribits_cmake-3.24.3_makefiles_python-3.8_g++-11_noninja	TriBITS_CTestDriver_TribitsExMetaProj_clone_custom_branch_remote	Failed	12s 750ms	Completed (Failed)	2024-01-12T21:01:39 EST
ubuntu-latest	pr-598_tribits_cmake-3.23.1_makefiles_python-3.8_g++-9	TriBITS_CTestDriver_TribitsExMetaProj_clone_custom_branch_remote	Failed	11s 740ms	Completed (Failed)	2024-01-12T21:01:41 EST
ubuntu-latest	pr-598_tribits_cmake-3.25.2_makefiles_python-3.8_g++-11	TriBITS_CTestDriver_TribitsExMetaProj_clone_custom_branch_remote	Failed	14s 440ms	Completed (Failed)	2024-01-12T21:01:50 EST
ubuntu-latest	pr-598_tribits_cmake-3.24.3_makefiles_python-3.8_g++-11_noninja	TriBITS_CTestDriver_TribitsExMetaProj_clone_default_branch_remote	Failed	15s 80ms	Completed (Failed)	2024-01-12T21:01:39 EST
ubuntu-latest	pr-598_tribits_cmake-3.23.1_makefiles_python-3.8_g++-9	TriBITS_CTestDriver_TribitsExMetaProj_clone_default_branch_remote	Failed	13s 600ms	Completed (Failed)	2024-01-12T21:01:41 EST
ubuntu-latest	pr-598_tribits_cmake-3.25.2_makefiles_python-3.8_g++-11	TriBITS_CTestDriver_TribitsExMetaProj_clone_default_branch_remote	Failed	17s 740ms	Completed (Failed)	2024-01-12T21:01:50 EST

shown here. Not clear what the cause is for this but I think that, by default, we should not be printing parent info.

bartlettroscoe

@achauphan, this is looking pretty good. It looks like like have have read quite a bit of CMake documentation for all of this new functionality :-)

I have just a few suggestions to improve this:

Add global cmake cache var named something like ${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS and set it to OFF by default. (You would define this next to where the option ${PROJECT_NAME}_GENERATE_REPO_VERSION_FILE is defined.) This will also need to be documented in the file TribitsBuildReferenceBody.rst and optionally also in the file TribitsCoreDetailedReference.rst if projects will be providing a default value ${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS_DEFAULT.
Add the option INCLUDE_PARENT_COMMITS to tribits_generate_single_repo_version_string() and update the code that calls this to pass in that option based on the value of ${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS. (The logic to pass in INCLUDE_PARENT_COMMITS or now would be put into the function tribits_generate_repo_version_file_string().) (It would nice if the file TribitsGitRepoVersionInfo.cmake could avoid project-level settings so this could could be reused in non-TriBITS projects in the future. Currently that file does not contain PROJECT_NAME anywhere and there is no concept of a TriBITS Project. In fact, that makes this file TribitsGlobalMacros.cmake a candidate to be moved to tribits/core/utils/, see TriBITS Core Directory Contents.)
Change the git log calls to be in the standard order git log [<options>] [<revision-range>] [[--] <path>...] (see below)
In tribits_generate_single_repo_version_string(), change for a foreach() loop over indexes to a foreach() loop over list elements and change to 1-based parent indexing like Git does (and rename the var index to parentIdx). See below.
Eliminate calling tribits_git_repo_sha1() and just pass in HEAD (see below).
Move the definition of tribits_generate_commit_info_string() below the definition of tribits_generate_single_repo_version_string(). (I have been trying to convert TriBITS code to Clean Code ordering of functions where higher level functions are listed above the functions it calls. This is called called the "Newspaper Metaphor", see "Clean Code" book by Robert Martin.)
See other suggestions below to improve the readability and maintainability of the code (IMHO).

Added global cmake cache variable `<PROJECT_NAME>_SHOW_GIT_COMMIT_PARENTS:BOOL` that is default to OFF. This will be a project based variable to including parent commit info in the repo version output.

Reordered git log arguments to the expected order as well as condensed cmake command arguments into a single line.

Instead of passing in the deferenced HEAD SHA1 to tribites_generate_commit_info_string, simply just pass in the HEAD pointer keyword for Git to deference itself.

…tput (#597) In the off chance that Git decides to change the output of git log -1 "--pretty=format:%p", use a regex match and replace in cmake to match consecutive spaces and replace them with a single ;.

Simplified foreach loop in tribits_generate_single_repo_version_string and changed the parent indexing to be 1-based instead of 0-based.

achauphan · 2024-01-17T16:26:53Z

@bartlettroscoe @sebrowne whenever ya'll have time, can ya'll give this another review?

sebrowne

Overall looks fine to me. I made a couple of style-ish recommendations.

So the concept is that TriBITS will provide all of the parents, and then we can process them as we see fit in the CDash analysis?

sebrowne · 2024-01-22T20:53:58Z

      COMMAND ${GIT_EXECUTABLE} log -1 "--pretty=format:%H"
      WORKING_DIRECTORY  ${gitRepoDir}
-      RESULT_VARIABLE  gitCmndRtn  OUTPUT_VARIABLE  gitCmndOutput
+      RESULT_VARIABLE  gitCmndRtn OUTPUT_VARIABLE  gitCmndOutput


I'm nitpicking, but leave this whitespace alone for git blame purposes

@sebrowne, I actually requested this to compress the vertical density of the code.

sebrowne · 2024-01-22T20:57:52Z

+      SINGLE_REPO_VERSION INCLUDE_COMMIT_PARENTS)
+  else()
+    tribits_generate_single_repo_version_string(
+      ${CMAKE_CURRENT_SOURCE_DIR} SINGLE_REPO_VERSION)


I would suggest splitting here for readability/comparability with the if() case.

bartlettroscoe

@achauphan, thanks for making these changes! This was a really easy set up updates to review commit-by-commit given your excellent separate changes commits.

I just had one follow-up suggestion:

Change the INCLUDE_PARENT_COMMITS argument to tribits_generate_single_repo_version_string() defined with cmake_parse_arguments() from <option> to <one_value_keywords>. That matches the documentation for that argument and will help to avoid some code duplication. (See detailed comments below.)

Otherwise, this looks great!

bartlettroscoe · 2024-01-23T18:08:39Z

      COMMAND ${GIT_EXECUTABLE} log -1 "--pretty=format:%H"
      WORKING_DIRECTORY  ${gitRepoDir}
-      RESULT_VARIABLE  gitCmndRtn  OUTPUT_VARIABLE  gitCmndOutput
+      RESULT_VARIABLE  gitCmndRtn OUTPUT_VARIABLE  gitCmndOutput


@sebrowne, I actually requested this to compress the vertical density of the code.

bartlettroscoe · 2024-01-23T18:21:59Z

  # C) Get each parent's commit info and format the output

-  if (headNumParents GREATER 1)
+  if (PARSE_INCLUDE_COMMIT_PARENTS)


I like this. If the user is requesting the parent commit info, it makes sense to show them even if the top commit is not a merge commit and there is just one parent. That is useful information for the user in case they were expecting a merge commit. Excellent!

bartlettroscoe · 2024-01-23T18:36:17Z

+  if (${${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS})
+    tribits_generate_single_repo_version_string(
+      ${CMAKE_CURRENT_SOURCE_DIR}
+      SINGLE_REPO_VERSION INCLUDE_COMMIT_PARENTS)
+  else()
+    tribits_generate_single_repo_version_string(
+      ${CMAKE_CURRENT_SOURCE_DIR} SINGLE_REPO_VERSION)
+  endif()


One way to reduce duplication in a case like this in CMake is to create a variable that can be the empty string or have the value INCLUDE_COMMIT_PARENTS with code like this:

set(includeCommitParentsArg "") if (${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS) set(includeCommitParentsArg INCLUDE_COMMIT_PARENTS) endif() tribits_generate_single_repo_version_string( ${CMAKE_CURRENT_SOURCE_DIR} singleRepoVersion ${includeCommitParentsArg} )

Note that above I did not quote ${includeCommitParentsArg} so that if it evaluates to the empty string, that it will pass not argument at all. (This is an example of taking advantage of behavior unique to CMake.)

The other option that I suggest above is to take advantage of the ${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS bool value of TRUE/FALSE and change the optional input argument to tribits_generate_single_repo_version_string() from an option to a single value and pass it in like:

tribits_generate_single_repo_version_string( ${CMAKE_CURRENT_SOURCE_DIR} singleRepoVersion INCLUDE_COMMIT_PARENTS ${${PROJECT_NAME}_SHOW_GIT_COMMIT_PARENTS} )

All things considered, I think that may be a better option.

bartlettroscoe · 2024-01-23T18:52:44Z

 #
 #   tribits_generate_single_repo_version_string(<gitRepoDir>
-#     <repoVersionStringOut>)
+#     <repoVersionStringOut> INCLUDE_PARENT_COMMITS [ON|OFF])


This is actually the incorrect documentation. When using the <options> argument with cmake_parse_arguments(), the documentation would be listed as:

# tribits_generate_single_repo_version_string(<gitRepoDir> # <repoVersionStringOut> [INCLUDE_PARENT_COMMITS])

However, as I mention below, I would suggest instead using the <one_value_keywords> argument with cmake_parse_arguments() instead so that the above documentation would be correct, but making it clear that you can completely leave out the INCLUDE_PARENT_COMMITS <value> argument with:

# tribits_generate_single_repo_version_string(<gitRepoDir> # <repoVersionStringOut> [INCLUDE_PARENT_COMMITS ON|OFF])

Ah thanks for the catch. I forgot to update that documentation as I did initially write it using one_value_keywords but decided to switch to using optional since it felt more "optional"

bartlettroscoe · 2024-01-23T18:55:31Z

+  cmake_parse_arguments( PARSE_ARGV 2
+    PARSE "INCLUDE_COMMIT_PARENTS" # prefix, optional
+    "" "" # one_value_keywords, multi_value_keyword
+    )


Change this to:

cmake_parse_arguments( PARSE_ARGV 2 PARSE "" # prefix, optional "INCLUDE_COMMIT_PARENTS" "" # one_value_keywords, multi_value_keyword )

and then the optional argument becomes:

# tribits_generate_single_repo_version_string(<gitRepoDir> # <repoVersionStringOut> [INCLUDE_PARENT_COMMITS ON|OFF])

bartlettroscoe · 2024-01-23T21:19:37Z

@achauphan, since it took me so long to get to this review, I can make the final changes and get this merged (and set up the snapshot to Trilinos PR). Or if you want to make the final change, I can review that and get it merged. Just let me know.

P.S. I could to document the TriBITS snaphsottting to Trilinos on the Trilinos wiki if you would like to give that a try.

achauphan · 2024-01-23T22:29:26Z

@achauphan, since it took me so long to get to this review, I can make the final changes and get this merged (and set up the snapshot to Trilinos PR). Or if you want to make the final change, I can review that and get it merged. Just let me know.

P.S. I could to document the TriBITS snaphsottting to Trilinos on the Trilinos wiki if you would like to give that a try.

@bartlettroscoe thanks for another round of reviews, this one was kinda fun! Yes, if you could, please go a head and make the final changes and get this snapshotted into Trilinos :)

I would love to go through learning the process of snapshotting this myself, but would like to try to use my time left for this sprint to try and finish up the python tool.

Implemented usage of previously added global cmake variable, `PROJECT_NAME_SHOW_GIT_COMMIT_PARENTS` in `tribits_generate_single_repo_version_string`. This is done with an optional argument named INCLUDE_COMMIT_PARENTS whenever calling `tribits_generate_single_repo_version_string`

With Clean Code ordering (i.e. the Newspaper Metaphor), in general, the functions should be ordered from top-level to low-level in the calling order. That is, the functions that are called in a given function should be listed below that function.

Cleaned up empty lines in `TribitsHelloWorld_Tests.cmake`

bartlettroscoe · 2024-01-24T00:36:20Z

Yes, if you could, please go a head and make the final changes and get this snapshotted into Trilinos :)

Doing that now ...

This allows the calling code to be more compact. Since this function is not really called by any external users (perhaps ever), this seems like a good trade-off.

…HANDLING_ONLY=ON (#597) This spead up these tests on my machine 'crf450' from about 3.4 sec to about 0.45 sec. We don't need to enable the compilers and process the package CMakeLists.txt files to test this version info.

This is a variable that project developers and users should know about.

…up (#597) Using camelCase for local vars helps to differentiate from UPPER_CACHE vars for function argument keywords and global vars. I figured I would clean this function up while I was working on it. Now this fits in one screen-shot for me.

…utils/ (#597) I noticed that this module does not depend on the TriBITS framework in any way so it should live under the core/utils/ directory to make that clear and to allow easy reuse in non-TriBITS projects that would like to use it.

…IT_COMMIT_PARENTS (#597) We eventually need to use this for all of those vars to make that code more compact.

…aintainers guide (#597)

bartlettroscoe · 2024-01-24T17:23:52Z

@achauphan, I created the stacked PR:

Should be final commits for Git version parent info PR #598 (#597) #601

against this PR for which should contain the final changes needed to merge this PR #598. (A stacked PR makes it easier to review a chuck of commits at one time. But I would still suggest reviewing the commits one-at-a-time.) If you have time to review that stacked PR, please let me know. Otherwise, I will merge that PR to this PR and move forward with the final merge and snapshotting to Trilinos.

I noticed this while looking at the documentation while working on #597.

#597) This sets the option: -DTribitsExMetaProj_SHOW_GIT_COMMIT_PARENTS=ON in the inner CMake configuration with a multi-repo setup so we can test the extraction and printing of the parent commit info. This tests a call of the function tribits_generate_single_repo_version_string() for extra repos from the function tribits_generate_repo_version_file_string(). This test also validates that the correct Git commands are called since this uses real Git on a Git repos. The downside of this updated test is that it makes it a little more difficult to maintain these tests when we have to update these snapshotted Git repos.

…fo-rab Should be final commits for Git version parent info PR #598 (#597)

rabartlett1972

@achauphan reviewed and approved the final set of commits from the stacked PR #601 so when the GHA builds run and pass, this can merge.

…arent-info (TriBITSPub/TriBITS#597) Main purpose is to pull in the TriBITS PR for getting the git repo parent commit info: * TriBITSPub/TriBITS#598

bartlettroscoe added 4 commits January 4, 2024 16:49

Add test TribitsHelloWorld_config_git_version (#597)

00b3c6f

Added test TribitsHelloWorld_config_git_version that uses mockprogram.py as git to generate and check git version info. ToDo: We should also check the generated TribitsHelloWorldRepoVersion.txt file

Print the stdout/stderr from the failed git command (#587)

80191d4

Strip the whitespace from git command (#597)

98750bb

For some reason, mockprogram.py prints an newline that raw git does not. But this should work for raw git too.

Add check of generatead TribitsHelloWorldRepoVersion.txt file (#597)

1db9378

bartlettroscoe added type: enhancement component: core labels Jan 5, 2024

bartlettroscoe assigned achauphan Jan 5, 2024

achauphan added 3 commits January 12, 2024 11:20

Removed trailing whitespace for tribits_git_repo_sha1 git cmnd output (…

8966263

…#597) Removed trailing whitespace to keep git command outputs consistent with other functions performing git commands and for the output to be more processable.

achauphan force-pushed the 597-config-git-version-info branch from 2374542 to 8966263 Compare January 12, 2024 18:22

achauphan added 5 commits January 12, 2024 12:37

Add commit sha1 input for tribits_generate_commit_info_string (#597)

fd3a2e9

Make tribits_generate_commit_info_string output commit info based on a passed in commit sha1.

Updated TribitsHelloWorld_config_git_version_single_repo_one_parent t…

be3a494

…est (#597) Updated mockprogram inputs and outputs to be more specific about which commit message is being outputted.

Added git version info for parent commits (#597)

0dacf47

If the HEAD sha1 contains more than one parent, include each parent's commit info in the final repoVersionStringOut of tribits_generate_single_repo_version_string.

Added double quotes around variable (#597)

f34a429

Forgot to double quote variable and caused configure failures during Github Actions testing.

Fix type causing sphinx build to fail (#597)

3af75e3

bartlettroscoe commented Jan 15, 2024

View reviewed changes

achauphan added 5 commits January 15, 2024 21:27

Added global cmake cache var to show git commit parents (#597)

a40bd5e

Added global cmake cache variable `<PROJECT_NAME>_SHOW_GIT_COMMIT_PARENTS:BOOL` that is default to OFF. This will be a project based variable to including parent commit info in the repo version output.

Cleaned up git log and cmake command argument formatting (#597)

592ea0d

Reordered git log arguments to the expected order as well as condensed cmake command arguments into a single line.

Updated tribits_gernerate_single_repo_version_string to use HEAD (#597)

e4b19b6

Instead of passing in the deferenced HEAD SHA1 to tribites_generate_commit_info_string, simply just pass in the HEAD pointer keyword for Git to deference itself.

Updated to use cmake string(REGEX REPLACE) when converting git log ou…

852e651

…tput (#597) In the off chance that Git decides to change the output of git log -1 "--pretty=format:%p", use a regex match and replace in cmake to match consecutive spaces and replace them with a single ;.

Simplified loop and changed to 1-based indexing (#597)

f6b6442

Simplified foreach loop in tribits_generate_single_repo_version_string and changed the parent indexing to be 1-based instead of 0-based.

achauphan mentioned this pull request Jan 17, 2024

Add tool for analyzing and reporting random CDash test failures #600

Open

sebrowne reviewed Jan 22, 2024

View reviewed changes

bartlettroscoe commented Jan 23, 2024

View reviewed changes

achauphan and others added 4 commits January 23, 2024 17:28

Cleaned up empty lines (#597)

ba7e8da

Cleaned up empty lines in `TribitsHelloWorld_Tests.cmake`

Add missing () on function block doc (#597)

628b27e

bartlettroscoe added 7 commits January 23, 2024 17:51

Switch to one_value_keywords for INCLUDE_COMMIT_PARENTS (#597)

ce4728b

This allows the calling code to be more compact. Since this function is not really called by any external users (perhaps ever), this seems like a good trade-off.

Document <Project>_TRACE_DEPENDENCY_HANDLING_ONLY (#597)

3aa1ea8

This is a variable that project developers and users should know about.

Use tribits_advanced_set_cache_var_and_default() for <Project>_SHOW_G…

3bed1f9

…IT_COMMIT_PARENTS (#597) We eventually need to use this for all of those vars to make that code more compact.

Document setting <Project>_SHOW_GIT_COMMIT_PARENTS_DEFAULT in users/m…

ba27309

…aintainers guide (#597)

bartlettroscoe force-pushed the 597-config-git-version-info branch from b07e361 to ba7e8da Compare January 24, 2024 17:08

bartlettroscoe mentioned this pull request Jan 24, 2024

Should be final commits for Git version parent info PR #598 (#597) #601

Merged

bartlettroscoe requested review from achauphan and removed request for achauphan January 24, 2024 17:20

bartlettroscoe added 3 commits January 24, 2024 16:24

Fix some typos in tribits_ctest_driver() (#597)

731841e

I noticed this while looking at the documentation while working on #597.

Merge pull request #601 from bartlettroscoe/597-config-git-version-in…

da80610

…fo-rab Should be final commits for Git version parent info PR #598 (#597)

bartlettroscoe enabled auto-merge January 25, 2024 00:33

Merge branch 'master' into 597-config-git-version-info

2f17d8e

rabartlett1972 approved these changes Jan 25, 2024

View reviewed changes

bartlettroscoe merged commit 9328b34 into master Jan 25, 2024

This was referenced Jan 25, 2024

Add parents' commit hashes to repoVersionStringOut in tribits_generate_single_repo_version_string() #597

Closed

Update TriBITS snapshot 2024-01-24 and set Trilinos_SHOW_GIT_COMMIT_PARENTS=ON for PR builds trilinos/Trilinos#12695

Merged

bartlettroscoe mentioned this pull request Feb 14, 2024

TriBITS snapshot 2024-02-13 trilinos/Trilinos#12741

Merged

Conversation

bartlettroscoe commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achauphan commented Jan 12, 2024

Uh oh!

achauphan commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebrowne commented Jan 15, 2024

Uh oh!

bartlettroscoe commented Jan 15, 2024

Uh oh!

bartlettroscoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achauphan commented Jan 17, 2024

Uh oh!

sebrowne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartlettroscoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartlettroscoe commented Jan 23, 2024

Uh oh!

achauphan commented Jan 23, 2024

Uh oh!

bartlettroscoe commented Jan 24, 2024

Uh oh!

bartlettroscoe commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabartlett1972 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bartlettroscoe commented Jan 5, 2024 •

edited

Loading

achauphan commented Jan 13, 2024 •

edited

Loading

bartlettroscoe commented Jan 24, 2024 •

edited

Loading