Created attachment 94574 [details]
patch

I can see how revision numbers can be useful.
However, I would not want to see them in the output of valgrind --version. That output should stay as is for consistency with existing practice and with other tools.
Instead, valgrind -v --version should output the revision numbers.
Note, that there are two revision numbers:
(1) from valgrind
(2) from VEX (which is a separate repository)

Created attachment 101947 [details]
try 2

This only shows svn info if -v/--verbose is given.

Need to find a way to avoid redefining the package version..

Created attachment 101951 [details]
try 3

This version uses PACKAGE_VERSION, avoiding the redefinition.

Tested with both a proper subversion tree, as well as a git svn checkout.

Please review / apply

I think this is a good idea.
Jute one question: as I understand, the svn versions are retrieved at
configure time.
This means that the following will give wrong versions:
    ./autogen.sh
    configure
    make install
    svn update
    make install

(often, there is no need to reconfigure after a svn update).
Wouldn't it be better to get the svn version during the make 
and e.g. re-create a file   valgrind_svn_version.h
if the current version differs from the version in the file ?

Created attachment 102288 [details]
try 4, run from make instead of configure

Makefiles are not my specialty, so comments welcome.

This has been tested working with svn and git svn.

I had a look and overall it looks pretty good.
However 'pushd/popd' is a bash'ism and won't work in other shells.
Please can you try this also under shells, for example ksh?

What happens (and what is the version) if this is run not inside a SVN/GIT repository? For example one can run 'make dist', take the tarball, unpack it somewhere else and build the stuff again.

Also this construct:
"if test -d .svn; then svnversion -n .; elif test -d .git; then git svn info | \
 grep ^Revision | cut -d ' ' -f2 | tr -d '\n'; fi"
can be reused, for example through a Makefile variable:

EXTRACT_SVN_VERSION = "<above>"
vgversion.h:
    ...
    ... $(shell $(EXTRACT_SVN_VERSION)) ...

Note also that we should avoid modifying vgversion.h
each time we call make, as otherwise this will imply to recompile
the including file(s).

So, what we need is a command like:
   create_or_update_vgversion_h_if_needed
which compares the current version with the version in the file
and creates or updates the file if needed.

Created attachment 102375 [details]
try 5

Try 5 only updates the header if if has changed, and moves the git/svn parsing code to a Makefile variable.

Additionally, in the case of git svn, it checks that .git/svn exists rather than .git. There are (unofficial) git repos for valgrind, e.g., https://github.com/mslusarz/valgrind, that this would fail for. Adding git support would be doable, but I don't think it's worth it when valgrind itself is on SVN. I'd like git svn to be supported for the official tree (since that's what I use for development) :).

(In reply to Ivo Raisr from comment #7)
> I had a look and overall it looks pretty good.
> However 'pushd/popd' is a bash'ism and won't work in other shells.
> Please can you try this also under shells, for example ksh?

Thanks, fixed.

> What happens (and what is the version) if this is run not inside a SVN/GIT
> repository? For example one can run 'make dist', take the tarball, unpack it
> somewhere else and build the stuff again.

The svn revisions are empty. E.g., in the case of an SVN/git svn checkout:
austin@austin2:~/src/valgrind$ ./vg-in-place -v --version
valgrind-3.12.0.SVN-16106M-vex-3283
austin@austin2:~/src/valgrind$ ./vg-in-place --version
valgrind-3.12.0.SVN

and not:
austin@austin2:~/src/valgrind$ ./vg-in-place -v --version
valgrind-3.12.0.SVN--vex-
austin@austin2:~/src/valgrind$ ./vg-in-place --version
valgrind-3.12.0.SVN

> Also this construct:
> "if test -d .svn; then svnversion -n .; elif test -d .git; then git svn info
> | \
>  grep ^Revision | cut -d ' ' -f2 | tr -d '\n'; fi"
> can be reused, for example through a Makefile variable:
> 
> EXTRACT_SVN_VERSION = "<above>"
> vgversion.h:
>     ...
>     ... $(shell $(EXTRACT_SVN_VERSION)) ...

Thanks. I had trouble getting that working for a while. The variable contents should NOT be quoted, then it works :)

(In reply to austinenglish@gmail.com from comment #10)
> (In reply to Ivo Raisr from comment #7)

> > What happens (and what is the version) if this is run not inside a SVN/GIT
> > repository? For example one can run 'make dist', take the tarball, unpack it
> > somewhere else and build the stuff again.
> 
> The svn revisions are empty. E.g., in the case of an SVN/git svn checkout:
> austin@austin2:~/src/valgrind$ ./vg-in-place -v --version
> valgrind-3.12.0.SVN-16106M-vex-3283
> austin@austin2:~/src/valgrind$ ./vg-in-place --version
> valgrind-3.12.0.SVN
> 
> and not:
> austin@austin2:~/src/valgrind$ ./vg-in-place -v --version
> valgrind-3.12.0.SVN--vex-
> austin@austin2:~/src/valgrind$ ./vg-in-place --version
> valgrind-3.12.0.SVN

I known close to 0 about the make dist and how the tarball is done,
but still giving my 2 cents:

As long as we run 'make dist' in an svn (or git svn), it should
either build or update vgversion.h
so that the distr tarball contains a vgversion.h

then if make is done in an unpacked place (without being an SVN directory),
then vgversion.h should not be modified.

In other words, if we have a command such as:
   create_or_update_vgversion_h
which:
   if "inside svn or git svn, and vgversion.h does not exist or differs 
           from current svn version,
   then  (re-)create vgversion.h from git svn/svn
   else if vgversion.h exists 
        do nothing  // this must be the vgversion.h created when the tarball was done
                    // or source files have been copied somewhere else so just keep
                    // the version which is present
   else
        create a vgversion.h with empty versions // copied source files; no svn, so cannot
                                                 // do better


Then we need to call create_or_update_vgversion_h as part of:
     make dist
     make
     ??? maybe some other targets ?

to ensure vgversion.h always contain (as much as possible) the svn version
it originates from.

How commonly are tarballs created/used for random SVN revisions rather than releases?

To be frank, I'm not very experienced with makefiles, and the current changes have already been a pain. I don't particularly feel like revamping it a 6th time for a corner case that doesn't currently work to begin with.

(In reply to Austin English from comment #12)
> How commonly are tarballs created/used for random SVN revisions rather than
> releases?
> 
> To be frank, I'm not very experienced with makefiles, and the current
> changes have already been a pain. I don't particularly feel like revamping
> it a 6th time for a corner case that doesn't currently work to begin with.

It seems to be currently broken, for that matter:
$ make dist
...
export XML_CATALOG_FILES=/etc/xml/catalog && \
mkdir -p ../docs/print && \
mkdir -p ../docs/print/images && \
cp ../docs/images/*.png ../docs/print/images && \
xsltproc --nonet --xinclude -o ../docs/print/index.fo ../docs/lib/vg-fo.xsl ../docs/xml/index.xml && \
(cd ../docs/print && \
         ( pdfxmltex index.fo && \
   pdfxmltex index.fo && \
   pdfxmltex index.fo ) &> print.log < /dev/null && \
 echo "Generating PS file: ../docs/print/index.ps ..." && \
 pdftops index.pdf && \
 rm -f *.log *.aux *.fo *.out)
Making portrait pages on USletter paper (8.5inx11in)
Makefile:642: recipe for target 'print-docs' failed
make[3]: *** [print-docs] Error 127
make[3]: Leaving directory '/tmp/valgrind/docs'
Makefile:450: recipe for target 'distdir' failed
make[2]: *** [distdir] Error 2
make[2]: Leaving directory '/tmp/valgrind/docs'
Makefile:930: recipe for target 'distdir' failed
make[1]: *** [distdir] Error 1
make[1]: Leaving directory '/tmp/valgrind'
Makefile:1028: recipe for target 'dist' failed
make: *** [dist] Error 2

(In reply to Austin English from comment #13)
> (In reply to Austin English from comment #12)
> > How commonly are tarballs created/used for random SVN revisions rather than
> > releases?
> > 
> > To be frank, I'm not very experienced with makefiles, and the current
> > changes have already been a pain. I don't particularly feel like revamping
> > it a 6th time for a corner case that doesn't currently work to begin with.
> 
> It seems to be currently broken, for that matter:
> $ make dist
> ...
Yes, I understand that this change has already implied several reworks,
following several comments, and that this does not converge very
quickly. Sorry for that.

For what concerns the broken make dist: this might be something in your
setup, related to the documentation building procedure, which is
not straightforward: many packages are needed to build the doc.
I just tried, and make dist works for me.

Otherwise, to my knowledge, make dist is run for a set of SVN
versions for the (often several) release candidates
and for the final release. So, as far as I understand, we need to? should? 
produce a vgversion.h as part of the make dist, as this ensures the
build from a dist tarball is similar to a 'non dist' build.
IMO, identifying tarballs precisely is  as (or more?) important than
identifying 'in a corner' SVN versions, as release candidates and dist
have more chances to be replicated here and there.

I will take a look at this whenever I have some time, and see if I can help

I agree with Philippe that vgversion.h should be a part of the tarball created by 'make dist'. This way it is very intuitive and reasonable.

As regards the errors during 'make dist' build you encountered, they are most probably connected with missing packages for documentation build. See comments in docs/README which lists some distributions and the required packages.
I will be happy to test 'make dist' for you on Solaris which is working as well.

Created attachment 102468 [details]
try 6 : modified to have vgversion.h also handled as part of make dist

Changed compared to try5:
* vgversion.h make or update logic moved
      from Makefile.am to auxprogs/make_or_upd_vgversion_h
* added dist-hook to copy vgversion.h

I have tested this (on linux debian 8 amd64) in a 'in place' build,
and  in a build from a make dist. I have checked that the dist contains
vgversion.h. I have also tested that outside svn, vgversion.h is created
with unknown svn versions, but that it is not replaced if it already exists.

Would be nice to do some more tests before commit
(e.g. on other OS solaris, macos)

(In reply to Philippe Waroquiers from comment #16)
> Created attachment 102468 [details]
> try 6 : modified to have vgversion.h also handled as part of make dist
> 
> Changed compared to try5:
> * vgversion.h make or update logic moved
>       from Makefile.am to auxprogs/make_or_upd_vgversion_h
> * added dist-hook to copy vgversion.h
> 
> I have tested this (on linux debian 8 amd64) in a 'in place' build,
> and  in a build from a make dist. I have checked that the dist contains
> vgversion.h. I have also tested that outside svn, vgversion.h is created
> with unknown svn versions, but that it is not replaced if it already exists.
> 
> Would be nice to do some more tests before commit
> (e.g. on other OS solaris, macos)

Thanks for working on that Philippe!

You've got a syntax error here:

+    elif [ -d .git/svn ]
+        git svn info $1 | grep '^Revision' | cut -d ' ' -f2 | tr -d '\n'
+    then
+        echo "unknown"
+    fi    
+}

should be elif ; then ; else

Created attachment 102479 [details]
try7 : fix syntax error in the git extract version

bash -n is not reporting any syntax error on this :(

(my kingdom for a shell compiler)

Although it's not a compiler, the following tool is very convenient for checking the syntax of shell scripts: https://github.com/koalaman/shellcheck. I build and install it as follows:
git clone https://github.com/koalaman/shellcheck
cd shellcheck
cabal update
cabal install cabal-install
cabal install

Patch #6 seems to be working well. I tested it on Solaris, inside SVN tree, outside and after 'make dist'.

In addition to the last comment by Austin, please change also:

auxprogs/make_or_upd_vgversion_h:
-   when using command lines options:  -v --version 
+   when using command line options:  -v --version 

and make sure that auxprogs/make_or_upd_vgversion_h is marked as executable in SVN repository.

There is also online tool at: http://www.shellcheck.net/
However it did not catch the problem with mismatched then...

(In reply to Ivo Raisr from comment #20)
> Patch #6 seems to be working well. I tested it on Solaris, inside SVN tree,
> outside and after 'make dist'.
> 
> In addition to the last comment by Austin, please change also:
> 
> auxprogs/make_or_upd_vgversion_h:
> -   when using command lines options:  -v --version 
> +   when using command line options:  -v --version 
> 
> and make sure that auxprogs/make_or_upd_vgversion_h is marked as executable
> in SVN repository.

Yeah, it fails for me out of the box, with:
echo "# This is a generated file, composed of the following suppression rules:" > default.supp
auxprogs/make_or_upd_vgversion_h
make: execvp: auxprogs/make_or_upd_vgversion_h: Permission denied
Makefile:1374: recipe for target 'vgversion.h' failed
make: *** [vgversion.h] Error 127
make: *** Waiting for unfinished jobs....
echo "# " exp-sgcheck.supp xfree-3.supp xfree-4.supp glibc-2.X-drd.supp glibc-2.34567-NPTL-helgrind.supp glibc-2.X.supp  >> default.supp
cat exp-sgcheck.supp xfree-3.supp xfree-4.supp glibc-2.X-drd.supp glibc-2.34567-NPTL-helgrind.supp glibc-2.X.supp >> default.supp

chmod +x auxprogs/make_or_upd_vgversion_h gets past that problem.

Next, it fails for VEX, with git svn:
valgrind-3.12.0.SVN-16107-vex-

+        git svn info $1 | grep '^Revision' | cut -d ' ' -f2 | tr -d '\n'

won't work the way you expect. It needs to be in the VEX directory for this to work:
austin@austin2:~/src/valgrind-git$ git svn info | grep Revision
Revision: 16107

austin@austin2:~/src/valgrind-git$ git svn info VEX | grep Revision
svn: 'VEX' is not under version control

austin@austin2:~/src/valgrind-git$ cd VEX && git svn info | grep Revision
Revision: 3276

FYI, I can also text on OSX (once these issues are sorted out ;) ).

I have done:
svn propset svn:executable ON auxprogs/make_or_upd_vgversion_h

used shellcheck, which told me to put $1 between quotes, which I did
and fixed the syntax error.

So, as I understand, this was checked on linux with svn (me)
was checked by Austin with git
was checked by Ivo on Solaris (git or svn ?)
Would be nice to have a Macos check , but if no bad news received in the
coming days or so, I will commit

Created attachment 102509 [details]
try 8 : some additional fixes.

Created attachment 102510 [details]
yet another small fix following irc discussion to make it work with git

Created attachment 102512 [details]
try 10, extra sanity checks

try 9 works for me, but I put some extra sanity checks in. In theory, someone could have valgrind from svn, and VEX in git svn, or vice versa, and try 9 would break with that.

I also added '|| exit 1' to the cd command, as a sanity check / to satisfy shellcheck.

(In reply to Austin English from comment #26)
> Created attachment 102512 [details]
> try 10, extra sanity checks
> 
> try 9 works for me, but I put some extra sanity checks in. In theory,
> someone could have valgrind from svn, and VEX in git svn, or vice versa, and
> try 9 would break with that.
> 
> I also added '|| exit 1' to the cd command, as a sanity check / to satisfy
> shellcheck.

Note: version differences aren't a bug, just checked out at different times and I was too lazy to sync them :)

Gentoo/AMD64:
linux-svn: valgrind-3.13.0.SVN-16159M-vex-3285
linux-git-svn: valgrind-3.12.0.SVN-16107-vex-3276

OS X Sierra:
mac-svn: valgrind-3.13.0.SVN-16159M-vex-3285
mac-git-svn: valgrind-3.12.0.SVN-16107-vex-3276

(In reply to Austin English from comment #27)
> (In reply to Austin English from comment #26)
> > Created attachment 102512 [details]
> > try 10, extra sanity checks
> > 
> > try 9 works for me, but I put some extra sanity checks in. In theory,
> > someone could have valgrind from svn, and VEX in git svn, or vice versa, and
> > try 9 would break with that.
> > 
> > I also added '|| exit 1' to the cd command, as a sanity check / to satisfy
> > shellcheck.
> 
> Note: version differences aren't a bug, just checked out at different times
> and I was too lazy to sync them :)
> 
> Gentoo/AMD64:
> linux-svn: valgrind-3.13.0.SVN-16159M-vex-3285
> linux-git-svn: valgrind-3.12.0.SVN-16107-vex-3276
> 
> OS X Sierra:
> mac-svn: valgrind-3.13.0.SVN-16159M-vex-3285
> mac-git-svn: valgrind-3.12.0.SVN-16107-vex-3276

Thanks for the test.
Strange that the git-svn version does not show a M(odified) marker.

(In reply to Philippe Waroquiers from comment #28)
> Strange that the git-svn version does not show a M(odified) marker.

Yeah, I noticed that too. I'll try to remember to file a bug upstream.

Committed in revision 16164