Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix make succeeding when the recipe fails #135

Closed
wants to merge 1 commit into from

Conversation

cbrt64
Copy link

@cbrt64 cbrt64 commented Feb 6, 2022

The current doc/Makefile has "successfully" created empty man pages on (Arch-ish) MSYS2. See the original issue here.

Without a working txt2tags, the recipes that generate the grml-zsh-config docs print an error, but make succeeds anyway. This is because the error returned to make is from the last tool in the pipeline, regardless of any failing or even absent tools preceding; see here.

Also, redirecting a failing command to target (i.e. > $@) even without a pipeline is problematic without the GNUish .DELETE_ON_ERROR: rule, see here (empty build targets are generated, and so will mistakenly succeed on next invocation of make).

To mitigate both these case, I went with this approach. These changes could affect portability in ways I'm not aware, if so please advise.

@ft
Copy link
Member

ft commented Feb 6, 2022

Thanks!

I don't think we should be adding --quiet to txt2tags for no reason.

For the actual fix, I think the proper solution is to make txt2tags generate an intermediate file, that the final file can then put a dependency onto. That's because the -i option of GNU sed is non-portable. The intermediate file generation could be done in the same target as well, but I think with make it's more natural to just add another target that does it.

@mika
Copy link
Member

mika commented Feb 7, 2022

Thanks @cbrt64 for bringing this to our attention and even providing a PR! 👍

@ft your suggestion sounds good to me, is anyone capable of providing such an implementation? :)

@ft
Copy link
Member

ft commented Feb 7, 2022

I'd go for this:

MANPAGE = grmlzshrc.5
HTMLPAGE = grmlzshrc.html

all: $(MANPAGE) $(HTMLPAGE)

$(MANPAGE)-pre: grmlzshrc.t2t
	txt2tags --target man -o$@ $<

$(MANPAGE): $(MANPAGE)-pre
	sed -e '/^$$/d' -e 's/^\\e$$//' < $< > $@

$(HTMLPAGE)-pre: grmlzshrc.t2t
	txt2tags --target html --style t2t-modern.css -o$@ $<

$(HTMLPAGE): $(HTMLPAGE)-pre
	sed -e '/^$$/d' -e 's/^\\$$//' < $< > $@

clean:
	rm -f *.5 *.html *.gz *-pre *~

online: all
	scp grmlzshrc.html t2t-modern.css grml:/var/www/grml/zsh/

.PHONY: all clean online

@ft
Copy link
Member

ft commented Feb 7, 2022

Or maybe this, which is more in line with the previous style:

MANPAGES = grmlzshrc.5
HTMLPAGES = grmlzshrc.html

all: $(MANPAGES) $(HTMLPAGES)

.SUFFIXES:
.SUFFIXES: .t2t .5 .html

.t2t.5:
	txt2tags --target man -o$@-pre $<
	sed -e '/^$$/d' -e 's/^\\e$$//' < $@-pre > $@

.t2t.html:
	txt2tags --target html --style t2t-modern.css -o$@-pre $<
	sed -e '/^$$/d' -e 's/^\\$$//' < $@-pre > $@

clean:
	rm -f *.5 *.html *.gz *-pre *~

online: all
	scp grmlzshrc.html t2t-modern.css grml:/var/www/grml/zsh/

.PHONY: all clean online

@cbrt64
Copy link
Author

cbrt64 commented Feb 7, 2022

is anyone capable of providing such an implementation? :)

Not me! The update I was trying for was kinda ugly; I'm just a hobbyist, about a level up from script kiddie wrt build utils, so thanks @ft for writing those up. I'd of course prefer a maintainer to decide which format to go with; somebody taking over is fine too. If I need to do anything further please tell me.

Two things to point out:

  • There is much more build info printed this way, I assume that's desired behavior?
  • When I tested, both @ft's submissions work, except sed failing will allow make to "succeed" on a second run (because > $@ always works). This claims mv foo $@ at the end of a recipe is kosher, e.g.
-	sed -e '/^$$/d' -e 's/^\\$$//' < $@-pre > $@
+	sed -e '/^$$/d' -e 's/^\\$$//' < $@-pre > $@~
+	mv $@~ $@

@ft
Copy link
Member

ft commented Feb 8, 2022

Two things to point out:

* There is much more build info printed this way, I assume that's desired behavior?

It was, since it makes things more transparent. But I've made a few additions since, that obscure what's going one, so I went back to the @printf … scheme.

* When I tested, both @ft's submissions work, except sed failing will allow make to "succeed" on a second run (because `> $@` always works). [This](https://unix.stackexchange.com/a/16180) claims `mv foo $@` at the end of a recipe is kosher, e.g.
-	sed -e '/^$$/d' -e 's/^\\$$//' < $@-pre > $@
+	sed -e '/^$$/d' -e 's/^\\$$//' < $@-pre > $@~
+	mv $@~ $@

While it's very uncommon for sed to fail when called like this, the point it valid. In make only timestamps matter. And the shell will create a file and update its timestamp upon > … no matter if the program driving the redirected data fails or not.

I'd prefer a suffix like .tmp since *~ is a common editor backup file. So here's an update, that also cleans up the intermediate file. I've also actually added --quiet since otherwise the output of txt2tags uses the intermediate file's name, which in this stripped down form would now be confusing.

MANPAGES = grmlzshrc.5
HTMLPAGES = grmlzshrc.html

all: $(MANPAGES) $(HTMLPAGES)

.SUFFIXES:
.SUFFIXES: .t2t .5 .html

.t2t.5:
	@printf 'TXT2TAGS %s\n' "$@"
	@txt2tags --quiet --target man -o$@.pre $<
	@sed -e '/^$$/d' -e 's/^\\e$$//' < $@.pre > $@.tmp
	@rm $@.pre
	@mv $@.tmp $@

.t2t.html:
	@printf 'TXT2TAGS %s\n' "$@"
	@txt2tags --quiet --target html --style t2t-modern.css -o$@.pre $<
	@sed -e '/^$$/d' -e 's/^\\$$//' < $@.pre > $@.tmp
	@rm $@.pre
	@mv $@.tmp $@

clean:
	rm -f *.5 *.html *.gz *.pre *.tmp *~

online: all
	scp grmlzshrc.html t2t-modern.css grml:/var/www/grml/zsh/

.PHONY: all clean online

I can commit this version to master if nobody objects.

@mika
Copy link
Member

mika commented Feb 8, 2022

@ft no objections against committing this to master! Please also consider giving credits to cbrt64 in your commit message. :)
Thanks for taking care! 🤗

@ft
Copy link
Member

ft commented Feb 8, 2022

Don't worry, I'm not forgetting proper credit. ;)

mika pushed a commit that referenced this pull request Feb 8, 2022
In make only timestamps and return values matter. And the shell will create a
file and update its timestamp when it sees an output redirection. No matter if
the program driving the redirected data fails or not.

So even if the first generator run fails, rerunning make will work, with things
looking like they worked out just fine.

This fixes the issue by running the separate steps toward file generation one
by one.

This issue was brought up by Edward E. (cbrt64 on github) in PR#135 on github¹,
including a proof-of-concept implementation, as well as valuable feedback. This
implementation was derived from it. See the pull-request log for details.

¹ #135
@ft
Copy link
Member

ft commented Feb 8, 2022

Pushed as 4a3593b to master. Closing this PR therefore. Thanks your help!

@ft ft closed this Feb 8, 2022
@cbrt64
Copy link
Author

cbrt64 commented Feb 8, 2022

Cool beans! Thanks for the attention to detail and the quick turnaround, especially across time zones. One doesn't get that all the time. (pun intended :)
Which reminds me, I haven't set my GH location, how rude.

@cbrt64 cbrt64 deleted the fix-make-continuing-on-failure branch February 8, 2022 20:15
bhoppi pushed a commit to bhoppi/grml-etc-core that referenced this pull request Dec 8, 2022
In make only timestamps and return values matter. And the shell will create a
file and update its timestamp when it sees an output redirection. No matter if
the program driving the redirected data fails or not.

So even if the first generator run fails, rerunning make will work, with things
looking like they worked out just fine.

This fixes the issue by running the separate steps toward file generation one
by one.

This issue was brought up by Edward E. (cbrt64 on github) in PR#135 on github¹,
including a proof-of-concept implementation, as well as valuable feedback. This
implementation was derived from it. See the pull-request log for details.

¹ grml#135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants