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

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 21, 2009, 00:41 UTC
Message-ID
<alpine.DEB.1.00.0901210130030.19014@racer>
In-Reply-To
<20090121001219.GA18169@coredump.intra.peff.net>
Hi,
On Tue, 20 Jan 2009, Jeff King wrote:
Show 13 quoted lines
> On Tue, Jan 20, 2009 at 04:04:28PM +0100, Johannes Schindelin wrote:
> 
> > +else
> > +	# override all git executables in PATH and TEST_DIRECTORY/..
> > +	GIT_VALGRIND=$TEST_DIRECTORY/valgrind
> > +	mkdir -p "$GIT_VALGRIND"
> 
> Isn't this mkdir unnecessary, since it is actually part of the
> repository (i.e., there is a gitignore there already).
> 
> However, I think it makes more sense to put the symlink cruft into
> "$GIT_VALGRIND/bin". That way you can clean up the cruft very easily. In
> which case you do need to "mkdir" that directory.

Hmm. I actually liked the hierarchy to be shallow, but I could be convinced...

Show 7 quoted lines
> > +	OLDIFS=$IFS
> > +	IFS=:
> > +	for path in $PATH:$TEST_DIRECTORY/..
> > +	do
> > +		ls "$TEST_DIRECTORY"/../git "$path"/git-* 2> /dev/null |
> 
> Why aren't these both "$path"/ ?
Yeah.  Makes it more readable, doesn't it?
Show 7 quoted lines
> But more importantly, do we really need to bother overriding the whole 
> $PATH? In theory, we aren't calling anything git-* that isn't in 
> "$TEST_DIRECTORY/..". And while it might be nice to catch it if we do, 
> it seems like detecting that is totally orthogonal to running valgrind, 
> and we get different behavior from valgrind versus not. And I think the 
> two should be as similar as possible (with the obvious except of 
> actually, you know, running valgrind).
Actually, the two _are_ orthogonal from the technical viewpoint.

But with the infrastructure we have in place, it was already very easy to make sure that calls to a Git program we no longer ship are caught.

I vividly remember such a bug costing me 3 hours of my life, and a few hairs.

So I think "as it's already _that_ easy, we should catch them bugs, too".
Needs some documentation though, I agree.
Show 14 quoted lines
> > +			base=$(basename "$file")
> > +			test ! -h "$GIT_VALGRIND"/"$base" || continue
> > +
> > +			if test "#!" = "$(head -c 2 < "$file")"
> > +			then
> > +				# do not override scripts
> > +				ln -s ../../"$base" "$GIT_VALGRIND"/"$base"
> > +			else
> > +				ln -s valgrind.sh "$GIT_VALGRIND"/"$base"
> > +			fi
> 
> It would be nice to actually detect errors. But you have to
> differentiate between EEXIST and other errors, which is a pain. And you
> can't use "ln -sf" because it isn't atomic.

I really would not care all that much about that. 'GIT_TEST_OPTS==--valgrind make test' should be run by experts. And even if it is a dummy driving the test, the next "make" call should take care of that.

> Copying would solve that (provided you copied to a tempfile and did
> an atomic rename). Or writing this snippet as a C helper.

Nah, that is really too much work for such a rare thing. Think about it. The symlinks are set up once. And even if you do that with -j50, there is hardly a chance that two processes conflict with each other, and even if they do, they do the same thing.

No, what I really want to fix is a script being replaced by a binary.
Show 21 quoted lines
> > --- /dev/null
> > +++ b/t/valgrind/valgrind.sh
> > @@ -0,0 +1,12 @@
> > +#!/bin/sh
> > +
> > +base=$(basename "$0")
> > +
> > +exec valgrind -q --error-exitcode=126 \
> > +	--leak-check=no \
> > +	--suppressions="$GIT_VALGRIND/default.supp" \
> > +	--gen-suppressions=all \
> > +	--log-fd=4 \
> > +	--input-fd=4 \
> > +	$GIT_VALGRIND_OPTIONS \
> > +	"$GIT_VALGRIND"/../../"$base" "$@"
> 
> Hm. My version had to do some magic with the GIT_EXEC_PATH, but I think
> that is because I didn't set GIT_EXEC_PATH in the first place. If yours
> works (and I haven't really tested it -- I remember it being a real pain
> in the butt to make sure valgrind was getting called from every code
> path), then I like your approach much better.
I set GIT_EXEC_PATH... to $GIT_VALGRIND.

Ciao, Dscho

Previous: Jeff KingNext: Johannes Schindelin
Message 12 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.