This was "just for fun," but it seems to work and let me delete a lot of code. I dare say it's even a tiny bit more understandable than before.
Created attachment 574592 [details, diff] 0001-GNUmakefile-rename-and-simplify.patch
Ping, this should be somewhat "safe" since it isn't user-facing and you can just revert it if it causes an unforeseen problem. I've got another patch floating around for the makefile and it merging this would let me rebase that one.
Created attachment 596298 [details, diff] 0001-GNUmakefile-rename-and-simplify.patch Rebased onto master to avoid a conflict with bc9e43f1d (which I already fixed with this patch, in April, if anyone wants to take a look).
@mgorny: sorry to bother you in every situation like this, but this needs a technical review, and otherwise it will sit here forever. I'm not adamant about renaming the file, although it seems like the right thing to do. Both the old and the new version require GNU make.
I think it makes sense but I defer to ulm. That is, the change in content. I don't think the rename makes any sense. It would make sense if we were to supply additional 'legacy' Makefiles but we don't.
(In reply to Michał Górny from comment #5) > > I don't think the rename makes any sense. It would make sense if we were to > supply additional 'legacy' Makefiles but we don't. My reasoning was that this will produce a better error message if someone actually tries to use a non-GNU make. The best thing that could happen is a harmless crash, but it also seems possible (I've never used a non-GNU make) that another implementation could misinterpret one of the GNU extensions and start firing off dangerous commands. But in any case, I'll just rename it again if people prefer "Makefile".
I think as well that the renaming is pointless. As for the changes, can you please attach a real diff (i.e. only showing those lines that have actually changed)? Have you tested these changes with app-doc/devmanual-9999? Preliminary review (I've just looked at the code, not actually tried to run it): +HTMLS := $(subst text.xml,index.html,$(shell find ./ -type f -name 'text.xml')) +IMAGES := $(patsubst %.svg,%.png,$(shell find ./ -type f -name '*.svg')) Why the ./ in find? The current directory is the default anyway. + +all: prereq $(HTMLS) $(IMAGES) + +prereq: + @type convert >/dev/null 2>&1 || { echo "media-gfx/imagemagick with corefonts, svg and truetype required" >&2; exit 1; }; \ + type xsltproc >/dev/null 2>&1 || { echo "dev-libs/libxslt is required" >&2; exit 1; } The continuation line is not necessary, it would be more readable as two separate commands. While at it, use a reasonable line length. + + +%.png : %.svg + convert $< $@ + +clean: + rm -f $(HTMLS) $(IMAGES) + +.PHONY: all prereq clean Move clean and .PHONY targets to the end of the file please. + + +# Secondary expansion allows us to use the automatic variable $@ in +# the prerequisites. When it is used (and we have no idea when that +# is, so we assume always) our <include href="foo"> tag induces a +# dependency on the output of all subdirectories of the current +# directories. This wacky rule finds all of those subdirectories by +# looking for text.xml in them, and then replaces "text.xml" in the +# path with "index.html". +# +# We use the pattern %.html rather than the more-sensible %index.html +# because the latter doesn't match our top-level index.html target. +# +.SECONDEXPANSION: +%.html: $$(dir $$@)text.xml devbook.xsl xsl/*.xsl $$(subst text.xml,index.html,$$(wildcard $$(dir $$@)*/text.xml)) + xsltproc devbook.xsl $< > $@
(In reply to Ulrich Müller from comment #7) > I think as well that the renaming is pointless. As for the changes, can you > please attach a real diff (i.e. only showing those lines that have actually > changed)? > > Have you tested these changes with app-doc/devmanual-9999? > > Preliminary review (I've just looked at the code, not actually tried to run > it): > > +HTMLS := $(subst text.xml,index.html,$(shell find ./ -type f -name > 'text.xml')) > +IMAGES := $(patsubst %.svg,%.png,$(shell find ./ -type f -name '*.svg')) > > Why the ./ in find? The current directory is the default anyway. You're thinking GNU. POSIX requires the path argument, and so does e.g. NetBSD.
Created attachment 596366 [details, diff] 0001-Makefile-simplify-generated-rules.patch
> I think as well that the renaming is pointless. Undone. > Have you tested these changes with app-doc/devmanual-9999? Yes, the patch is against master. > Why the ./ in find? The current directory is the default anyway. Left this alone per mgorny's comment. > The continuation line is not necessary, it would be more readable as two separate commands. While at it, use a reasonable line length. Agreed, done. > Move clean and .PHONY targets to the end of the file please. I think I left "clean" there so that the diff would show it properly, but I prefer it at the end of the file as well. Moved.
(In reply to Michael Orlitzky from comment #10) > > Why the ./ in find? The current directory is the default anyway. > > Left this alone per mgorny's comment. If we really consider non-GNU find, then for best portability "-print" should be added as well. There are find implementations where it isn't the default.
(In reply to Ulrich Müller from comment #11) > (In reply to Michael Orlitzky from comment #10) > > > Why the ./ in find? The current directory is the default anyway. > > > > Left this alone per mgorny's comment. > > If we really consider non-GNU find, then for best portability "-print" > should be added as well. There are find implementations where it isn't the > default. POSIX is pretty clear that -print is the default: If no expression is present, -print shall be used as the expression. Otherwise, if the given expression does not contain any of the primaries -exec, -ok, or -print, the given expression shall be effectively replaced by: ( given_expression ) -print I would normally not care either way, except that adding a "-print" to the find commands will make them wrap at 80 chars and make the file a tiny bit uglier. Do you remember which "find" implementations don't follow POSIX? If it's something like OpenBSD, I would be inclined to support them. But if it's one version of Cygwin from 1995, then I'd rather keep the file tidy.
(In reply to Michael Orlitzky from comment #12) No strong opinion on that one. I hadn't brought up non-GNU find, in the first place. :)
Created attachment 596858 [details, diff] 0001-Makefile-simplify-generated-rules.patch Here's one more version that excludes ".git" from the two "find" commands for performance/correctness.
Is it possible at all to accomplish this functionality without using the GNU extensions? I'm asking out of curiosity. Otherwise I'm ok with this patch. I'll merge it now because the current Makefile is not adding xsl/* as deps (which this patch fixes) and I don't want to cause a rebase by fixing that separately.
(In reply to Göktürk Yüksek from comment #15) > Is it possible at all to accomplish this functionality without using the GNU > extensions? I'm asking out of curiosity. Otherwise I'm ok with this patch. What are you willing to sacrifice? If you look in e.g. ebuild-writing/text.xml, you'll see... <include href="file-format/"/> <include href="eapi/"/> <include href="use-conditional-code/"/> ... which is creating a dependency on those referenced documents. We've already declared the dependency once, but... it's in a string, in an XML file. How do you convey the same information to the build system? Option 1: you tell it. Simple, easy, and requires no GNU extensions, but you have to duplicate the information contained in the XML files, and keep the two copies synchronized for eternity. Option 2: you do something insane that approximately correctly generates the list of dependencies based on a heuristic. We already had option 2, so I went with an improved option 2. Ridiculousness aside, this option is probably more reliable than trusting people to update the Makefile when adding or deleting a page. The GNU extensions are needed for this unless someone more clever than I cares to prove me wrong. Option 3: you parse the XML properly in something akin to a ./configure step, and output the Makefile as a result. Robust, but involves overhauling the build system.
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/proj/devmanual.git/commit/?id=3c2491a9e3018d1fc98feaf5d3b91c4ad2e7013a commit 3c2491a9e3018d1fc98feaf5d3b91c4ad2e7013a Author: Michael Orlitzky <mjo@gentoo.org> AuthorDate: 2019-04-29 01:10:28 +0000 Commit: Göktürk Yüksek <gokturk@gentoo.org> CommitDate: 2019-11-26 02:17:03 +0000 Makefile: simplify generated rules. Our Makefile is GNU-specific, thanks to heavy use of call/eval to automatically generate rules. In this commit, those generated rules have been replaced by slightly less (in terms of code volume) GNU-specific magic. The new magic uses a secondary expansion rule that allows us to access the target name inside its own prerequisites. The single resulting SECONDEXPANSION rule seems to be able to replace all of the generated rules. The variables and "clean" targets were also simplified using the full power of the GNU extensions, which we are assuming anyway. Finally, two other minor improvements were made: 1. The "clean" target was moved to the end of the file. 2. The "prereq" rule was split into two separate commands, and wrapped to a reasonable line length. Closes: https://bugs.gentoo.org/684688 Signed-off-by: Michael Orlitzky <mjo@gentoo.org> Signed-off-by: Göktürk Yüksek <gokturk@gentoo.org> Makefile | 60 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 30 insertions(+), 30 deletions(-)
