[PATCH] tests: handle NO_PYTHON setting

Without this, test-lib checks that the git_remote_helpers
directory has been built. However, if we are building
without python, we will not have done anything at all in
that directory, and test-lib's sanity check will fail.

Signed-off-by: Jeff King <redacted>
---
On top of sr/vcs-helper.

This feels a little funny for NO_PYTHON to mean "no remote helpers at
all". But that is the way the Makefile is set up, since we seem to have
only python helpers.

 Makefile      |    1 +
 t/test-lib.sh |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 42744a4..443565e 100644
--- a/Makefile
+++ b/Makefile

@@ -1743,6 +1743,7 @@ GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
 	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
+	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a40520..ca0839c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -638,7 +638,7 @@ test -d ../templates/blt || {
 	error "You haven't built things yet, have you?"
 }
 
-if test -z "$GIT_TEST_INSTALLED"
+if test -z "$GIT_TEST_INSTALLED" && test -z "$NO_PYTHON"
 then
 	GITPYTHONLIB="$(pwd)/../git_remote_helpers/build/lib"
 	export GITPYTHONLIB

-- 
1.6.6.rc0.327.gd49b

Heya,

On Mon, Nov 30, 2009 at 08:52, Jeff King [off-list ref] wrote:

This feels a little funny for NO_PYTHON to mean "no remote helpers at
all". But that is the way the Makefile is set up, since we seem to have
only python helpers.

I don't understand what you mean? Do you mean NO_PYTHON implies "no
remote helpers at all", or "not having any remote helpers" implies
NO_PYTHON? Either way, I'm not sure how to set it up differently, not
having that much Makefile foo myself, so maybe Johan and Daniel could
comment?

-- 
Cheers,

Sverre Rabbelier

On Mon, Nov 30, 2009 at 08:55:51AM +0100, Sverre Rabbelier wrote:

quoted

This feels a little funny for NO_PYTHON to mean "no remote helpers at
all". But that is the way the Makefile is set up, since we seem to have
only python helpers.

I don't understand what you mean? Do you mean NO_PYTHON implies "no
remote helpers at all", or "not having any remote helpers" implies
NO_PYTHON? Either way, I'm not sure how to set it up differently, not
having that much Makefile foo myself, so maybe Johan and Daniel could
comment?

I mean, I would think that the "git_remote_helpers" directory contained
remote helpers of all sorts, not just the python ones. Right now we
_only_ have python ones. So checking for NO_PYTHON in test-lib.sh before
looking at git_remote_helpers makes sense.  But I am concerned that
assumption will be broken silently in the future if non-python helpers
are added to git_remote_helpers.

It is probably not worth caring about too much, though.

-Peff

Heya,

On Mon, Nov 30, 2009 at 08:59, Jeff King [off-list ref] wrote:

I mean, I would think that the "git_remote_helpers" directory contained
remote helpers of all sorts, not just the python ones.

I don't think that's true, git.git currently does not have such a
structure (everything is just dumped in the root directory). The only
reason git_remote_helpers exists is to make it easier to create a
python egg out of it and install that. At least, that's what I think
is going on, Johan and Daniel might have comments to the contrary.

-- 
Cheers,

Sverre Rabbelier

On Mon, Nov 30, 2009 at 09:04:25AM +0100, Sverre Rabbelier wrote:

On Mon, Nov 30, 2009 at 08:59, Jeff King [off-list ref] wrote:

quoted

I mean, I would think that the "git_remote_helpers" directory contained
remote helpers of all sorts, not just the python ones.

I don't think that's true, git.git currently does not have such a
structure (everything is just dumped in the root directory). The only
reason git_remote_helpers exists is to make it easier to create a
python egg out of it and install that. At least, that's what I think
is going on, Johan and Daniel might have comments to the contrary.

OK. It is just my confusion, then. Don't worry about it.

-Peff

Jeff King wrote:

Shouldn't this section be moved down below the sourcing of ../GIT-BUILD-OPTIONS
on line 656 so that the value of NO_PYTHON will be available when running the
test scripts directly?

-brandon

ps. There's something eerily familiar about this patch.

On Mon, Nov 30, 2009 at 12:07:40PM -0600, Brandon Casey wrote:

Shouldn't this section be moved down below the sourcing of ../GIT-BUILD-OPTIONS
on line 656 so that the value of NO_PYTHON will be available when running the
test scripts directly?

Oops, good catch. I stupidly tested with "make NO_PYTHON=1 test" instead
of actually checking that GIT-BUILD-OPTIONS was propagating it
correctly.

ps. There's something eerily familiar about this patch.

Hmmm. Yes, I didn't search before writing it, but you probably mean:

  http://article.gmane.org/gmane.comp.version-control.git/127172

But that is missing the NO-PYTHON bit in GIT-BUILD-OPTIONS (did you
forget it there, or was it part of some other patch that also didn't get
applied?).

Also, I am tempted to move the GIT-BUILD-OPTIONS invocation _up_. It
is about reading config and should probably come before we start doing
_anything_.

So maybe this instead:

-- >8 --
Subject: [PATCH] tests: handle NO_PYTHON setting

Without this, test-lib checks that the git_remote_helpers
directory has been built. However, if we are building
without python, we will not have done anything at all in
that directory, and test-lib's sanity check will fail.

We bump the inclusion of GIT-BUILD-OPTIONS further up in
test-lib; it contains configuration, and as such should be
read before we do any checks (and in this particular case,
we need its value to do our check properly).

Signed-off-by: Jeff King <redacted>
---
I moved the BUILD-OPTIONS thing to just above the beginning of the
"have you built anything" checks, but after all of the function
definitions. But perhaps it should simply go at the very top of the
script. After all, in the case of "make NO_PYTHON=1 test", those
variables will already be defined at the very beginning of the script.

 Makefile      |    1 +
 t/test-lib.sh |    6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 42744a4..443565e 100644
--- a/Makefile
+++ b/Makefile

@@ -1743,6 +1743,7 @@ GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
 	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
+	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a40520..2d523fe 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -632,13 +632,15 @@ GIT_CONFIG_NOSYSTEM=1
 GIT_CONFIG_NOGLOBAL=1
 export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
 
+. ../GIT-BUILD-OPTIONS
+
 GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
 export GITPERLLIB
 test -d ../templates/blt || {
 	error "You haven't built things yet, have you?"
 }
 
-if test -z "$GIT_TEST_INSTALLED"
+if test -z "$GIT_TEST_INSTALLED" && test -z "$NO_PYTHON"
 then
 	GITPYTHONLIB="$(pwd)/../git_remote_helpers/build/lib"
 	export GITPYTHONLIB

@@ -653,8 +655,6 @@ if ! test -x ../test-chmtime; then
 	exit 1
 fi
 
-. ../GIT-BUILD-OPTIONS
-
 # Test repository
 test="trash directory.$(basename "$0" .sh)"
 test -n "$root" && test="$root/$test"

-- 
1.6.6.rc0.327.gd49b

Jeff King wrote:

On Mon, Nov 30, 2009 at 12:07:40PM -0600, Brandon Casey wrote:

quoted

ps. There's something eerily familiar about this patch.

Hmmm. Yes, I didn't search before writing it, but you probably mean:

  http://article.gmane.org/gmane.comp.version-control.git/127172

:) yeah, that was it, nbd.

But that is missing the NO-PYTHON bit in GIT-BUILD-OPTIONS (did you
forget it there, or was it part of some other patch that also didn't get
applied?).

It was 1/2 of that series.

Also, I am tempted to move the GIT-BUILD-OPTIONS invocation _up_. It
is about reading config and should probably come before we start doing
_anything_.

So maybe this instead:

<snip the patch>

Looks fine to me.

No strong opinion on whether the BUILD-OPTIONS thing should be
at the beginning of the script, or in the place where you placed
it.

-brandon

On Monday 30 November 2009, Brandon Casey wrote:

Jeff King wrote:

quoted

On Mon, Nov 30, 2009 at 12:07:40PM -0600, Brandon Casey wrote:

quoted

ps. There's something eerily familiar about this patch.

Hmmm. Yes, I didn't search before writing it, but you probably mean:

  http://article.gmane.org/gmane.comp.version-control.git/127172

:) yeah, that was it, nbd.

Oops. I got Brandon's patches in my local tree, but I never got around to 
resend the series until Sverre picked up and refactored it. Sorry for the 
screwup.

quoted

But that is missing the NO-PYTHON bit in GIT-BUILD-OPTIONS (did you
forget it there, or was it part of some other patch that also didn't
get applied?).

It was 1/2 of that series.

Indeed.

quoted

Also, I am tempted to move the GIT-BUILD-OPTIONS invocation _up_. It
is about reading config and should probably come before we start doing
_anything_.

So maybe this instead:

<snip the patch>

Looks fine to me.

As with Brandon's original patch, this is of course

Acked-by: Johan Herland <redacted>

No strong opinion on whether the BUILD-OPTIONS thing should be
at the beginning of the script, or in the place where you placed
it.

Me neither.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net