threads / patch / 30818

patchRe: [PATCH] Fix git-svn in non-MakeMaker builds

Subject: Re: [PATCH] Fix git-svn in non-MakeMaker builds

## tl;dr

8 messages between Jun 15, 2012 and Jun 29, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jonathan Nieder· Jun 15, 2012, 18:05 UTC · lore
Adam Roben wrote:
> 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.

Reported-by: Adam Roben <adam@roben.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmali.com>
---
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(-)
Show changes to perl/Makefile +24 −7
diff --git a/perl/Makefile b/perl/Makefile
index 3e21766d..fe7a4864 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -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"' >> $@

-- 
1.7.10.4
Jonathan Nieder· Jun 15, 2012, 18:14 UTC · re: Jonathan Nieder · lore

[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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 perl/Makefile.PL |    4 ++++
 1 file changed, 4 insertions(+)
Show changes to perl/Makefile.PL +4 −0
diff --git a/perl/Makefile.PL b/perl/Makefile.PL
index 2c20290f..b54b04a6 100644
--- a/perl/Makefile.PL
+++ b/perl/Makefile.PL
@@ -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',

-- 
1.7.10.4
Junio C Hamano· Jun 15, 2012, 21:25 UTC · re: Jonathan Nieder · lore
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 28 quoted lines
> Adam Roben wrote:
>
>> 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.
>
> Reported-by: Adam Roben <adam@roben.org>
> Signed-off-by: Jonathan Nieder <jrnieder@gmali.com>
> ---
> 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.
Show 34 quoted lines
>  perl/Makefile |   31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/perl/Makefile b/perl/Makefile
> index 3e21766d..fe7a4864 100644
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -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' >> $@

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?

Show 15 quoted lines
> +	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"' >> $@
Likewise.
Show 9 quoted lines
> +	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"' >> $@
Jonathan Nieder· Jun 15, 2012, 23:02 UTC · re: Junio C Hamano · lore

[PATCH] 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.

Requested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jun 15, 2012 at 02:25:34PM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> +	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(-)
Show changes to perl/Makefile +14 −2
diff --git a/perl/Makefile b/perl/Makefile
index fe7a4864..2e8f9804 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -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"' >> $@

-- 
1.7.10.4
Junio C Hamano· Jun 15, 2012, 23:10 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 24 quoted lines
> 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.
>
> Requested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jun 15, 2012 at 02:25:34PM -0700, Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> +	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.

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.
Show 44 quoted lines
>  perl/Makefile |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/perl/Makefile b/perl/Makefile
> index fe7a4864..2e8f9804 100644
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -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"' >> $@
Johannes Sixt· Jun 29, 2012, 08:07 UTC · re: Jonathan Nieder · lore

[PATCH jn/perl-makemaker-leading-paths] perl/Makefile: Fix a missing double-quote

From: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 perl/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to perl/Makefile +1 −1
diff --git a/perl/Makefile b/perl/Makefile
index 2e8f980..6ca7d47 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -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"' >> $@

-- 
1.7.11.1.1304.g11834c6
Jonathan Nieder· Jun 29, 2012, 08:55 UTC · re: Johannes Sixt · lore

Re: [PATCH jn/perl-makemaker-leading-paths] perl/Makefile: Fix a missing double-quote

Johannes Sixt wrote:
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Yes, not sure how I missed that. :(
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

← back to recent threads