threads / patch / 26234

patchgit-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

Subject: [PATCH] git-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

## tl;dr

6 messages between Jan 7, 2011 and Jan 7, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Robin H. Johnson· Jan 7, 2011, 20:48 UTC · lore

Presently, the CVS interface scripts are always built, and their test-suites run based on a binary named 'cvs' happening to return zero. If there something other than the real CVS there, bad things happened during the test-suite run.

This patch implements NO_CVS in the manner of NO_PERL, and ensures that the CVS scripts get the unimplemented variant when appropriate, as well as making sure that the tests properly declare CVS as a prerequisite (shortcut to test_done like the Perl prerequisites).

Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
X-Gentoo-Bug: 350330
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=350330
---
 Makefile                           |   37 ++++++++++++++++++++++++--------
 t/t9200-git-cvsexportcommit.sh     |    5 ++++
 t/t9400-git-cvsserver-server.sh    |    8 ++++++-
 t/t9401-git-cvsserver-crlf.sh      |   15 ++++++++----
 t/t9600-cvsimport.sh               |   41 ++++++++++++++++++++++-------------
 t/t9601-cvsimport-vendor-branch.sh |   11 +++++++++
 t/t9602-cvsimport-branches-tags.sh |   11 +++++++++
 t/t9603-cvsimport-patchsets.sh     |   11 +++++++++
 t/test-lib.sh                      |    1 +
 9 files changed, 110 insertions(+), 30 deletions(-)
Show changes to 9 files +110 −30

Makefile, t/t9200-git-cvsexportcommit.sh, t/t9400-git-cvsserver-server.sh, t/t9401-git-cvsserver-crlf.sh, t/t9600-cvsimport.sh, t/t9601-cvsimport-vendor-branch.sh, t/t9602-cvsimport-branches-tags.sh, t/t9603-cvsimport-patchsets.sh, t/test-lib.sh

diff --git a/Makefile b/Makefile
index 775ee83..158489a 100644
--- a/Makefile
+++ b/Makefile
@@ -188,6 +188,8 @@ all::
 #
 # Define NO_TCLTK if you do not want Tcl/Tk GUI.
 #
+# Define NO_CVS if you do not want any CVS interface utilities.
+#
 # The TCL_PATH variable governs the location of the Tcl interpreter
 # used to optimize git-gui for your system.  Only used if NO_TCLTK
 # is not set.  Defaults to the bare 'tclsh'.
@@ -347,6 +349,7 @@ LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =
 SCRIPT_PERL =
+SCRIPT_PERL_CVS =
 SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
@@ -384,17 +387,18 @@ SCRIPT_LIB += git-sh-setup
 SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-difftool.perl
 SCRIPT_PERL += git-archimport.perl
-SCRIPT_PERL += git-cvsexportcommit.perl
-SCRIPT_PERL += git-cvsimport.perl
-SCRIPT_PERL += git-cvsserver.perl
 SCRIPT_PERL += git-relink.perl
 SCRIPT_PERL += git-send-email.perl
 SCRIPT_PERL += git-svn.perl
 
+SCRIPT_PERL_CVS += git-cvsexportcommit.perl
+SCRIPT_PERL_CVS += git-cvsimport.perl
+SCRIPT_PERL_CVS += git-cvsserver.perl
+
 SCRIPT_PYTHON += git-remote-testgit.py
 
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
-	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	  $(patsubst %.perl,%,$(SCRIPT_PERL) $(SCRIPT_PERL_CVS)) \
 	  $(patsubst %.py,%,$(SCRIPT_PYTHON)) \
 	  git-instaweb
 
@@ -1721,13 +1725,25 @@ $(SCRIPT_LIB) : % : %.sh
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
+_SCRIPT_PERL_BUILD = 
+_SCRIPT_PERL_NOBUILD = 
+
 ifndef NO_PERL
-$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
+
+_SCRIPT_PERL_BUILD += $(SCRIPT_PERL)
+
+ifndef NO_CVS
+_SCRIPT_PERL_BUILD += $(SCRIPT_PERL_CVS)
+else # NO_CVS
+_SCRIPT_PERL_NOBUILD += $(SCRIPT_PERL_CVS)
+endif # NO_CVS
+
+$(patsubst %.perl,%,$(_SCRIPT_PERL_BUILD)): perl/perl.mak
 
 perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
-$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
+$(patsubst %.perl,%,$(_SCRIPT_PERL_BUILD)): % : %.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	sed -e '1{' \
@@ -1784,14 +1800,17 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/static/gitweb.css gitweb/
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PERL
-$(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
+_SCRIPT_PERL_NOBUILD += $(SCRIPT_PERL) $(SCRIPT_PERL_CVS) git-instaweb
+endif # NO_PERL
+
+# This is any perl scripts that were disabled it might be empty...
+$(patsubst %.perl,%,$(_SCRIPT_PERL_NOBUILD)): % : unimplemented.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
-endif # NO_PERL
 
 ifndef NO_PYTHON
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
@@ -1826,7 +1845,7 @@ configure: configure.ac
 # These can record GIT_VERSION
 git.o git.spec \
 	$(patsubst %.sh,%,$(SCRIPT_SH)) \
-	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	$(patsubst %.perl,%,$(SCRIPT_PERL) $(SCRIPT_PERL_CVS)) \
 	: GIT-VERSION-FILE
 
 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index e5da65b..4b3f010 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -12,6 +12,11 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
+if ! test_have_prereq CVS; then
+	skip_all='skipping git cvsexportcommit tests, cvs not available'
+	test_done
+fi
+
 cvs >/dev/null 2>&1
 if test $? -ne 1
 then
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 9199550..52ea99d 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -11,9 +11,15 @@ cvs CLI client via git-cvsserver server'
 . ./test-lib.sh
 
 if ! test_have_prereq PERL; then
-	skip_all='skipping git cvsserver tests, perl not available'
+	skip_all='skipping git-cvsserver tests, perl not available'
 	test_done
 fi
+
+if ! test_have_prereq CVS; then
+	skip_all='skipping git-cvsserver tests, cvs not available'
+	test_done
+fi
+
 cvs >/dev/null 2>&1
 if test $? -ne 1
 then
diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index ff6d6fb..f0d7aad 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -38,15 +38,20 @@ not_present() {
     fi
 }
 
-cvs >/dev/null 2>&1
-if test $? -ne 1
+if ! test_have_prereq PERL
 then
-    skip_all='skipping git-cvsserver tests, cvs not found'
+    skip_all='skipping git-cvsserver tests, perl not available'
     test_done
 fi
-if ! test_have_prereq PERL
+if ! test_have_prereq CVS
 then
-    skip_all='skipping git-cvsserver tests, perl not available'
+    skip_all='skipping git-cvsserver tests, cvs not available'
+    test_done
+fi
+cvs >/dev/null 2>&1
+if test $? -ne 1
+then
+    skip_all='skipping git-cvsserver tests, cvs not found'
     test_done
 fi
 "$PERL_PATH" -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index 4c384ff..d601f32 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh
@@ -3,14 +3,25 @@
 test_description='git cvsimport basic tests'
 . ./lib-cvs.sh
 
-test_expect_success PERL 'setup cvsroot environment' '
+if ! test_have_prereq PERL
+then
+    skip_all='skipping git cvsimport tests, perl not available'
+    test_done
+fi
+if ! test_have_prereq CVS
+then
+    skip_all='skipping git cvsimport tests, cvs not available'
+    test_done
+fi
+
+test_expect_success 'setup cvsroot environment' '
 	CVSROOT=$(pwd)/cvsroot &&
 	export CVSROOT
 '
 
-test_expect_success PERL 'setup cvsroot' '$CVS init'
+test_expect_success 'setup cvsroot' '$CVS init'
 
-test_expect_success PERL 'setup a cvs module' '
+test_expect_success 'setup a cvs module' '
 
 	mkdir "$CVSROOT/module" &&
 	$CVS co -d module-cvs module &&
@@ -42,23 +53,23 @@ EOF
 	)
 '
 
-test_expect_success PERL 'import a trivial module' '
+test_expect_success 'import a trivial module' '
 
 	git cvsimport -a -R -z 0 -C module-git module &&
 	test_cmp module-cvs/o_fortuna module-git/o_fortuna
 
 '
 
-test_expect_success PERL 'pack refs' '(cd module-git && git gc)'
+test_expect_success 'pack refs' '(cd module-git && git gc)'
 
-test_expect_success PERL 'initial import has correct .git/cvs-revisions' '
+test_expect_success 'initial import has correct .git/cvs-revisions' '
 
 	(cd module-git &&
 	 git log --format="o_fortuna 1.1 %H" -1) > expected &&
 	test_cmp expected module-git/.git/cvs-revisions
 '
 
-test_expect_success PERL 'update cvs module' '
+test_expect_success 'update cvs module' '
 	(cd module-cvs &&
 	cat <<EOF >o_fortuna &&
 O Fortune,
@@ -86,7 +97,7 @@ EOF
 	)
 '
 
-test_expect_success PERL 'update git module' '
+test_expect_success 'update git module' '
 
 	(cd module-git &&
 	git config cvsimport.trackRevisions true &&
@@ -97,7 +108,7 @@ test_expect_success PERL 'update git module' '
 
 '
 
-test_expect_success PERL 'update has correct .git/cvs-revisions' '
+test_expect_success 'update has correct .git/cvs-revisions' '
 
 	(cd module-git &&
 	 git log --format="o_fortuna 1.1 %H" -1 HEAD^ &&
@@ -105,7 +116,7 @@ test_expect_success PERL 'update has correct .git/cvs-revisions' '
 	test_cmp expected module-git/.git/cvs-revisions
 '
 
-test_expect_success PERL 'update cvs module' '
+test_expect_success 'update cvs module' '
 
 	(cd module-cvs &&
 		echo 1 >tick &&
@@ -114,7 +125,7 @@ test_expect_success PERL 'update cvs module' '
 	)
 '
 
-test_expect_success PERL 'cvsimport.module config works' '
+test_expect_success 'cvsimport.module config works' '
 
 	(cd module-git &&
 		git config cvsimport.module module &&
@@ -126,7 +137,7 @@ test_expect_success PERL 'cvsimport.module config works' '
 
 '
 
-test_expect_success PERL 'second update has correct .git/cvs-revisions' '
+test_expect_success 'second update has correct .git/cvs-revisions' '
 
 	(cd module-git &&
 	 git log --format="o_fortuna 1.1 %H" -1 HEAD^^ &&
@@ -135,7 +146,7 @@ test_expect_success PERL 'second update has correct .git/cvs-revisions' '
 	test_cmp expected module-git/.git/cvs-revisions
 '
 
-test_expect_success PERL 'import from a CVS working tree' '
+test_expect_success 'import from a CVS working tree' '
 
 	$CVS co -d import-from-wt module &&
 	(cd import-from-wt &&
@@ -148,12 +159,12 @@ test_expect_success PERL 'import from a CVS working tree' '
 
 '
 
-test_expect_success PERL 'no .git/cvs-revisions created by default' '
+test_expect_success 'no .git/cvs-revisions created by default' '
 
 	! test -e import-from-wt/.git/cvs-revisions
 
 '
 
-test_expect_success PERL 'test entire HEAD' 'test_cmp_branch_tree master'
+test_expect_success 'test entire HEAD' 'test_cmp_branch_tree master'
 
 test_done
diff --git a/t/t9601-cvsimport-vendor-branch.sh b/t/t9601-cvsimport-vendor-branch.sh
index 827d39f..d730a41 100755
--- a/t/t9601-cvsimport-vendor-branch.sh
+++ b/t/t9601-cvsimport-vendor-branch.sh
@@ -34,6 +34,17 @@
 test_description='git cvsimport handling of vendor branches'
 . ./lib-cvs.sh
 
+if ! test_have_prereq PERL
+then
+    skip_all='skipping git cvsimport tests, perl not available'
+    test_done
+fi
+if ! test_have_prereq CVS
+then
+    skip_all='skipping git cvsimport tests, cvs not available'
+    test_done
+fi
+
 setup_cvs_test_repository t9601
 
 test_expect_success PERL 'import a module with a vendor branch' '
diff --git a/t/t9602-cvsimport-branches-tags.sh b/t/t9602-cvsimport-branches-tags.sh
index e1db323..68f0974 100755
--- a/t/t9602-cvsimport-branches-tags.sh
+++ b/t/t9602-cvsimport-branches-tags.sh
@@ -6,6 +6,17 @@
 test_description='git cvsimport handling of branches and tags'
 . ./lib-cvs.sh
 
+if ! test_have_prereq PERL
+then
+    skip_all='skipping git cvsimport tests, perl not available'
+    test_done
+fi
+if ! test_have_prereq CVS
+then
+    skip_all='skipping git cvsimport tests, cvs not available'
+    test_done
+fi
+
 setup_cvs_test_repository t9602
 
 test_expect_success PERL 'import module' '
diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh
index 52034c8..db4d682 100755
--- a/t/t9603-cvsimport-patchsets.sh
+++ b/t/t9603-cvsimport-patchsets.sh
@@ -14,6 +14,17 @@
 test_description='git cvsimport testing for correct patchset estimation'
 . ./lib-cvs.sh
 
+if ! test_have_prereq PERL
+then
+    skip_all='skipping git cvsimport tests, perl not available'
+    test_done
+fi
+if ! test_have_prereq CVS
+then
+    skip_all='skipping git cvsimport tests, cvs not available'
+    test_done
+fi
+
 setup_cvs_test_repository t9603
 
 test_expect_failure 'import with criss cross times on revisions' '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index cb1ca97..d594c95 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1066,6 +1066,7 @@ case $(uname -s) in
 	;;
 esac
 
+test -z "$NO_CVS" && test_set_prereq CVS
 test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PYTHON" && test_set_prereq PYTHON
 

-- 
1.7.3.4
Jonathan Nieder· Jan 7, 2011, 22:01 UTC · re: Robin H. Johnson · lore

Re: [PATCH] git-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

Robin H. Johnson wrote:
> Presently, the CVS interface scripts are always built, and their
> test-suites run based on a binary named 'cvs' happening to return zero.
> If there something other than the real CVS there, bad things happened
> during the test-suite run.

This explanation seems quite odd to me. Are you saying we can't rely on the 'cvs' name being "taken" and should live in fear that someone will implement an incompatible utility with the same name? Did that actually happen?

I would find it easier to believe
	Building and testing git's cvs support is slow, because ...
	So give users with no interest in cvs interoperability a way
	out.  By defining NO_CVS you can avoid this time-consuming
	piece of the build process.
Or for a different patch:
	Add a NO_CVS knob so users with no interest in cvs support
	can avoid polluting their $(libexecdir) with unwanted entries.
Or:
	Introduce a new NO_CVS knob.  If set, the CVS interop scripts
	will be replaced by unimplemented.sh so sysadmins and
	distributors can hopefully get a nice, clear error report instead
	of confusion when users try to run them with cvs not installed.
Robin H. Johnson· Jan 7, 2011, 22:55 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

On Fri, Jan 07, 2011 at 04:01:48PM -0600, Jonathan Nieder wrote:
> This explanation seems quite odd to me.  Are you saying we can't rely
> on the 'cvs' name being "taken" and should live in fear that someone
> will implement an incompatible utility with the same name?  Did that
> actually happen?

Not in the linked bug report, but it does explain a previous bug I had seen, where a user had a little script in /usr/local/bin that complained at him whenever he ran 'cvs', so he would learn to migrate away faster.

> I would find it easier to believe

I'm going to respin the patch with a new text, and one other improvement from Junio.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85
Junio C Hamano· Jan 7, 2011, 23:50 UTC · re: Robin H. Johnson · lore

Re: [PATCH] git-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

"Robin H. Johnson" <robbat2@gentoo.org> writes:
Show 8 quoted lines
> On Fri, Jan 07, 2011 at 04:01:48PM -0600, Jonathan Nieder wrote:
>> This explanation seems quite odd to me.  Are you saying we can't rely
>> on the 'cvs' name being "taken" and should live in fear that someone
>> will implement an incompatible utility with the same name?  Did that
>> actually happen?
> Not in the linked bug report, but it does explain a previous bug I had
> seen, where a user had a little script in /usr/local/bin that complained
> at him whenever he ran 'cvs', so he would learn to migrate away faster.

I suspect that NO_CVS is not the best way to help a person who is trying to wean herself off of cvs by having a phony cvs in a directory that is early on the $PATH (be it $HOME/bin/cvs or /usr/local/bin/cvs). Before ceasing to actively build more histories in cvs, it would help to have an access to git-cvsimport and friends to salvage what's already there, no?

A change that models after PERL_PATH, not after NO_PERL, would be a more appropriate thing to do for that particular purpose.

I still think the patch itself is a worthy thing to have, so everything above is a tangent that is orthogonal to what your patch tries to do, though.

Junio C Hamano· Jan 7, 2011, 22:05 UTC · re: Robin H. Johnson · lore

Re: [PATCH] git-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

"Robin H. Johnson" <robbat2@gentoo.org> writes:
> Presently, the CVS interface scripts are always built, and their
> test-suites run based on a binary named 'cvs' happening to return zero.
> If there something other than the real CVS there, bad things happened
> during the test-suite run.
Is that a problem?

It makes sense to let people whose system happens to have a working cvs to omit cvs interoperability bits when they know the do not want them, and that alone would be a good enough motivation.

I'd even consider the above justification of yours detrimental---it would be an excuse for other people to add patches to support NO_CAT, NO_DIFF, NO_LS, ... saying "if a binary 'cat' that is not cat is there, things break, so work it around". That is a road to nonsense land.

> This patch implements NO_CVS in the manner of NO_PERL, and ensures that
> the CVS scripts get the unimplemented variant when appropriate, as well
> as making sure that the tests properly declare CVS as a prerequisite
> (shortcut to test_done like the Perl prerequisites).

While the patch looks good, some people seem to prefer skipping individual tests without shortcut; see 900eab4 (t/t9600-cvsimport.sh: change from skip_all=* to prereq skip, 2010-08-13) for example. I am slightly in favor of the short-cut as I haven't heard convincing argument against it other than "skipped statistics" which I don't think is interesting nor accurate anyway.

I wonder if "check PERL and CVS prerequisite and say test_done" should be made into a helper in lib-cvs.sh or somewhere instead of repeating them in individual tests, but that is a minor point.

Robin H. Johnson· Jan 7, 2011, 22:53 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvs*: Make building (and testing) of CVS interface scripts optionally selectable

On Fri, Jan 07, 2011 at 02:05:59PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> > Presently, the CVS interface scripts are always built, and their
> > test-suites run based on a binary named 'cvs' happening to return zero.
> > If there something other than the real CVS there, bad things happened
> > during the test-suite run.
> It makes sense to let people whose system happens to have a working cvs to
> omit cvs interoperability bits when they know the do not want them, and
> that alone would be a good enough motivation.
Ok, i'll respin to a different explanation.
Show 6 quoted lines
> While the patch looks good, some people seem to prefer skipping individual
> tests without shortcut; see 900eab4 (t/t9600-cvsimport.sh: change from
> skip_all=* to prereq skip, 2010-08-13) for example.  I am slightly in
> favor of the short-cut as I haven't heard convincing argument against it
> other than "skipped statistics" which I don't think is interesting nor
> accurate anyway.

I did it this was because of your prior comment on my perl prereq patch for the send-email tests.

> I wonder if "check PERL and CVS prerequisite and say test_done" should be
> made into a helper in lib-cvs.sh or somewhere instead of repeating them in
> individual tests, but that is a minor point.
I'll see if I can respin to do this.
-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

← back to recent threads