1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] docs: ensure outputfile is deleted if rst2html/rst2man fail

Daniel P. Berrangé posted 1 patch 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191211184504.1828156-1-berrange@redhat.com
docs/Makefile.am | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
[libvirt] [PATCH] docs: ensure outputfile is deleted if rst2html/rst2man fail
Posted by Daniel P. Berrangé 4 years, 4 months ago 
This avoids leaving a zero length or partially generated output
file on errors.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/Makefile.am | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/Makefile.am b/docs/Makefile.am
index eb8de80b9c..9a1f7a6117 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -259,21 +259,21 @@ man8_MANS = $(manpages8_rst:%.rst=%.8)
 	   grep -v '^\.\. contents::' < $< | \
 	   sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
 	       -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
-	   $(RST2MAN) > $@
+	   $(RST2MAN) > $@ || { rm $@ && exit 1; }
 
 %.7: %.rst
 	$(AM_V_GEN)$(MKDIR_P) `dirname $@` && \
 	   grep -v '^\.\. contents::' < $< | \
 	   sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
 	       -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
-	   $(RST2MAN) > $@
+	   $(RST2MAN) > $@ || { rm $@ && exit 1; }
 
 %.8: %.rst
 	$(AM_V_GEN)$(MKDIR_P) `dirname $@` && \
 	   grep -v '^\.\. contents::' < $< | \
 	   sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
 	       -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
-	   $(RST2MAN) > $@
+	   $(RST2MAN) > $@ || { rm $@ && exit 1; }
 
 manpages/virkeycode-%.rst: $(top_srcdir)/src/keycodemapdb/data/keymaps.csv \
 		$(top_srcdir)/src/keycodemapdb/tools/keymap-gen Makefile.am
@@ -414,11 +414,11 @@ manpages/%.html.in: manpages/%.rst
 	 grep -v '^:Manual ' < $< | \
 	  sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
 	     -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
-	  $(RST2HTML) > $@
+	  $(RST2HTML) > $@ || { rm $@ && exit 1; }
 
 %.html.in: %.rst
 	$(AM_V_GEN)$(MKDIR_P) `dirname $@` && \
-	  $(RST2HTML) $< > $@
+	  $(RST2HTML) $< > $@ || { rm $@ && exit 1; }
 
 %.html.tmp: %.html.in site.xsl subsite.xsl page.xsl \
 		$(acl_generated)
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: ensure outputfile is deleted if rst2html/rst2man fail
Posted by Eric Blake 4 years, 4 months ago 
On 12/11/19 12:45 PM, Daniel P. Berrangé wrote:
> This avoids leaving a zero length or partially generated output
> file on errors.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   docs/Makefile.am | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index eb8de80b9c..9a1f7a6117 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -259,21 +259,21 @@ man8_MANS = $(manpages8_rst:%.rst=%.8)
>   	   grep -v '^\.\. contents::' < $< | \
>   	   sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
>   	       -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
> -	   $(RST2MAN) > $@
> +	   $(RST2MAN) > $@ || { rm $@ && exit 1; }

But still allows a truncated view of the file if another process 
accesses the file while RST2MAN is still running.  Even better is to 
generate output to a temp file then atomically mv it into place, so that 
no concurrent process can ever see an incomplete file.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: ensure outputfile is deleted if rst2html/rst2man fail
Posted by Daniel P. Berrangé 4 years, 4 months ago 
On Wed, Dec 11, 2019 at 12:52:20PM -0600, Eric Blake wrote:
> On 12/11/19 12:45 PM, Daniel P. Berrangé wrote:
> > This avoids leaving a zero length or partially generated output
> > file on errors.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   docs/Makefile.am | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/docs/Makefile.am b/docs/Makefile.am
> > index eb8de80b9c..9a1f7a6117 100644
> > --- a/docs/Makefile.am
> > +++ b/docs/Makefile.am
> > @@ -259,21 +259,21 @@ man8_MANS = $(manpages8_rst:%.rst=%.8)
> >   	   grep -v '^\.\. contents::' < $< | \
> >   	   sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
> >   	       -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
> > -	   $(RST2MAN) > $@
> > +	   $(RST2MAN) > $@ || { rm $@ && exit 1; }
> 
> But still allows a truncated view of the file if another process accesses
> the file while RST2MAN is still running.  Even better is to generate output
> to a temp file then atomically mv it into place, so that no concurrent
> process can ever see an incomplete file.

Is that a problem that actually impacts negatively in real world for builds?
In various places in the make rules, we've either done the "|| rm" approach,
or the temp file and rename approach. Personally I find the recipes using
the "|| rm" approach are more maintainable/readable.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: ensure outputfile is deleted if rst2html/rst2man fail
Posted by Eric Blake 4 years, 4 months ago 
On 12/11/19 12:59 PM, Daniel P. Berrangé wrote:
> On Wed, Dec 11, 2019 at 12:52:20PM -0600, Eric Blake wrote:
>> On 12/11/19 12:45 PM, Daniel P. Berrangé wrote:
>>> This avoids leaving a zero length or partially generated output
>>> file on errors.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    docs/Makefile.am | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/Makefile.am b/docs/Makefile.am
>>> index eb8de80b9c..9a1f7a6117 100644
>>> --- a/docs/Makefile.am
>>> +++ b/docs/Makefile.am
>>> @@ -259,21 +259,21 @@ man8_MANS = $(manpages8_rst:%.rst=%.8)
>>>    	   grep -v '^\.\. contents::' < $< | \
>>>    	   sed -e 's|SYSCONFDIR|$(sysconfdir)|g' \
>>>    	       -e 's|RUNSTATEDIR|$(runstatedir)|g' | \
>>> -	   $(RST2MAN) > $@
>>> +	   $(RST2MAN) > $@ || { rm $@ && exit 1; }
>>
>> But still allows a truncated view of the file if another process accesses
>> the file while RST2MAN is still running.  Even better is to generate output
>> to a temp file then atomically mv it into place, so that no concurrent
>> process can ever see an incomplete file.
> 
> Is that a problem that actually impacts negatively in real world for builds?
> In various places in the make rules, we've either done the "|| rm" approach,
> or the temp file and rename approach. Personally I find the recipes using
> the "|| rm" approach are more maintainable/readable.

If you're trying to browse the docs and build at the same time, such as 
checking if your tweak to a doc renders well, you could hit it; but 
you're also right that it's not a show-stopper problem.  Go with 
whatever is easier to maintain if you don't care about the issue of 
atomicity in the files (the rm to avoid a long-term corruption is more 
important than the atomic mv to avoid a short-term corruption).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.