On Sat, Nov 28, 2009 at 11:44:39AM -0800, Junio C Hamano wrote:
> - Patch #1 and #2 are good and are independent from the later patches, as
> without them running tests with GIT_TEST_INSTALLED would not work.
>
> By the way, 6720721 (test-lib.sh: Allow running the test suite against
> installed git, 2009-03-16) failed to document the feature in t/README.
> Could you please fix this while you are at it?
Sure, I'll include another patch for this when I re-roll the
series. It will probably mention something about still needing
a build so that it can access the test-* support executables.
> - It certainly is _possible_ to add $(pwd)/test-bin to $PATH instead of
> the established practice of using GIT_EXEC_PATH for every day/permanent
> use without installation, but I doubt we should _encourage_ it for a
> few reasons:
>
> . The set-up will force one extra exec due to the wrapper; this is good
> for the purpose of running tests, but unnecessary for a set-up for
> every day/permanent use by people, compared with the already working
> approach. The user needs to change an environment variable _anyway_
> (either GIT_EXEC_PATH with the traditional approach, or PATH with
> your patch).
>
> . The new component to be added to $PATH shouldn't be named "test-bin/"
> if it is meant for every day/permanent use.
>
> . Advertising this forces the Makefile build test-bin/ contents from
> "all" target. I think test-bin/ should only depend on "test" (iow,
> after "make all && make install" there shouldn't have to be "test-bin"
> directory.
>
> I would rather treat it an unintended side-effect that you can add that
> directory to the $PATH. It is designed to work in such an environment
> (otherwise the tests won't exercise the version of git they are meant
> to test), but I do not think it is _meant_ to be _used_ by end users
> that way. And an unintended side-effect does not have to be mentioned
> in INSTALL (especially with the directory name with "test" in it).
I personally like the idea of being able to use an uninstalled
build without touching environment variables at all. Just specify
the full path to the the version you want to run on the
command line, as in: ~/SANDBOX/test-bin/git WHATEVER
Especially handy for trying "ssh MACHINE /PATH/SANDBOX/...".
FYI: There are already a number of test suite support executables
built by "make all" (test-*). It might be hard to eliminate them
from "all" without risking stale executables. As Jeff
King <peff@xxxxxxxx> pointed out in a separate email, some people
(including me) often don't use the top-level "make test" target
to run tests.
I'm still thinking about this. I've noted some possible changes
to the patch series below, some of which are mutually exclusive.
Any opinions?
Options geared towards isolating/hiding test-bin:
1. Scrap the part of the patch that modifies INSTALL.
2. Perhaps use hardlinks, symlinks, and/or copies within test-bin,
instead of wrapper scripts, to eliminate the extra exec. Since
test-lib.sh already sets up necessary environment variables,
they don't strictly need to be set in the wrappers as
well. On the other hand, hardlinks and copies are potentially
vulnerable to stale executable issues, and symlinks typically
don't work on Windows.
3. Scrap pre-built test-bin completely, and switch to a solution
that more closely resembles the valgrind option (have test-lib.sh
build the directory). This can't use the same makefile variables
to define the contents of the directory, though.
Options geared towards making something like test-bin an official way
to run an uninstalled build:
4. Rename test-bin. Perhaps "bin-wrappers", "bin-dashless",
"bin-install", "bin", or "bindir". Any preferences?
5. The current patch doesn't quite handle the simple
"~/SANDBOX/test-bin/gitSOMETHING WHATEVER" idiom perfectly
if the executable (gitSOMETHING) tries to run additional
git commands without adjusting the PATH first. I could enhance
the wrapper to prefix test-bin onto the PATH just in case it
isn't there already.
Other cleanup options:
6. There is a stale script issue if someone does something like:
make
cp -a . /some/other/path
cd /some/other/path
[optional modifications, without a "make clean"]
make
[run tests; uses wrong executables...]
Including GIT-CFLAGS as a makefile dependency for the
wrappers was intended to address this, but looking
closer, I don't think it works. Perhaps I should
include $(shell pwd) in GIT-CFLAGS, or make a new GIT-PWD target
that works similarly to GIT-CFLAGS. Without this, a workaround
(and probably best option overall) is to do a "make clean" after
copying a sandbox.
7. Enable similar dashless environment when
GIT_TEST_INSTALLED and/or valgrind are enabled?
8. Include wrappers for other dashed-commands in test-bin, which
would always fail, in case someone runs tests with an installed
GIT_EXEC_PATH already in their PATH. This might catch a new test
using dashes in such an environment. I don't really think this
is worth it, though. Most people don't have GIT_EXEC_PATH in their
PATH, and some such person would notice any problems soon.
9. This may be outside the scope of this patch series, but perhaps
git executables could try to find argv[0] in the PATH
(if argv[0] is not absolute), and see if they can find various other
executables (GIT_EXEC_PATH) and data files (perl, templates,
etc) using paths relative to itself. This may include
manually dereferencing argv[0] if it is a symlink. GIT_EXEC_PATH
and friends still takes precedence, but only fallback on
compile-time defaults if "find relative to argv[0]" fails.
It looks like Makefile RUNTIME_PREFIX enables something like
this, but it is currently disabled by default on most platforms.
--
Matthew Ogilvie [mmogilvi_git@xxxxxxxxxxxx]
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html