| Message ID | pull.938.v3.git.git.1728764613835.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] diff-highlight: make install link into DESTDIR | expand |
On Sat, Oct 12, 2024 at 08:23:33PM +0000, immeëmosol via GitGitGadget wrote: > diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile > index f2be7cc9243..a53e09e0bdd 100644 > --- a/contrib/diff-highlight/Makefile > +++ b/contrib/diff-highlight/Makefile > @@ -10,6 +10,9 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl > chmod +x $@+ > mv $@+ $@ > > +install: diff-highlight > + test -w $(DESTDIR) && ln -s $(abspath $<) $(DESTDIR) > + I'm not opposed to having an install target here, like we do in the main Makefile and in a few other contrib directories. But in that case, I think it should behave more like those other targets: 1. Actually copy the program rather than making a symlink. Preferably using $(INSTALL). 2. Respect $(prefix) in the usual way. And also... > clean: > + test ! -L $(DESTDIR)/diff-highlight || \ > + $(RM) -f $(DESTDIR)/diff-highlight > $(RM) diff-highlight 3. It's unusual for "clean" to reach outside of the build directory. What you're doing here is more like an "uninstall" target, but we don't usually provide one. There are a few different approaches other contrib/ items take to work like the rest of the Git: - in contrib/contacts, we source config.mak from the top-level, and then define a default $(prefix). This gives some repeated boilerplate, but is pretty independent from the top-level Makefile. - in contrib/credential/netrc, we piggy-back on the top-level Makefile's "install-perl-script", which knows where the user has asked us to install things. That might not be appropriate here, though, as I think it only puts things in libexec/, so "diff-highlight" wouldn't be generally available in the user's $PATH (though it would be enough to use as a pager within git). -Peff
Resending this mail to the list 'cause mail-client added html to previous mail. On Sat, 12 Oct 2024 at 22:55, Jeff King <peff@peff.net> wrote: > > On Sat, Oct 12, 2024 at 08:23:33PM +0000, immeëmosol via GitGitGadget wrote: > > > diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile > > index f2be7cc9243..a53e09e0bdd 100644 > > --- a/contrib/diff-highlight/Makefile > > +++ b/contrib/diff-highlight/Makefile > > @@ -10,6 +10,9 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl > > chmod +x $@+ > > mv $@+ $@ > > > > +install: diff-highlight > > + test -w $(DESTDIR) && ln -s $(abspath $<) $(DESTDIR) > > + > > I'm not opposed to having an install target here, like we do in the main > Makefile and in a few other contrib directories. > > But in that case, I think it should behave more like those other > targets: > > 1. Actually copy the program rather than making a symlink. Preferably > using $(INSTALL). > > 2. Respect $(prefix) in the usual way. > > And also... > > > clean: > > + test ! -L $(DESTDIR)/diff-highlight || \ > > + $(RM) -f $(DESTDIR)/diff-highlight > > $(RM) diff-highlight > > 3. It's unusual for "clean" to reach outside of the build directory. > What you're doing here is more like an "uninstall" target, but we > don't usually provide one. > > There are a few different approaches other contrib/ items take to > work like the rest of the Git: > > - in contrib/contacts, we source config.mak from the top-level, and > then define a default $(prefix). This gives some repeated > boilerplate, but is pretty independent from the top-level Makefile. > > - in contrib/credential/netrc, we piggy-back on the top-level > Makefile's "install-perl-script", which knows where the user has > asked us to install things. That might not be appropriate here, > though, as I think it only puts things in libexec/, so > "diff-highlight" wouldn't be generally available in the user's $PATH > (though it would be enough to use as a pager within git). > > -Peff As mentioned, `contrib/diff-highlight` is less like other perl contribs like `contrib/contacts` and `contrib/credential/netrc`, those two seem to be git subcommands (`git-*`) where diff-highlight is more of a "standalone" command. My usecase was to peek at what the command does by making it available in a `$PATH` writable by a non-root user. (Much like what is mentioned in `contrib/diff-highlight/README#Use`: `git log -p --color | diff-highlight`.= ) ```sh echo '# Given ~/.local/bin is in $PATH,' ( export DESTDIR=3D"${HOME?}/.local/bin/" ; make linked-in-destdir ) echo '# In another already open shell, try suggestion from readme.' ( export DESTDIR=3D"${HOME?}/.local/bin/" ; make clean ) ``` Thanks to all of you for the introduction into how contributions to git/git are handled. Though it has been quite an informative introduction, and i can understand the suggestion of making it a install-target like other contrib-parts, i am currently not spending more time on this. Thanks again, and have a good day= . --- Make git's diff-highlight program immediately available to the command-line= . Create a link in DESTDIR that refers to the generated/concatenated diff-highlight perl script Signed-off-by: imme=C3=ABmosol <will+developer@willfris.nl> --- contrib/diff-highlight/Makefile | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile index f2be7cc9243719..84f6e65c730380 100644 --- a/contrib/diff-highlight/Makefile +++ b/contrib/diff-highlight/Makefile @@ -10,6 +10,11 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl chmod +x $@+ mv $@+ $@ +linked-in-destdir: diff-highlight + test -n "$(DESTDIR)" && \ + test -w $(DESTDIR) && \ + ln -s $(abspath $<) $(DESTDIR) + shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ @@ -17,7 +22,13 @@ shebang.perl: FORCE test: all $(MAKE) -C t -clean: +unlink-from-destdir: + test -z "$(DESTDIR)" || \ + test ! -L $(DESTDIR)/diff-highlight || \ + $(RM) $(DESTDIR)/diff-highlight + +clean: unlink-from-destdir $(RM) diff-highlight .PHONY: FORCE +.PHONY: linked-in-destdir unlink-from-destdir
On 2024-10-13 01:41:06+0200, immeëmosol <will+developer@willfris.nl> wrote: > As mentioned, `contrib/diff-highlight` is less like other perl contribs > like `contrib/contacts` and `contrib/credential/netrc`, those two seem to > be git subcommands (`git-*`) where diff-highlight is more of a "standalone" > command. > > My usecase was to peek at what the command does by making it available in a > `$PATH` writable by a non-root user. (Much like what is mentioned in > `contrib/diff-highlight/README#Use`: `git log -p --color | diff-highlight`.= > ) > > ```sh > echo '# Given ~/.local/bin is in $PATH,' > ( export DESTDIR=3D"${HOME?}/.local/bin/" ; make linked-in-destdir ) > echo '# In another already open shell, try suggestion from readme.' > ( export DESTDIR=3D"${HOME?}/.local/bin/" ; make clean ) > ``` Nah, it isn't DESTDIR's usage, it's prefix job! make prefix=${HOME}/.local install > --- > Make git's diff-highlight program immediately available to the command-line= > . > Create a link in DESTDIR that > refers to the generated/concatenated diff-highlight perl script > > Signed-off-by: imme=C3=ABmosol <will+developer@willfris.nl> > --- > contrib/diff-highlight/Makefile | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/contrib/diff-highlight/Makefile > b/contrib/diff-highlight/Makefile > index f2be7cc9243719..84f6e65c730380 100644 > --- a/contrib/diff-highlight/Makefile > +++ b/contrib/diff-highlight/Makefile > @@ -10,6 +10,11 @@ diff-highlight: shebang.perl DiffHighlight.pm > diff-highlight.perl > chmod +x $@+ > mv $@+ $@ > > +linked-in-destdir: diff-highlight > + test -n "$(DESTDIR)" && \ > + test -w $(DESTDIR) && \ > + ln -s $(abspath $<) $(DESTDIR) So it would be something like this: install: diff-highlight $(INSTALL) diff-highlight '$(DESTDIR)$(bindir_SQ)' > + > shebang.perl: FORCE > @echo '#!$(PERL_PATH_SQ)' >$@+ > @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ > @@ -17,7 +22,13 @@ shebang.perl: FORCE > test: all > $(MAKE) -C t > > -clean: > +unlink-from-destdir: > + test -z "$(DESTDIR)" || \ > + test ! -L $(DESTDIR)/diff-highlight || \ > + $(RM) $(DESTDIR)/diff-highlight > + > +clean: unlink-from-destdir > $(RM) diff-highlight > > .PHONY: FORCE > +.PHONY: linked-in-destdir unlink-from-destdir >
On Sun, Oct 13, 2024 at 01:41:06AM +0200, immeëmosol wrote: > Thanks to all of you for the introduction into how contributions to > git/git are handled. > Though it has been quite an informative introduction, and i can > understand the suggestion of making it a install-target like other > contrib-parts, i am currently not spending more time on this. Thanks > again, and have a good day= It is a bit sad to hear that you do not have time to bring this patch over the finish line, having received some useful pointers from others on the list. Any takers who might want to pick this up instead? Thanks, Taylor
diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile index f2be7cc9243..a53e09e0bdd 100644 --- a/contrib/diff-highlight/Makefile +++ b/contrib/diff-highlight/Makefile @@ -10,6 +10,9 @@ diff-highlight: shebang.perl DiffHighlight.pm diff-highlight.perl chmod +x $@+ mv $@+ $@ +install: diff-highlight + test -w $(DESTDIR) && ln -s $(abspath $<) $(DESTDIR) + shebang.perl: FORCE @echo '#!$(PERL_PATH_SQ)' >$@+ @cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@ @@ -18,6 +21,9 @@ test: all $(MAKE) -C t clean: + test ! -L $(DESTDIR)/diff-highlight || \ + $(RM) -f $(DESTDIR)/diff-highlight $(RM) diff-highlight .PHONY: FORCE +.PHONY: install