git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2 v2] Add valgrind support in test scripts

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 21, 2009, 20:49 UTC
Message-ID
<alpine.DEB.1.00.0901212137130.3586@pacific.mpi-cbg.de>
In-Reply-To
<20090121190201.GA21686@coredump.intra.peff.net>
Hi,
On Wed, 21 Jan 2009, Jeff King wrote:
Show 16 quoted lines
> On Wed, Jan 21, 2009 at 02:10:17AM +0100, Johannes Schindelin wrote:
> 
> > 	- symbolic links are inspected for correct targets now, and if they
> > 	  point somewhere else than expected, they are removed (this can
> > 	  error out if the file could not be removed) and recreated.
> 
> Now you _do_ have a race on this, and triggering it will cause you to
> run a random version of git from your PATH, not using valgrind (instead
> of running the version from the repo using valgrind). Something like:
> 
>   A: execvp("git-foo")
>   B: oops, "git-foo" is out of date
>   B: rm $GIT_VALGRIND/git-foo
>   A: look for $GIT_VALGRIND/git-foo; not there
>   A: look for $PATH[1]/git-foo; ok, there it is
>   B: ln -s ../../git-valgrind $GIT_VALGRIND/git-foo

Except that A had to check the link first, and it was out-of-date already -- except if you changed a script into a builtin _and_ run make while a valgrinded test is called _and_ you're unlucky.

Show 9 quoted lines
> > +--valgrind::
> > +	Execute all Git binaries with valgrind and stop on errors (the
> > +	exit code will be 126).
> 
> It doesn't necessarily stop: it just causes the command to fail, which
> causes the test to fail. Which _will_ stop if you have "-i".
> 
> Also, you might want to mention that valgrind errors go to stderr, so
> using "-v" is helpful.
Okay.
Show 8 quoted lines
> > +	# override all git executables in PATH and TEST_DIRECTORY/..
> > +	GIT_VALGRIND=$TEST_DIRECTORY/valgrind/bin
> 
> I think you should leave GIT_VALGRIND pointing to the main valgrind
> directory. That way it is more convenient for people using
> GIT_VALGRIND_OPTIONS to make use of GIT_VALGRIND without having to ".."
> everything (for example, they may want to pick and choose suppressions
> to load for their platform).
Okay.
Show 6 quoted lines
> > +			case "$base" in
> > +			*.sh|*.perl)
> > +				symlink_target=../unprocessed-script
> > +			esac
> 
> AFAIK, this triggers an error if I try to call "git-foo.perl" directly.
Yep.
> What does this have to do with valgrind?
Nothing, except that the infrastructure is there now.
> Why does this error checking happen when I run --valgrind, but _not_ 
> otherwise?

Because we can only check for that kind of mistake in our scripts (which the author would not realize is a mistake when running on a system where GIT_SHELL=/bin/sh) when we redirect GIT_EXEC_PATH.

So basically, it would take a tremendous effort otherwise, but here, it is just easy.

Show 9 quoted lines
> And yes, I know the answer is "because it's easy to do here, since
> --valgrind is munging the PATH anyway". But my point is that that is an
> _implementation_ detail, and the external behavior to a user is
> nonsensical.
> 
> The fact that there are other uses for munging the PATH than valgrind
> implies to me that we should _always_ be munging the PATH like this to
> catch these sorts of errors. And then "--valgrind" can just change the
> way we munge.
Hmm.  Maybe.
Show 12 quoted lines
> > +			# create the link, or replace it if it is out of date
> > +			if test ! -h "$GIT_VALGRIND"/"$base" ||
> > +			    test "$symlink_target" != \
> > +					"$(readlink "$GIT_VALGRIND"/"$base")"
> > +			then
> 
> readlink is not portable; it's part of GNU coreutils. Right now valgrind
> basically only runs on Linux, which I think generally means that
> readlink will be available (though I have no idea if there are
> distributions that vary in this). However, there is an experimental
> valgrind port to FreeBSD and NetBSD, which are unlikely to have
> readlink.

As I mentioned earlier: let's bridge this bridge when we face it (probably it involves making a test-readlink).

Or are you insisting that the patch should be reworked _now_ so that GIT_EXEC_PATH _always_ points somewhere else?

I hope not, because then you break Windows.

Ciao, Dscho

Previous: Jeff KingNext: Jeff King
Message 18 of 73 in “What's cooking in git.git (Jan 2009, #04; Mon, 19)”
  1. Junio C HamanoJan 19, 2009
  2. Kjetil BarvikJan 19, 2009
  3. Johannes SchindelinJan 19, 2009
  4. Johannes SchindelinJan 19, 2009
  5. Jeff KingJan 20, 2009
  6. valgrind patches, was Re: What's cooking in git.git (Jan 2009, #04; Mon, 19)Johannes Schindelin, Jan 20, 2009
  7. Jeff KingJan 20, 2009
  8. Johannes SchindelinJan 20, 2009
  9. 1/2 Add valgrind support in test scriptsJohannes Schindelin, Jan 20, 2009
  10. 2/2 valgrind: ignore ldso errorsJohannes Schindelin, Jan 20, 2009
  11. Jeff KingJan 21, 2009
  12. Johannes SchindelinJan 21, 2009
  13. 1/2 Add valgrind support in test scriptsJohannes Schindelin, Jan 21, 2009
  14. 1/2 Add valgrind support in test scriptsJohannes Schindelin, Jan 21, 2009
  15. Junio C HamanoJan 21, 2009
  16. Johannes SchindelinJan 21, 2009
  17. Jeff KingJan 21, 2009
  18. Johannes SchindelinJan 21, 2009
  19. Jeff KingJan 21, 2009
  20. Johannes SchindelinJan 21, 2009
  21. 0/3 Valgrind supportJohannes Schindelin, Jan 25, 2009
  22. 1/3 Add valgrind support in test scriptsJohannes Schindelin, Jan 25, 2009
  23. Jeff KingJan 25, 2009
  24. Johannes SchindelinJan 25, 2009
  25. Jeff KingJan 25, 2009
  26. 2/3 valgrind: ignore ldso and more libz errorsJohannes Schindelin, Jan 25, 2009
  27. Jeff KingJan 25, 2009
  28. Johannes SchindelinJan 26, 2009
  29. Jeff KingJan 26, 2009
  30. 3/3 Valgrind support: check for more than just programming errorsJohannes Schindelin, Jan 25, 2009
  31. Jeff KingJan 25, 2009
  32. Johannes SchindelinJan 26, 2009
  33. valgrind tests: be super-super paranoid when creating symlinksJohannes Schindelin, Jan 21, 2009
  34. Jeff KingJan 20, 2009
  35. Johannes SchindelinJan 21, 2009
  36. Jeff KingJan 21, 2009
  37. Johannes SchindelinJan 21, 2009
  38. Jeff KingJan 21, 2009
  39. Johannes SchindelinJan 21, 2009
  40. 2/2 valgrind: ignore ldso errorsJohannes Schindelin, Jan 21, 2009
  41. Jeff KingJan 21, 2009
  42. Johannes SchindelinJan 21, 2009
  43. Jeff KingJan 21, 2009
  44. Johannes SchindelinJan 21, 2009
  45. Jeff KingJan 21, 2009
  46. Junio C HamanoJan 22, 2009
  47. Jeff KingJan 22, 2009
  48. Johannes SchindelinJan 22, 2009
  49. Jeff KingJan 22, 2009
  50. Valgrind updatesJohannes Schindelin, Jan 27, 2009
  51. Linus TorvaldsJan 27, 2009
  52. Johannes SchindelinJan 27, 2009
  53. Johannes SchindelinJan 27, 2009
  54. Mark BrownJan 27, 2009
  55. Johannes SchindelinJan 27, 2009
  56. Linus TorvaldsJan 27, 2009
  57. Johannes SchindelinJan 27, 2009
  58. Linus TorvaldsJan 29, 2009
  59. Johannes SchindelinJan 29, 2009
  60. Mark AdlerJan 28, 2009
  61. Johannes SchindelinJan 28, 2009
  62. Mark AdlerJan 29, 2009
  63. Johannes SchindelinJan 29, 2009
  64. Johannes SchindelinJan 29, 2009
  65. Jeff KingJan 27, 2009
  66. Johannes SchindelinJan 27, 2009
  67. Jeff KingJan 20, 2009
  68. Jeff KingJan 20, 2009
  69. Junio C HamanoJan 20, 2009
  70. Johannes SixtJan 20, 2009
  71. Jeff KingJan 20, 2009
  72. Boyd Stephen Smith Jr.Jan 20, 2009
  73. Thomas RastJan 20, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.