create /proc/sys/kernel/spl/gitrev with git hash #7965

ahrens · 2018-09-27T03:30:18Z

Motivation and Context

The existing mechanisms for determining what code is running in the kernel do not always correctly report the git hash. The versions reported there do not reflect changes made since configure was run (i.e. incremental builds do not update the version) and they are misleading if git tags are not set up properly. This applies to modinfo zfs, dmesg, and /sys/module/zfs/version.

This is related to #7931. It doesn't directly fix it (existing version info can still be stale), but it provides an alternative which decreases the need for a fix.

Description

There are complicated requirements on how the existing version is generated. Therefore we are leaving that alone, and adding a new mechanism to record and retrieve the git hash: cat /proc/sys/kernel/spl/gitrev

The gitrev is re-generated at compile time, when running make (including for incremental builds). The value is the output of git describe (or "unknown" if not in a git repo or there are uncommitted changes).

We're also removing /proc/sys/kernel/spl/version, which was never very useful.

How Has This Been Tested?

Check the contents of /proc/sys/kernel/spl/gitrev after full and incremental builds, and when there are uncommitted changes.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

pzakha

LGTM, few nits

scripts/make_gitrev.sh

pzakha · 2018-09-27T14:53:02Z

scripts/make_gitrev.sh

+#
+# Check if there are uncommitted changes
+#
+git diff-index --quiet HEAD -- || exit


Should we perhaps still generate a hash with "(modified)" appended to it, just so that people aren't too confused why it's suddenly "unknown"?
Also nit: -- looks unnecessary

For simplicity, I'm going to drop this (adding (modified)) if that's OK with you.

It's up to you. That should be very easy to implement though.

scripts/make_gitrev.sh

module/spl/spl-generic.c

scripts/make_gitrev.sh

dweeezil · 2018-10-02T15:45:57Z

How about adding spl_gitrev to the string logged in spa_history_log_version()?

behlendorf · 2018-10-02T18:42:24Z

Makefile.am

+gitrev:
+	-${top_srcdir}/scripts/make_gitrev.sh
+
+BUILT_SOURCES = gitrev


I think you should add a call to -${top_srcdir}/scripts/make_gitrev.sh in dist-hook:. This target is run at the end of make dist and it let's you add additional files prior to the tar. Right now it's used to ensure an updated META file is included in the the final tarball. This would ensure that tarball also includes the git hash if available, and a valid version of sys/zfs_gitrev.h.

I added a dependency on gitrev, which I think will do the same thing without duplicating code. Let me know if you see a problem with this (I'm not a makefile expert).

If it works, that sounds good to me.

Well, it does the same thing as what you suggested. However, zfs_gitrev.h doesn't end up in the tarball. I suspect that we would need to tell it to generate the file in an alternate location.

Turns out we copy the .c/.h files before doing dist-hook. So I had to also add a line to dist-hook to copy zfs_gitrev.h into the distdir. Now I have:

$ make dist ... make[2]: Entering directory '/export/home/delphix/zfs' ./scripts/make_gitrev.sh cp ./include/zfs_gitrev.h zfs-0.8.0/include; \ sed -i 's/Release:[[:print:]]*/Release: rc1/' \ zfs-0.8.0/META ... $ tar -tzf zfs-0.8.0.tar.gz |grep git zfs-0.8.0/scripts/make_gitrev.sh zfs-0.8.0/include/zfs_gitrev.h

ahrens · 2018-10-02T22:51:13Z

How about adding spl_gitrev to the string logged in spa_history_log_version()?

Great idea. What would you think about replacing "software version 5000/5" with software version <gitrev>? I would argue that the 5000/5 is basically useless. It's been the same for years despite numerous software changes.

ahrens · 2018-10-03T00:01:19Z

@behlendorf I'm getting the following errors, do you know how I should address them? I think I'm not using POSIX sh's echo, I'm using /bin/echo.

./scripts/make_gitrev.sh:37:11: warning: In POSIX sh, echo flags are undefined. [SC2039]
./scripts/make_gitrev.sh:52:11: warning: In POSIX sh, echo flags are undefined. [SC2039]
Makefile:1536: recipe for target 'shellcheck' failed

Incidentally, I don't get these errors when running make checkstyle locally, because I don't have shellcheck installed. I don't think it's a good idea to have this silently pass. We could at least print a warning if shellcheck is not installed.

dweeezil · 2018-10-03T02:49:23Z

@ahrens Yes, that's exactly what I was thinking. The existing "software version" thing doesn't seem very useful at all.

ahrens · 2018-10-03T02:59:01Z

Regarding the shellcheck errors, I noticed that several other scripts use echo -e but they are bash scripts. Should I change the new script to be a bash script, rather than sh?

ahrens · 2018-10-03T03:55:44Z

I also see that the kernel.org build is not working, it seems that it isn't generating the new header or it's being put in the wrong place.

fs/zfs/spl/spl-generic.c:48:10: fatal error: zfs_gitrev.h: No such file or directory
 #include "zfs_gitrev.h"
          ^~~~~~~~~~~~~~
compilation terminated.

The existing mechanisms for determining what code is running in the kernel do not always correctly report the git hash. The versions reported there do not reflect changes made since `configure` was run (i.e. incremental builds do not update the version) and they are misleading if git tags are not set up properly. This applies to `modinfo zfs`, `dmesg`, and `/sys/module/zfs/version`. There are complicated requirements on how the existing version is generated. Therefore we are leaving that alone, and adding a new mechanism to record and retrieve the git hash: `cat /proc/sys/kernel/spl/gitrev` The gitrev is re-generated at compile time, when running `make` (including for incremental builds). The value is the output of `git describe` (or "unknown" if not in a git repo or there are uncommitted changes). We're also removing /proc/sys/kernel/spl/version, which was never very useful. Signed-off-by: Matthew Ahrens <mahrens@delphix.com>

codecov · 2018-10-05T18:47:18Z

Codecov Report

Merging #7965 into master will decrease coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7965      +/-   ##
==========================================
- Coverage   78.65%   78.48%   -0.18%     
==========================================
  Files         377      377              
  Lines      114213   114213              
==========================================
- Hits        89837    89642     -195     
- Misses      24376    24571     +195

Flag	Coverage Δ
#kernel	`78.76% <100%> (-0.09%)`	⬇️
#user	`67.81% <100%> (-0.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfbe267...dd560b1. Read the comment docs.

ahrens · 2018-10-05T18:47:54Z

I believe I've addressed all the review feedback and this is now ready for final review and integration. Note that I moved the location of the new (generated) header to include/zfs_gitrev.h. This was needed so that the zfs kernel module could also use it (in spa_history_log_version).

The existing mechanisms for determining what code is running in the kernel do not always correctly report the git hash. The versions reported there do not reflect changes made since `configure` was run (i.e. incremental builds do not update the version) and they are misleading if git tags are not set up properly. This applies to `modinfo zfs`, `dmesg`, and `/sys/module/zfs/version`. There are complicated requirements on how the existing version is generated. Therefore we are leaving that alone, and adding a new mechanism to record and retrieve the git hash: `cat /proc/sys/kernel/spl/gitrev` The gitrev is re-generated at compile time, when running `make` (including for incremental builds). The value is the output of `git describe` (or "unknown" if not in a git repo or there are uncommitted changes). We're also removing /proc/sys/kernel/spl/version, which was never very useful. Reviewed by: Pavel Zakharov <pavel.zakharov@delphix.com> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed by: Tim Chase <tim@chase2k.com> Signed-off-by: Matthew Ahrens <mahrens@delphix.com> Closes openzfs#7931 Closes openzfs#7965

ahrens requested review from behlendorf and pzakha September 27, 2018 03:30

ahrens mentioned this pull request Sep 27, 2018

reported version is inaccurate #7931

Closed

pzakha approved these changes Sep 27, 2018

View reviewed changes

behlendorf requested changes Sep 27, 2018

View reviewed changes

ahrens added the Status: Revision Needed Changes are required for the PR to be accepted label Sep 28, 2018

ahrens force-pushed the githash branch from 3db9092 to 0ef5d11 Compare September 28, 2018 18:25

ahrens added Status: Code Review Needed Ready for review and testing and removed Status: Revision Needed Changes are required for the PR to be accepted labels Sep 28, 2018

ahrens force-pushed the githash branch from 0ef5d11 to a6013a7 Compare October 1, 2018 17:03

behlendorf reviewed Oct 2, 2018

View reviewed changes

ahrens force-pushed the githash branch from a6013a7 to f3dae3e Compare October 2, 2018 23:17

ahrens force-pushed the githash branch from f3dae3e to 937a9ab Compare October 3, 2018 00:01

ahrens force-pushed the githash branch from 937a9ab to 707c8e8 Compare October 3, 2018 20:35

ahrens added 8 commits October 5, 2018 11:46

CR feedback

45db6a7

add to packaging

80b2d0f

find path

0197407

more

a4940be

style

2005546

fix

631be01

dist

dd560b1

ahrens force-pushed the githash branch from 707c8e8 to dd560b1 Compare October 5, 2018 18:46

behlendorf approved these changes Oct 7, 2018

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 7, 2018

behlendorf merged commit 4cbde2e into openzfs:master Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create /proc/sys/kernel/spl/gitrev with git hash #7965

create /proc/sys/kernel/spl/gitrev with git hash #7965

ahrens commented Sep 27, 2018

pzakha left a comment

pzakha Sep 27, 2018

ahrens Sep 28, 2018

pzakha Sep 28, 2018

dweeezil commented Oct 2, 2018

behlendorf Oct 2, 2018

ahrens Oct 2, 2018

behlendorf Oct 2, 2018

ahrens Oct 2, 2018

ahrens Oct 5, 2018

ahrens commented Oct 2, 2018

ahrens commented Oct 3, 2018

dweeezil commented Oct 3, 2018

ahrens commented Oct 3, 2018

ahrens commented Oct 3, 2018

codecov bot commented Oct 5, 2018 •

edited

Loading

ahrens commented Oct 5, 2018

create /proc/sys/kernel/spl/gitrev with git hash #7965

create /proc/sys/kernel/spl/gitrev with git hash #7965

Conversation

ahrens commented Sep 27, 2018

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

pzakha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweeezil commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrens commented Oct 2, 2018

ahrens commented Oct 3, 2018

dweeezil commented Oct 3, 2018

ahrens commented Oct 3, 2018

ahrens commented Oct 3, 2018

codecov bot commented Oct 5, 2018 • edited Loading

Codecov Report

ahrens commented Oct 5, 2018

codecov bot commented Oct 5, 2018 •

edited

Loading