Fix git-svn in non-MakeMaker builds
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 4 lines · Adam Roben
c102f4cf729a65b3520dbb17b52aa0c4fe4d4b29anda6180325e885e2226d68144000e8bb18a906a2a6split 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
@@ -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
[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
@@ -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
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'.
Quoted 53 lines · Jonathan Nieder
Quoted 4 lines · Adam Roben
c102f4cf729a65b3520dbb17b52aa0c4fe4d4b29anda6180325e885e2226d68144000e8bb18a906a2a6split 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, toov1.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_MAKEMAKERis 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 <[email protected]>Signed-off-by Jonathan Nieder <[email protected]>---
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@@ -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"' >> $@
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
@@ -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
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.
Requested-by Junio C Hamano <[email protected]>Signed-off-by Jonathan Nieder <[email protected]>---
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@@ -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"' >> $@
PATCH 1/1 perl/Makefile: Fix a missing double-quote
In reply to Jonathan NiederFrom: Johannes Sixt <[email protected]>
--- perl/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Makefile +1 -1
@@ -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
Re: [PATCH jn/perl-makemaker-leading-paths] perl/Makefile: Fix a missing double-quote
Quoted 1 line · Johannes Sixt
Signed-off-by Johannes Sixt <[email protected]>
Yes, not sure how I missed that. :(