Skip to contentSkip to conversation

Fix git-svn in non-MakeMaker builds

PATCH In review Jonathan Nieder 3 participants 8 messages Jun 15, 2016 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw
Quoted 4 lines · Adam Roben

c102f4cf729a65b3520dbb17b52aa0c4fe4d4b29 and a6180325e885e2226d68144000e8bb18a906a2a6 split some git-svn code into separate modules, but forgot to inform the non-MakeMaker build about those new modules.

Good catch. How about this patch against master (untested)?

-- >8 --
Subject: perl/Makefile: install Git::SVN::* when NO_PERL_MAKEMAKER=yes, too

v1.7.11-rc1~12^2~2 (2012-05-27) and friends split some git-svn code into separate modules but did not update the fallback rules to install them when NO_PERL_MAKEMAKER is set. Add the appropriate rules so users without MakeMaker can use git-svn again.

Affected modules: Git::SVN::Prompt, Git::SVN::Fetcher, Git::SVN::Editor, Git::SVN::Ra, Git::SVN::Memoize::YAML.

---
Sensible?

In principle this should be two patches --- one to introduce the loop, another to add Git::SVN::* to that loop. Let me know if you think this would be easier to read that way and I can try it.

 perl/Makefile |   31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)
perl/Makefile +24 -7
index 3e21766d..fe7a4864 100644
@@ -2,6 +2,7 @@
 # Makefile for perl support modules and routine
 #
 makfile:=perl.mak
+modules =
 
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 prefix_SQ = $(subst ','\'',$(prefix))
@@ -22,19 +23,35 @@ clean:
 
 ifdef NO_PERL_MAKEMAKER
 instdir_SQ = $(subst ','\'',$(prefix)/lib)
+
+modules += Git
+modules += Git/I18N
+modules += Git/SVN/Memoize/YAML
+modules += Git/SVN/Fetcher
+modules += Git/SVN/Editor
+modules += Git/SVN/Prompt
+modules += Git/SVN/Ra
+
 $(makfile): ../GIT-CFLAGS Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
-	echo '	mkdir -p blib/lib/Git' >> $@
-	echo '	$(RM) blib/lib/Git.pm; cp Git.pm blib/lib/' >> $@
-	echo '	$(RM) blib/lib/Git/I18N.pm; cp Git/I18N.pm blib/lib/Git/' >> $@
+	echo '	mkdir -p blib/lib/Git/SVN/Memoize' >> $@
+	set -e; \
+	for i in $(modules); \
+	do \
+		echo '	$(RM) blib/lib/'$$i'.pm' >> $@; \
+		echo '	cp '$$i'.pm blib/lib/'$$i'.pm' >> $@; \
+	done
 	echo '	$(RM) blib/lib/Error.pm' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm blib/lib/Error.pm' >> $@
 	echo install: >> $@
-	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)"' >> $@
-	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)/Git"' >> $@
-	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Git.pm"; cp Git.pm "$$(DESTDIR)$(instdir_SQ)"' >> $@
-	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Git/I18N.pm"; cp Git/I18N.pm "$$(DESTDIR)$(instdir_SQ)/Git"' >> $@
+	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)/Git/SVN/Memoize"' >> $@
+	set -e; \
+	for i in $(modules); \
+	do \
+		echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+		echo '	cp '$$i'.pm "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+	done
 	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
Signature · 1 line
1.7.10.4
#2 lore raw

[PATCH 2/1] perl/Makefile.PL: warn about duplicate module list in perl/Makefile

Adding or removing a module requires modifying both files to support builds with and without MakeMaker. Add a comment to remind patch authors and reviewers at the crucial moment.

Longer term, it would be nicer to maintain a single list, perhaps in a separate file used by both build systems.

---
 perl/Makefile.PL |    4 ++++
 1 file changed, 4 insertions(+)
perl/Makefile.PL +4 -0
index 2c20290f..b54b04a6 100644
@@ -24,6 +24,10 @@ endif
 MAKE_FRAG
 }
 
+# XXX. When editing this list:
+#
+# * Please update perl/Makefile, too.
+# * Don't forget to test with NO_PERL_MAKEMAKER=YesPlease
 my %pm = (
 	'Git.pm' => '$(INST_LIBDIR)/Git.pm',
 	'Git/I18N.pm' => '$(INST_LIBDIR)/Git/I18N.pm',
Signature · 1 line
1.7.10.4
#3 lore raw

Re: [PATCH 2/1] perl/Makefile.PL: warn about duplicate module list in perl/Makefile

Thanks for a last minute fix. Will apply both directly to 'master'.

#4 lore raw
In reply to Jonathan Nieder
Quoted 53 lines · Jonathan Nieder
Quoted 4 lines · Adam Roben

c102f4cf729a65b3520dbb17b52aa0c4fe4d4b29 and a6180325e885e2226d68144000e8bb18a906a2a6 split some git-svn code into separate modules, but forgot to inform the non-MakeMaker build about those new modules.

Good catch. How about this patch against master (untested)?

-- >8 --
Subject: perl/Makefile: install Git::SVN::* when NO_PERL_MAKEMAKER=yes, too

v1.7.11-rc1~12^2~2 (2012-05-27) and friends split some git-svn code into separate modules but did not update the fallback rules to install them when NO_PERL_MAKEMAKER is set. Add the appropriate rules so users without MakeMaker can use git-svn again.

Affected modules: Git::SVN::Prompt, Git::SVN::Fetcher, Git::SVN::Editor, Git::SVN::Ra, Git::SVN::Memoize::YAML.

---
Sensible?

In principle this should be two patches --- one to introduce the loop, another to add Git::SVN::* to that loop. Let me know if you think this would be easier to read that way and I can try it.

 perl/Makefile |   31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)
perl/Makefile +6 -0
index 3e21766d..fe7a4864 100644
@@ -2,6 +2,7 @@
 # Makefile for perl support modules and routine
 #
 makfile:=perl.mak
+modules =
 
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 prefix_SQ = $(subst ','\'',$(prefix))
@@ -22,19 +23,35 @@ clean:
 
 ifdef NO_PERL_MAKEMAKER
 instdir_SQ = $(subst ','\'',$(prefix)/lib)
+
+modules += Git
+modules += Git/I18N
+modules += Git/SVN/Memoize/YAML
+modules += Git/SVN/Fetcher

perl/Makefile clean: · line 37

Quoted 10 lines
perl/Makefile +5 -3
+modules += Git/SVN/Editor
+modules += Git/SVN/Prompt
+modules += Git/SVN/Ra
+
 $(makfile): ../GIT-CFLAGS Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
-	echo '	mkdir -p blib/lib/Git' >> $@
-	echo '	$(RM) blib/lib/Git.pm; cp Git.pm blib/lib/' >> $@
-	echo '	$(RM) blib/lib/Git/I18N.pm; cp Git/I18N.pm blib/lib/Git/' >> $@
+	echo '	mkdir -p blib/lib/Git/SVN/Memoize' >> $@

Wouldn't it be much less error prone to maintain if you did not hardcode the "blib/lib/Git/SVN/Memoize is the deepest level" here, and run the "mkdir -p blib/lib/$$(basename $$i)" or something in the loop instead?

Quoted 5 lines
perl/Makefile +5 -0
+	set -e; \
+	for i in $(modules); \
+	do \
+		echo '	$(RM) blib/lib/'$$i'.pm' >> $@; \
+		echo '	cp '$$i'.pm blib/lib/'$$i'.pm' >> $@; \

perl/Makefile clean: · line 48

Quoted 10 lines
perl/Makefile +2 -4
+	done
 	echo '	$(RM) blib/lib/Error.pm' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm blib/lib/Error.pm' >> $@
 	echo install: >> $@
-	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)"' >> $@
-	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)/Git"' >> $@
-	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Git.pm"; cp Git.pm "$$(DESTDIR)$(instdir_SQ)"' >> $@
-	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Git/I18N.pm"; cp Git/I18N.pm "$$(DESTDIR)$(instdir_SQ)/Git"' >> $@
+	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)/Git/SVN/Memoize"' >> $@

Likewise.

Quoted 9 lines
perl/Makefile +6 -0
+	set -e; \
+	for i in $(modules); \
+	do \
+		echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+		echo '	cp '$$i'.pm "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+	done
 	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
#5 lore raw

PATCH 1/1 perl/Makefile: move "mkdir -p" to module installation loop for maintainability

In the NO_PERL_MAKEMAKER=YesPlease fallback case, make the directory that will contain each module when installing it (simulating "install -D") instead of hardcoding "Git/SVN/Memoize is the deepest level". This should make this codepath which is not used often on development machines a little easier to maintain.

---

Quoted 8 lines · Junio C Hamano

Review

Quoted 1 line · Jonathan Nieder
(quoted diff) +1 -0
+	echo '	mkdir -p blib/lib/Git/SVN/Memoize' >> $@

Wouldn't it be much less error prone to maintain if you did not hardcode the "blib/lib/Git/SVN/Memoize is the deepest level" here, and run the "mkdir -p blib/lib/$$(basename $$i)" or something in the loop instead?

Yes, I think so.

Wasn't sure if "install -D" or "dirname" is portable, so this patch avoids them.

 perl/Makefile |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
perl/Makefile +14 -2
index fe7a4864..2e8f9804 100644
@@ -34,22 +34,34 @@ modules += Git/SVN/Ra
 
 $(makfile): ../GIT-CFLAGS Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
-	echo '	mkdir -p blib/lib/Git/SVN/Memoize' >> $@
 	set -e; \
 	for i in $(modules); \
 	do \
+		if test $$i = $${i%/*}; \
+		then \
+			subdir=; \
+		else \
+			subdir=/$${i%/*}; \
+		fi; \
 		echo '	$(RM) blib/lib/'$$i'.pm' >> $@; \
+		echo '	mkdir -p blib/lib'$$subdir >> $@; \
 		echo '	cp '$$i'.pm blib/lib/'$$i'.pm' >> $@; \
 	done
 	echo '	$(RM) blib/lib/Error.pm' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm blib/lib/Error.pm' >> $@
 	echo install: >> $@
-	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)/Git/SVN/Memoize"' >> $@
 	set -e; \
 	for i in $(modules); \
 	do \
+		if test $$i = $${i%/*}; \
+		then \
+			subdir=; \
+		else \
+			subdir=/$${i%/*}; \
+		fi; \
 		echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+		echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)'$$subdir >> $@; \
 		echo '	cp '$$i'.pm "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
 	done
 	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
Signature · 1 line
1.7.10.4
#6 lore raw

Re: [PATCH] perl/Makefile: move "mkdir -p" to module installation loop for maintainability

Quoted 20 lines · Jonathan Nieder

In the NO_PERL_MAKEMAKER=YesPlease fallback case, make the directory that will contain each module when installing it (simulating "install -D") instead of hardcoding "Git/SVN/Memoize is the deepest level". This should make this codepath which is not used often on development machines a little easier to maintain.

---

Quoted 8 lines · Junio C Hamano

Review

Quoted 1 line · Jonathan Nieder
(quoted diff) +1 -0
+	echo '	mkdir -p blib/lib/Git/SVN/Memoize' >> $@

Wouldn't it be much less error prone to maintain if you did not hardcode the "blib/lib/Git/SVN/Memoize is the deepest level" here, and run the "mkdir -p blib/lib/$$(basename $$i)" or something in the loop instead?

Quoted 4 lines

Yes, I think so.

Wasn't sure if "install -D" or "dirname" is portable, so this patch avoids them.

OK. I suspect others may come up with a cleaner solution, so I'm tempted to hold this in 'pu' for now and use the original one, which was _not wrong_ in any way, for the 1.7.11 final.

Thanks.

Quoted 44 lines
 perl/Makefile |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
perl/Makefile +14 -2
index fe7a4864..2e8f9804 100644
@@ -34,22 +34,34 @@ modules += Git/SVN/Ra
 
 $(makfile): ../GIT-CFLAGS Makefile
 	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
-	echo '	mkdir -p blib/lib/Git/SVN/Memoize' >> $@
 	set -e; \
 	for i in $(modules); \
 	do \
+		if test $$i = $${i%/*}; \
+		then \
+			subdir=; \
+		else \
+			subdir=/$${i%/*}; \
+		fi; \
 		echo '	$(RM) blib/lib/'$$i'.pm' >> $@; \
+		echo '	mkdir -p blib/lib'$$subdir >> $@; \
 		echo '	cp '$$i'.pm blib/lib/'$$i'.pm' >> $@; \
 	done
 	echo '	$(RM) blib/lib/Error.pm' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm blib/lib/Error.pm' >> $@
 	echo install: >> $@
-	echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)/Git/SVN/Memoize"' >> $@
 	set -e; \
 	for i in $(modules); \
 	do \
+		if test $$i = $${i%/*}; \
+		then \
+			subdir=; \
+		else \
+			subdir=/$${i%/*}; \
+		fi; \
 		echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+		echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)'$$subdir >> $@; \
 		echo '	cp '$$i'.pm "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
 	done
 	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
#7 lore raw

PATCH 1/1 perl/Makefile: Fix a missing double-quote

In reply to Jonathan Nieder

From: Johannes Sixt <[email protected]>

---
 perl/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Makefile +1 -1
index 2e8f980..6ca7d47 100644
@@ -61,7 +61,7 @@ $(makfile): ../GIT-CFLAGS Makefile
 			subdir=/$${i%/*}; \
 		fi; \
 		echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
-		echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)'$$subdir >> $@; \
+		echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)'$$subdir'"' >> $@; \
 		echo '	cp '$$i'.pm "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
 	done
 	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
Signature · 1 line
1.7.11.1.1304.g11834c6