[PATCH 1/1] perl: fix installing modules from contrib

[PATCH 1/1] perl: fix installing modules from contrib

[Christian Hesse]

Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
removed a target that allowed Makefiles from contrib/ to get the correct
install path. This introduces a new target for main Makefile and fixes
installation for Mediawiki module.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 Makefile                   | 2 ++
 contrib/mw-to-git/Makefile | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index a1d8775ad..bcaf50495 100644
--- a/Makefile
+++ b/Makefile
@@ -2002,6 +2002,8 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+perllibdir:
+	@echo $(perllibdir_SQ)
 
 .PHONY: gitweb
 gitweb:
diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile
index a4b6f7a2c..0a6e59579 100644
--- a/contrib/mw-to-git/Makefile
+++ b/contrib/mw-to-git/Makefile
@@ -21,8 +21,8 @@ HERE=contrib/mw-to-git/
 INSTALL = install
 
 SCRIPT_PERL_FULL=$(patsubst %,$(HERE)/%,$(SCRIPT_PERL))
-INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/perl \
-                -s --no-print-directory instlibdir)
+INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/ \
+                -s --no-print-directory perllibdir=$(perllibdir) perllibdir)
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 INSTLIBDIR_SQ = $(subst ','\'',$(INSTLIBDIR))
 

Re: [PATCH 1/1] perl: fix installing modules from contrib

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 03 2018, Christian Hesse wrote:

> Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
> removed a target that allowed Makefiles from contrib/ to get the correct
> install path. This introduces a new target for main Makefile and fixes
> installation for Mediawiki module.
>
> Signed-off-by: Christian Hesse <mail@eworm.de>
> ---
>  Makefile                   | 2 ++
>  contrib/mw-to-git/Makefile | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index a1d8775ad..bcaf50495 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2002,6 +2002,8 @@ GIT-PERL-DEFINES: FORCE
>  		echo "$$FLAGS" >$@; \
>  	    fi
>
> +perllibdir:
> +	@echo $(perllibdir_SQ)
>
>  .PHONY: gitweb
>  gitweb:
> diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile
> index a4b6f7a2c..0a6e59579 100644
> --- a/contrib/mw-to-git/Makefile
> +++ b/contrib/mw-to-git/Makefile
> @@ -21,8 +21,8 @@ HERE=contrib/mw-to-git/
>  INSTALL = install
>
>  SCRIPT_PERL_FULL=$(patsubst %,$(HERE)/%,$(SCRIPT_PERL))
> -INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/perl \
> -                -s --no-print-directory instlibdir)
> +INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/ \
> +                -s --no-print-directory perllibdir=$(perllibdir) perllibdir)
>  DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
>  INSTLIBDIR_SQ = $(subst ','\'',$(INSTLIBDIR))

Thanks, I (obviously) missed that when getting rid of the perl/Makefile.

This fixes it up for now, but it seems we're going to need some solution
to make this work with the in-flight RUNTIME_PREFIX Dan's been working
on.

I think the best solution for that, not just for this but for most of
contrib/ in general, is to simply move it into our main tree out of
contrib/, and introduce some Makefile flags for whether or not you'd
want to install such-and-such from contrib.

That would probably be easier than the current arrangement, and we could
do things like say we always want to run tests for contrib/ stuff, even
though we're not installing it.

Re: [PATCH 1/1] perl: fix installing modules from contrib

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>
>> +perllibdir:
>> +	@echo $(perllibdir_SQ)

This use of _SQ variant is fishy, isn't it?  Judging from the output
of 

    $ git grep _SQ Makefile

e.g.

    Makefile:	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
    Makefile:	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
    Makefile:	$(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'

I'd expect that any _SQ variant must be referenced inside a single
quote pair.  In fact, that is why a single quote (and nothing else)
in the base variable is replaced with the magic "'\''" sequence,
first stepping out of the current sq context, append a single sq
(escaped with a backslash from the shell), and then stepping back
into another sq context.

I think nobody saw breakage only because they do not have two
consecutive SPs (or any single quote) in their path to $perllibdir.
If we depend on such limitation, there is no point using _SQ
variant, but we already have _SQ variant, let's use it correctly.

[PATCH v2 1/1] perl: fix installing modules from contrib

[Christian Hesse]

Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
removed a target that allowed Makefiles from contrib/ to get the correct
install path. This introduces a new target for main Makefile and fixes
installation for Mediawiki module.

v2: Pass prefix as that can have influence as well, add single quotes
    for _SQ variant.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 Makefile                   | 2 ++
 contrib/mw-to-git/Makefile | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 96f6138f6..19ca5e8de 100644
--- a/Makefile
+++ b/Makefile
@@ -2011,6 +2011,8 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+perllibdir:
+	@echo '$(perllibdir_SQ)'
 
 .PHONY: gitweb
 gitweb:
diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile
index a4b6f7a2c..4e603512a 100644
--- a/contrib/mw-to-git/Makefile
+++ b/contrib/mw-to-git/Makefile
@@ -21,8 +21,9 @@ HERE=contrib/mw-to-git/
 INSTALL = install
 
 SCRIPT_PERL_FULL=$(patsubst %,$(HERE)/%,$(SCRIPT_PERL))
-INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/perl \
-                -s --no-print-directory instlibdir)
+INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/ \
+                -s --no-print-directory prefix=$(prefix) \
+                perllibdir=$(perllibdir) perllibdir)
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 INSTLIBDIR_SQ = $(subst ','\'',$(INSTLIBDIR))
 

Re: [PATCH v2 1/1] perl: fix installing modules from contrib

[Junio C Hamano]

Christian Hesse <mail@eworm.de> writes:

> Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
> removed a target that allowed Makefiles from contrib/ to get the correct
> install path. This introduces a new target for main Makefile and fixes
> installation for Mediawiki module.
>
> v2: Pass prefix as that can have influence as well, add single quotes
>     for _SQ variant.
>
> Signed-off-by: Christian Hesse <mail@eworm.de>
> ---
>  Makefile                   | 2 ++
>  contrib/mw-to-git/Makefile | 5 +++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 96f6138f6..19ca5e8de 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2011,6 +2011,8 @@ GIT-PERL-DEFINES: FORCE
>  		echo "$$FLAGS" >$@; \
>  	    fi
>  
> +perllibdir:
> +	@echo '$(perllibdir_SQ)'

Sorry for not noticing it before, but as this rule will not create
and update timestamp of a filesystem entity 'perllibdir', shouldn't
we mark it with .PHONY?  I'd call the target 'say-perllibdir' if I
were doing this patch but that is merely a personal preference.

>  .PHONY: gitweb
>  gitweb:
> diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile
> index a4b6f7a2c..4e603512a 100644
> --- a/contrib/mw-to-git/Makefile
> +++ b/contrib/mw-to-git/Makefile
> @@ -21,8 +21,9 @@ HERE=contrib/mw-to-git/
>  INSTALL = install
>  
>  SCRIPT_PERL_FULL=$(patsubst %,$(HERE)/%,$(SCRIPT_PERL))
> -INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/perl \
> -                -s --no-print-directory instlibdir)
> +INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/ \
> +                -s --no-print-directory prefix=$(prefix) \
> +                perllibdir=$(perllibdir) perllibdir)
>  DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
>  INSTLIBDIR_SQ = $(subst ','\'',$(INSTLIBDIR))
>  

[PATCH v3 1/1] perl: fix installing modules from contrib

[Christian Hesse]

Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
removed a target that allowed Makefiles from contrib/ to get the correct
install path. This introduces a new target for main Makefile and fixes
installation for Mediawiki module.

v2: Pass prefix as that can have influence as well, add single quotes
    for _SQ variant.
v3: Rename target, add to .PHONY.

Signed-off-by: Christian Hesse <mail@eworm.de>
---
 Makefile                   | 3 +++
 contrib/mw-to-git/Makefile | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f18168725..5a06eddf2 100644
--- a/Makefile
+++ b/Makefile
@@ -2014,6 +2014,9 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+.PHONY: say-perllibdir
+say-perllibdir:
+	@echo '$(perllibdir_SQ)'
 
 .PHONY: gitweb
 gitweb:
diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile
index a4b6f7a2c..e301a5b4e 100644
--- a/contrib/mw-to-git/Makefile
+++ b/contrib/mw-to-git/Makefile
@@ -21,8 +21,9 @@ HERE=contrib/mw-to-git/
 INSTALL = install
 
 SCRIPT_PERL_FULL=$(patsubst %,$(HERE)/%,$(SCRIPT_PERL))
-INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/perl \
-                -s --no-print-directory instlibdir)
+INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/ \
+                -s --no-print-directory prefix=$(prefix) \
+                perllibdir=$(perllibdir) say-perllibdir)
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 INSTLIBDIR_SQ = $(subst ','\'',$(INSTLIBDIR))
 

Re: [PATCH v3 1/1] perl: fix installing modules from contrib

[Junio C Hamano]

Christian Hesse <mail@eworm.de> writes:

> Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
> removed a target that allowed Makefiles from contrib/ to get the correct
> install path. This introduces a new target for main Makefile and fixes
> installation for Mediawiki module.
>
> v2: Pass prefix as that can have influence as well, add single quotes
>     for _SQ variant.
> v3: Rename target, add to .PHONY.
>
> Signed-off-by: Christian Hesse <mail@eworm.de>
> ---

Thanks for rerolling.  I should have made it a bit more clear that
the say-* thing was merely a personal preference "I would be writing
it that way if I were doing it", not a "You should write it this way
when working on this project".  I think .PHONY is still a good idea
to have, even for only its documentation value (it is unlikely that
anybody would create a file "perllibdir").

Let me queue this on top of the v2 queued in 'next' as an
incremental update.

Thanks.

-- >8 --
From: Christian Hesse <mail@eworm.de>
SUbject: Makefile: mark perllibdir as a .PHONY target

This target should be marked as .PHONY, just like other targets that
exist only for their side effects that do not create filesystem
entities with the same name.

Signed-off-by: Christian Hesse <mail@eworm.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 75b9ad3b48..b284eb20aa 100644
--- a/Makefile
+++ b/Makefile
@@ -1973,6 +1973,7 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+.PHONY: perllibdir
 perllibdir:
 	@echo '$(perllibdir_SQ)'
 

Re: [PATCH v3 1/1] perl: fix installing modules from contrib

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

Junio C Hamano <gitster@pobox.com> on Thu, 2018/04/19 06:44:
> Christian Hesse <mail@eworm.de> writes:
> 
> > Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make
> > rules) removed a target that allowed Makefiles from contrib/ to get the
> > correct install path. This introduces a new target for main Makefile and
> > fixes installation for Mediawiki module.
> >
> > v2: Pass prefix as that can have influence as well, add single quotes
> >     for _SQ variant.
> > v3: Rename target, add to .PHONY.
> >
> > Signed-off-by: Christian Hesse <mail@eworm.de>
> > ---  
> 
> Thanks for rerolling.  I should have made it a bit more clear that
> the say-* thing was merely a personal preference "I would be writing
> it that way if I were doing it", not a "You should write it this way
> when working on this project". 

Well,  it's you who maintains the code. So I am fine with whatever you
prefer. ;)

> I think .PHONY is still a good idea
> to have, even for only its documentation value (it is unlikely that
> anybody would create a file "perllibdir").
> 
> Let me queue this on top of the v2 queued in 'next' as an
> incremental update.

Thanks a lot!
-- 
Best regards,
Chris

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]