Skip to content
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.

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

Embed git branch and revision info #1357

Merged
merged 3 commits into from Mar 6, 2019

Conversation

@akohlmey
Copy link
Member

akohlmey commented Mar 5, 2019

Summary

When compiling LAMMPS in a working git checkout and git is installed, put some git revision info into LAMMPS_NS::LAMMPS::git_version and output it so one can tell which branch and revision a development binary was built with. Otherwise, this string will be empty and nothing additional is printed.

Related Issues

might also be useful to address some common scenarios of #1278
perhaps we can use this info or something equivalent like comparing the hash of the HEAD revision to what it was the last time cmake was run and thus detect when cmake needs to be re-run due to switching or updating branches. in my experience, those are two cases, where having re-running cmake triggered automatically, would be extremely useful and convenient.

Author(s)

Axel Kohlmeyer (Temple U)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

Not sure how portable the added script code is.

Implementation Notes

FindGit.cmake is available since 2010 so it should be in CMake 2.8.12, which is the minimum CMake version required

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

@akohlmey akohlmey added this to the Stable Release Spring 2019 milestone Mar 5, 2019

@akohlmey akohlmey self-assigned this Mar 5, 2019

@akohlmey akohlmey requested review from junghans, rbberger and sjplimp as code owners Mar 5, 2019

@ellio167

This comment has been minimized.

Copy link
Collaborator

ellio167 commented Mar 5, 2019

Is the branch name really helpful? It is not really tied to the commit in any permanent or reproducible way...

You also might want to record if the commit is dirty (local changes to files not committed to git).
I have this sort of thing implemented for the kim-api package here: https://github.com/openkim/kim-api/blob/master/cmake/Modules/DefineVersionVariables.cmake#L38 It's not perfect, but mostly works...

@sjplimp

sjplimp approved these changes Mar 5, 2019

Copy link
Contributor

sjplimp left a comment

good to me

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Mar 5, 2019

@akohlmey I would argue that creating some combination of all of these different strings into one git_version string prevents it of being useful for anything later. Anybody who wants to use that string for something will have to figure out how to parse it.

Why not keep it simple, store the full hash in one variable, the git branch in another, and finally record the output of git describe --dirty, which will output either patch_28Feb2019-6-g1c7d19198-dirty or patch_28Feb2019-6-g1c7d19198 depending if the working tree is dirty.

what I'm proposing is:
git_commit = git rev-parse HEAD
git_branch = git rev-parse --abbrev-ref HEAD
git_descriptor = git describe --dirty

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 5, 2019

Is the branch name really helpful? It is not really tied to the commit in any permanent or reproducible way...

@ellio167 thanks for your comments. yes, the branch information is important to me, especially in combination with the last tag (signifying the last patch release) and the number of commits since the tag. please keep in mind, that in my function as the person assessing pull requests and merging and maintaining various branches, i have typically around 5 build folders with different configurations and am frequently switching branches between 10 and 25 active branches at any point in time (master, unstable, stable and various feature branches of my own plus branches imported from remotes that provide pull requests) and am often building them in different folders to test for portability and cleanliness. the hash itself is not very helpful to me unless it want to go back to that specific revision, but the banch name and how many commits since the patch tag gives me useful information without having to look up anything.

You also might want to record if the commit is dirty (local changes to files not committed to git).

it is not very important to me, since most of the time my branches are dirty because of modifications to example inputs for testing features, but i am open to suggestions, for as long as it can be implemented cleanly for both conventional make and cmake.

I have this sort of thing implemented for the kim-api package here: https://github.com/openkim/kim-api/blob/master/cmake/Modules/DefineVersionVariables.cmake#L38 It's not perfect, but mostly works...

i did some googling, and there is a lot of this stuff out there and in some cases with really smart and fancy cmake scripting. I started this branch to implement what seemed most useful to me and what was easy to implement for both, conventional make and CMake. then i am hoping to get feedback from others about portability and improvements to the implementation. like yours. thanks, again.

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 5, 2019

@akohlmey I would argue that creating some combination of all of these different strings into one git_version string prevents it of being useful for anything later. Anybody who wants to use that string for something will have to figure out how to parse it.

Why not keep it simple, store the full hash in one variable, the git branch in another, and finally record the output of git describe --dirty, which will output either patch_28Feb2019-6-g1c7d19198-dirty or patch_28Feb2019-6-g1c7d19198 depending if the working tree is dirty.

what I'm proposing is:
git_commit = git rev-parse HEAD
git_branch = git rev-parse --abbrev-ref HEAD
git_descriptor = git describe --dirty

@sjplimp any problems, if i expand this to follow @rbberger 's suggestion?
it will lead to two or three more static const entries in lammps.h

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 5, 2019

@akohlmey I would argue that creating some combination of all of these different strings into one git_version string prevents it of being useful for anything later. Anybody who wants to use that string for something will have to figure out how to parse it.

Why not keep it simple, store the full hash in one variable, the git branch in another, and finally record the output of git describe --dirty, which will output either patch_28Feb2019-6-g1c7d19198-dirty or patch_28Feb2019-6-g1c7d19198 depending if the working tree is dirty.

what I'm proposing is:
git_commit = git rev-parse HEAD
git_branch = git rev-parse --abbrev-ref HEAD
git_descriptor = git describe --dirty

ok, i am fine with that in principle. but i don't like the "dirty" label. any objections if i replace it with "modified"? or just append a '*' or a '+'?
also, when going this route, i would add a static const bool have_git_info, that would make the test for the presence of git revision information more explicit. any objections?

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Mar 5, 2019

it will lead to two or three more static const entries in lammps.h
Seems fine. This will not produce different output to the screen and logfile
than currently (e,g, LAMMPS (1 Feb 2019)), unless you do a -h or an
info command? I don't want more than that for typical use.

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 5, 2019

it will lead to two or three more static const entries in lammps.h
Seems fine. This will not produce different output to the screen and logfile
than currently (e,g, LAMMPS (1 Feb 2019)), unless you do a -h or an
info command? I don't want more than that for typical use.

the requirements for embedding git info is, that a) you have a working git installation, and b) you are compiling inside a LAMMPS tree created by git clone.

so people that download tarballs from lammps.sandia.gov or github will definitely not see this.

it will help us to see, if people report problems with LAMMPS versions that were compiled from a git checkout, where the version info from version.h is not accurate, anymore. this PR won't catch all of these, but a lot of them, and it will help me significantly when reviewing/testing pull requests and new features.

@ellio167

This comment has been minimized.

Copy link
Collaborator

ellio167 commented Mar 5, 2019

Is the branch name really helpful? It is not really tied to the commit in any permanent or reproducible way...

@ellio167 thanks for your comments. yes, the branch information is important to me, especially in combination with the last tag (signifying the last patch release) and the number of commits since the tag. please keep in mind, that in my function as the person assessing pull requests and merging and maintaining various branches, i have typically around 5 build folders with different configurations and am frequently switching branches between 10 and 25 active branches at any point in time (master, unstable, stable and various feature branches of my own plus branches imported from remotes that provide pull requests) and am often building them in different folders to test for portability and cleanliness. the hash itself is not very helpful to me unless it want to go back to that specific revision, but the banch name and how many commits since the patch tag gives me useful information without having to look up anything.

You also might want to record if the commit is dirty (local changes to files not committed to git).

it is not very important to me, since most of the time my branches are dirty because of modifications to example inputs for testing features, but i am open to suggestions, for as long as it can be implemented cleanly for both conventional make and cmake.

I have this sort of thing implemented for the kim-api package here: https://github.com/openkim/kim-api/blob/master/cmake/Modules/DefineVersionVariables.cmake#L38 It's not perfect, but mostly works...

i did some googling, and there is a lot of this stuff out there and in some cases with really smart and fancy cmake scripting. I started this branch to implement what seemed most useful to me and what was easy to implement for both, conventional make and CMake. then i am hoping to get feedback from others about portability and improvements to the implementation. like yours. thanks, again.

@akohlmey Sounds good. I think I see how you hope to use this and I can see that it would be helpful.

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Mar 5, 2019

@akohlmey ok with me. So how about git describe --dirty=-modified?

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Mar 5, 2019

I agree with Steve, I don't really want this in every logfile. Can we make this optional, like if someone reports a bug, tell them to run again with a special flag to get the tag?

@akohlmey akohlmey requested a review from sjplimp Mar 5, 2019

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 5, 2019

now we have 3 individual strings and a boolean, output is only when compiled inside a git checkout and only with ./lmp_xxx -h or using echo "info config" | ./lmp_xxx. e.g.:


Large-scale Atomic/Molecular Massively Parallel Simulator - 28 Feb 2019
Git info (embed-git-revision / patch_28Feb2019-7-gc5a3b034c)

Usage example: ./lmp -var t 300 -echo screen -in in.alloy

List of command line options supported by this LAMMPS executable:

-echo none/screen/log/both  : echoing of input script (-e)
-help                       : print this help message (-h)
-in filename                : read input from file, not stdin (-i)
-kokkos on/off ...          : turn KOKKOS mode on or off (-k)
-log none/filename          : where to send log output (-l)
-mpicolor color             : which exe in a multi-exe mpirun cmd (-m)
-nocite                     : disable writing log.cite file (-nc)
-package style ...          : invoke package command (-pk)
-partition size1 size2 ...  : assign partition sizes (-p)
-plog basename              : basename for partition logs (-pl)
-pscreen basename           : basename for partition screens (-ps)
-restart2data rfile dfile ... : convert restart to data file (-r2data)
-restart2dump rfile dgroup dstyle dfile ... 
                            : convert restart to dump file (-r2dump)
-reorder topology-specs     : processor reordering (-r)
-screen none/filename       : where to send screen output (-sc)
-suffix gpu/intel/opt/omp   : style suffix to apply (-sf)
-var varname value          : set index style variable (-v)

OS: Linux 4.20.13-200.fc29.x86_64 on x86_64

Compiler: GNU C++ 8.3.1 20190223 (Red Hat 8.3.1-2) with OpenMP 4.5

Active compile time flags:

-DLAMMPS_GZIP
-DLAMMPS_PNG
-DLAMMPS_JPEG
-DLAMMPS_FFMPEG
-DLAMMPS_SMALLBIG

sizeof(smallint):  32-bit
sizeof(imageint):  32-bit
sizeof(tagint):    32-bit
sizeof(bigint):    64-bit
...

or

$ echo info config | ./lmp 
LAMMPS (28 Feb 2019)
  using 1 OpenMP thread(s) per MPI task

Info-Info-Info-Info-Info-Info-Info-Info-Info-Info-Info
Printed on Tue Mar  5 17:50:36 2019


LAMMPS version: 28 Feb 2019 / 20190228
Git info: embed-git-revision / patch_28Feb2019-7-gc5a3b034c / c5a3b034ce54ad213fe1c30f4636dc922f5607cf

OS information: Linux 4.20.13-200.fc29.x86_64 on x86_64

sizeof(smallint):  32-bit
sizeof(imageint):  32-bit
sizeof(tagint):    32-bit
sizeof(bigint):    64-bit

Compiler: GNU C++ 8.3.1 20190223 (Red Hat 8.3.1-2) with OpenMP 4.5

Active compile time flags:

-DLAMMPS_GZIP
-DLAMMPS_PNG
-DLAMMPS_JPEG
-DLAMMPS_FFMPEG
-DLAMMPS_SMALLBIG

Installed packages:

ASPHERE BODY CLASS2 COLLOID CORESHELL DIPOLE GPU GRANULAR KOKKOS KSPACE 
MANYBODY MC MISC MOLECULE PERI POEMS PYTHON QEQ REPLICA RIGID SHOCK SNAP SPIN 
SRD USER-ATC USER-AWPMD USER-BOCS USER-CGDNA USER-CGSDK USER-COLVARS 
USER-DIFFRACTION USER-DPD USER-DRUDE USER-EFF USER-FEP USER-LB USER-MANIFOLD 
USER-MEAMC USER-MESO USER-MGPT USER-MISC USER-MOFFF USER-MOLFILE USER-OMP 
USER-PHONON USER-PLUMED USER-PTM USER-QTB USER-REAXC USER-SCAFACOS USER-SDPD 
USER-SMD USER-SMTBQ USER-SPH USER-TALLY USER-UEF VORONOI 

Info-Info-Info-Info-Info-Info-Info-Info-Info-Info-Info

@akohlmey akohlmey merged commit 4a17cb3 into lammps:master Mar 6, 2019

6 checks passed

lammps/pull-requests/cmake/new-cmake-serial-pr head run ended
Details
lammps/pull-requests/new-build-docs-pr head run ended
Details
lammps/pull-requests/new-kokkos-omp-pr head run ended
Details
lammps/pull-requests/new-openmpi-pr head run ended
Details
lammps/pull-requests/new-serial-pr head run ended
Details
lammps/pull-requests/new-shlib-pr head run ended
Details

@akohlmey akohlmey deleted the akohlmey:embed-git-revision branch Mar 6, 2019

@junghans
Copy link
Member

junghans left a comment

I just saw this now.

ERROR_QUIET
OUTPUT_STRIP_TRAILING_WHITESPACE)
endif()
endif()

This comment has been minimized.

Copy link
@junghans

junghans Mar 6, 2019

Member

I think this has to be done at buildtime not CMake time (e.g. using a CMake script) Otherwise changes won’t be picked up without rerunning CMake.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Mar 6, 2019

Author Member

so far, for the cases, that i tested, cmake was run always when i expected the header to be updated. time will tell, if it skips it.

This comment has been minimized.

Copy link
@junghans

junghans Mar 6, 2019

Member

Ok here is an example of that what I did:

git checkout master
mkdir build
cd build
cmake ../src
make
echo " " >> ../src/angle.cpp
make

the lmpgitversion.h stays unchanged even though -modified should have been added, then if I do:

git add -u
git commit -m "foobar"
make

the lmpgitversion.h doesn't get updated now either with the new sha.

In VOTCA we solve that issue by doing some like this:
https://github.com/votca/csg/blob/master/src/libcsg/CMakeLists.txt#L31
with
https://github.com/votca/csg/blob/master/CMakeModules/gitversion.cmake

set(temp_git_branch "(unknown)")
set(temp_git_describe "(unknown)")
set(temp_git_info "false")
if(GIT_FOUND)

This comment has been minimized.

Copy link
@junghans

junghans Mar 6, 2019

Member

AND EXISTS ${CMAKE_SOURCE_DIR}/.git to avoid picking up a random git sha when building for a tarball.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Mar 6, 2019

Author Member

there was an alternate approach to do the same in place already, but this is simpler.

This comment has been minimized.

Copy link
@junghans

junghans Mar 6, 2019

Member

Sorry where is it?

This comment has been minimized.

Copy link
@junghans

junghans Mar 6, 2019

Member

Got it, see my comment there.

@akohlmey akohlmey referenced this pull request Mar 6, 2019

Merged

Collected small changes for the next patch release #1361

6 of 10 tasks complete
@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Mar 7, 2019

@akohlmey The file lmpgitversion.h shows up as untracked, can we add this to the .gitignore file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.