~sircmpwn/hare-dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
2 2

[PATCH hare-docs] Improve recommended lib.Makefile

Details
Message ID
<20240227090517.31398-2-contact@willowbarraco.fr>
DKIM signature
pass
Download raw message
Patch: +8 -5
I continue to dislike this way of writing install makefiles. When the
"example" folders contains subfolders (most of the time), install will
ommit them. We generally chains install commands, which means we have to
update the Makefile when folders are re-organised.

I found a better way (or at least I prefer it), using a bit more from Makefile.

Signed-off-by: Willow Barraco <contact@willowbarraco.fr>
---
 usage/lib.Makefile | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/usage/lib.Makefile b/usage/lib.Makefile
index 77d293b..ab178f4 100644
--- a/usage/lib.Makefile
+++ b/usage/lib.Makefile
@@ -9,15 +9,18 @@ SRCDIR=$(PREFIX)/src
HARESRCDIR=$(SRCDIR)/hare
THIRDPARTYDIR=$(HARESRCDIR)/third-party

SOURCES := $(wildcard example/**/*) $(wildcard example/*)
DESTS   := $(patsubst example/%,$(DESTDIR)$(THIRDPARTYDIR)/example/%,$(SOURCES))

check:
	$(HARE) test $(HAREFLAGS)

install:
	install -Dm644 example1/* $(DESTDIR)$(THIRDPARTYDIR)/example1/
	install -Dm644 example2/* $(DESTDIR)$(THIRDPARTYDIR)/example2/
$(DESTDIR)$(THIRDPARTYDIR)/example/%: example/%
	install -Dm644 $< $@

install: uninstall $(DESTS)

uninstall:
	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example1
	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example2
	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example

.PHONY: check install uninstall
-- 
2.44.0
Details
Message ID
<ZeCT5RYlCTdInBoq@fluorine>
In-Reply-To
<20240227090517.31398-2-contact@willowbarraco.fr> (view parent)
DKIM signature
pass
Download raw message
Quoth Willow Barraco:
>I continue to dislike this way of writing install makefiles. When the
>"example" folders contains subfolders (most of the time), install will
>ommit them. We generally chains install commands, which means we have to
>update the Makefile when folders are re-organised.

That’s true.  I don’t think it’s a huge problem, but repeating 
oneself like is done right now (in the install and uninstall 
recipes) isn’t very nice.

>I found a better way (or at least I prefer it), using a bit more from Makefile.

Nice!

One thing, though: The features you use to improve the Makefile 
(macro definition by :=, $(functions ), and pattern targets by %) 
are GNU make features only and not specified by POSIX.  So not 
only does your patch improve the Makefile, it also introduces 
dependence on GNU make specifically, which we’d like to avoid.

>Signed-off-by: Willow Barraco <contact@willowbarraco.fr>
>---
> usage/lib.Makefile | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
>diff --git a/usage/lib.Makefile b/usage/lib.Makefile
>index 77d293b..ab178f4 100644
>--- a/usage/lib.Makefile
>+++ b/usage/lib.Makefile
>@@ -9,15 +9,18 @@ SRCDIR=$(PREFIX)/src
> HARESRCDIR=$(SRCDIR)/hare
> THIRDPARTYDIR=$(HARESRCDIR)/third-party
>
>+SOURCES := $(wildcard example/**/*) $(wildcard example/*)
>+DESTS   := $(patsubst example/%,$(DESTDIR)$(THIRDPARTYDIR)/example/%,$(SOURCES))

So, in POSIX, there is no $(wildcard ) and no $(patsubst ).  Two 
options I see are

   • hardcoding all paths.  If we copy recursively, we only need 
the top-level module directories.

	SOURCES = example1 example2

This is my suggestion.

   • relying on a POSIX.1-202x feature: macro definition by !=.  
``a != b`` defines a as the output of the command b.  If we were 
to need all the source files and not just the top-level 
directories, we could use find or the like:

	SOURCES != find . -type f \( -name '*.ha' -o -name README \)

A perhaps rather minor point is performance: Both globbing via 
$(wildcard ) and using find on each invocation of make could get 
a little slow with a bunch of source files.

The DESTS macro, too, can be defined in terms of POSIX.1-202x: 
While POSIX.1-2017 only defines $(name:oldsuffix=newsuffix) 
substitutions in macro expansions, POSIX.1-202x introduces 
$(name:oldprefix%oldsuffix=newprefix%newsuffix).

	DESTS ::= $(SOURCES:%=$(DESTDIR)$(THIRDPARTYDIR)/%)

(The ::= is the POSIX.1-202x syntax for macros not to be expanded 
later, similar to :=.  That’s immaterial here.)

>+
> check:
> 	$(HARE) test $(HAREFLAGS)
>
>-install:
>-	install -Dm644 example1/* $(DESTDIR)$(THIRDPARTYDIR)/example1/
>-	install -Dm644 example2/* $(DESTDIR)$(THIRDPARTYDIR)/example2/
>+$(DESTDIR)$(THIRDPARTYDIR)/example/%: example/%
>+	install -Dm644 $< $@
>+
>+install: uninstall $(DESTS)

I too really like this way of writing install targets: simply 
having the installed files as prerequisites.  Alas, even 
POSIX.1-202x doesn’t yet have %-targets.  So while you could have 
a recipe for installing things like this

	$(DESTS):
		rm -rf -- $@
		cp -R -- $$(basename $@) $@

You’d still need to write out

	$(DESTDIR)$(THIRDPARTYDIR)/example1: example1
	$(DESTDIR)$(THIRDPARTYDIR)/example2: example2
	…

But if you do stick to an install target that copies everything by 
itself, you can loop over the things you want to install:

	install: uninstall
		for i in $(SOURCES); \
		do cp -R -- $$i $(DESTDIR)$(THIRDPARTYDIR)/$$i; \
		done

(Then, you also don’t need the DESTS macro.)

The same can be done for the uninstall target.

I like the idea of install having uninstall as prerequisite; 
I didn’t know that one.

>
> uninstall:
>-	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example1
>-	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example2
>+	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example
>
> .PHONY: check install uninstall

If you wish to learn more about POSIX and what it provides: 
POSIX.1-2017 is accessible at 
https://pubs.opengroup.org/onlinepubs/9699919799/ .  The 
definition of make is at Shell & Utilities > Utilities > make, or 
directly at 
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/make.html .  
You might also have that text installed as a man page make(1p).

While POSIX.1-202x isn’t public yet, most discussion happened on 
the public bug tracker at https://austingroupbugs.net/ .
Details
Message ID
<CZHQZYQIW6LQ.D2WVR2C0EBQM@willowbarraco.fr>
In-Reply-To
<ZeCT5RYlCTdInBoq@fluorine> (view parent)
DKIM signature
pass
Download raw message
Thanks a lot for this review. I learnt a lot!

> > check:
> > 	$(HARE) test $(HAREFLAGS)
> >
> >-install:
> >-	install -Dm644 example1/* $(DESTDIR)$(THIRDPARTYDIR)/example1/
> >-	install -Dm644 example2/* $(DESTDIR)$(THIRDPARTYDIR)/example2/
> >+$(DESTDIR)$(THIRDPARTYDIR)/example/%: example/%
> >+	install -Dm644 $< $@
> >+
> >+install: uninstall $(DESTS)
>
> I too really like this way of writing install targets: simply 
> having the installed files as prerequisites.  Alas, even 
> POSIX.1-202x doesn’t yet have %-targets.  So while you could have 
> a recipe for installing things like this
>
> 	$(DESTS):
> 		rm -rf -- $@
> 		cp -R -- $$(basename $@) $@
>
> You’d still need to write out
>
> 	$(DESTDIR)$(THIRDPARTYDIR)/example1: example1
> 	$(DESTDIR)$(THIRDPARTYDIR)/example2: example2
> 	…
>
> But if you do stick to an install target that copies everything by 
> itself, you can loop over the things you want to install:
>
> 	install: uninstall
> 		for i in $(SOURCES); \
> 		do cp -R -- $$i $(DESTDIR)$(THIRDPARTYDIR)/$$i; \
> 		done
>
> (Then, you also don’t need the DESTS macro.)

I think the whole point of my approach was to simplify this. If we are
sticking to POSIX, I prefer the status quo.

> The same can be done for the uninstall target.
>
> I like the idea of install having uninstall as prerequisite; 
> I didn’t know that one.
>
> >
> > uninstall:
> >-	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example1
> >-	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example2
> >+	rm -rf $(DESTDIR)$(THIRDPARTYDIR)/example
> >
> > .PHONY: check install uninstall
>
> If you wish to learn more about POSIX and what it provides: 
> POSIX.1-2017 is accessible at 
> https://pubs.opengroup.org/onlinepubs/9699919799/ .  The 
> definition of make is at Shell & Utilities > Utilities > make, or 
> directly at 
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/make.html .  
> You might also have that text installed as a man page make(1p).
>
> While POSIX.1-202x isn’t public yet, most discussion happened on 
> the public bug tracker at https://austingroupbugs.net/ .

I wasn't even aware Make was part of POSIX. The scope seems much
larger than I thought it was. Thanks for pointing at the doc.

I will probably continue to use %-target, because this is really
convenient. But should use find, and macro expansion instead of GNU
specific features.
Reply to thread Export thread (mbox)