perl: fix installing modules from contrib

PATCH Superseded Christian Hesse 3 participants 8 messages Apr 3, 2018 – Apr 19, 2018 Kbuild

Share & tools

Christian Hesse

author

2018-04-03 09:26 UTC #0 lore raw

PATCH 1/1 perl: fix installing modules from contrib

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 <[email protected]>

---
 Makefile                   | 2 ++
 contrib/mw-to-git/Makefile | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

Makefile +2 -0

index a1d8775ad..bcaf50495 100644
@@ -2002,6 +2002,8 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+perllibdir:
+	@echo $(perllibdir_SQ)
 
 .PHONY: gitweb
 gitweb:

contrib/mw-to-git/Makefile +2 -2

index a4b6f7a2c..0a6e59579 100644
@@ -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))

Ævar Arnfjörð Bjarmason

2018-04-03 10:50 UTC #1 lore raw

Quoted 29 lines · 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 <[email protected]>
---
 Makefile                   | 2 ++
 contrib/mw-to-git/Makefile | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)
Makefile +2 -0
index a1d8775ad..bcaf50495 100644
@@ -2002,6 +2002,8 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+perllibdir:
+	@echo $(perllibdir_SQ)
 
 .PHONY: gitweb
 gitweb:
contrib/mw-to-git/Makefile +0 -0
index a4b6f7a2c..0a6e59579 100644

contrib/mw-to-git/Makefile HERE=contrib/mw-to-git/ · line 27

Quoted 9 lines

contrib/mw-to-git/Makefile +2 -2

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

Junio C Hamano

2018-04-09 04:54 UTC #2 lore raw

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Makefile GIT-PERL-DEFINES: FORCE · line 2006
Quoted 3 lines
Makefile +2 -0
+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.

Christian Hesse

author

2018-04-10 13:37 UTC #3 lore raw

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

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

Signed-off-by Christian Hesse <[email protected]>

---
 Makefile                   | 2 ++
 contrib/mw-to-git/Makefile | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

Makefile +2 -0

index 96f6138f6..19ca5e8de 100644
@@ -2011,6 +2011,8 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+perllibdir:
+	@echo '$(perllibdir_SQ)'
 
 .PHONY: gitweb
 gitweb:

contrib/mw-to-git/Makefile +3 -2

index a4b6f7a2c..4e603512a 100644
@@ -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))

Junio C Hamano

2018-04-10 21:44 UTC #4 lore raw

Quoted 19 lines · 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 <[email protected]>
---
 Makefile                   | 2 ++
 contrib/mw-to-git/Makefile | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)
Makefile +0 -0
index 96f6138f6..19ca5e8de 100644

Makefile GIT-PERL-DEFINES: FORCE · line 2015

Quoted 5 lines

Makefile +2 -0

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

Quoted 18 lines

Makefile +0 -0

 .PHONY: gitweb
 gitweb:

contrib/mw-to-git/Makefile +3 -2

index a4b6f7a2c..4e603512a 100644
@@ -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))

Christian Hesse

author

2018-04-11 06:51 UTC #5 lore raw

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

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 <[email protected]>

---
 Makefile                   | 3 +++
 contrib/mw-to-git/Makefile | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

Makefile +3 -0

index f18168725..5a06eddf2 100644
@@ -2014,6 +2014,9 @@ GIT-PERL-DEFINES: FORCE
 		echo "$$FLAGS" >$@; \
 	    fi
 
+.PHONY: say-perllibdir
+say-perllibdir:
+	@echo '$(perllibdir_SQ)'
 
 .PHONY: gitweb
 gitweb:

contrib/mw-to-git/Makefile +3 -2

index a4b6f7a2c..e301a5b4e 100644
@@ -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))

Junio C Hamano

2018-04-18 21:44 UTC #6 lore raw

Quoted 11 lines · 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 <[email protected]>
---

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 <[email protected]>
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 <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

Makefile +1 -0

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

Christian Hesse

author

2018-04-19 21:44 UTC #7 lore raw

Junio C Hamano <[email protected]> on Thu, 2018/04/19 06:44:

Quoted 11 lines

Quoted 9 lines · 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.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Christian Hesse <[email protected]>
---
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. ;)

Quoted 6 lines

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!

Signature · 2 lines

Best regards,
Chris