<div dir="ltr">On Wed, Mar 11, 2015 at 11:13 PM, Bryce Harrington <span dir="ltr"><<a href="mailto:bryce@osg.samsung.com" target="_blank">bryce@osg.samsung.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On Wed, Mar 11, 2015 at 03:29:26PM +0100, Andrea Canciani wrote:<br>
> The make-cairo-test-constructors.sh script executes several commands<br>
> without checking their success. This can lead to undetected errors,<br>
> like those fixed in 86fad78fcd2bf987249890aea4eabcce02a58f45.<br>
><br>
> Setting the '-e' flag and writing to a temporary file ensures that the<br>
> 'cairo-test-constructors.c' target completes succesfully only if no<br>
> error occurred.<br>
> ---<br>
>  test/Makefile.am                     | 3 ++-<br>
>  test/make-cairo-test-constructors.sh | 2 ++<br>
>  2 files changed, 4 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/test/Makefile.am b/test/Makefile.am<br>
> index 950629b..a56e7ff 100644<br>
> --- a/test/Makefile.am<br>
> +++ b/test/Makefile.am<br>
> @@ -81,7 +81,8 @@ noinst_SCRIPTS = check-refs.sh<br>
>  TESTS += cairo-test-suite$(EXEEXT)<br>
><br>
>  cairo-test-constructors.c: Makefile $(test_sources) make-cairo-test-constructors.sh<br>
> -     (cd $(srcdir) && sh ./make-cairo-test-constructors.sh $(test_sources)) > $@<br>
> +     (cd $(srcdir) && sh ./make-cairo-test-constructors.sh $(test_sources)) > $@.tmp<br>
> +     mv $@.tmp $@<br>
<br>
</span>This will properly avoid overwriting $@ if the script fails, and make<br>
will bail at that point.  Does $@.tmp get created in that case?  If so,<br>
does anything clean that stray file up?  Would mktemp be a cleaner<br>
solution maybe?<br></blockquote><div><br></div><div>The $@.tmp file is generated both if the make-cairo-test-constructors.sh script succeeds and if it fails; in the first case it is renamed to $@, but upon error it stays and it is not cleaned up.<br></div><div>AFAICT mktemp would not address the issue, as it would only generate a unique filename, but it would not ensure that the file is eventually removed.<br></div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
>  cairo_test_suite_SOURCES =           \<br>
>       $(cairo_test_suite_sources)     \<br>
> diff --git a/test/make-cairo-test-constructors.sh b/test/make-cairo-test-constructors.sh<br>
> index cb1391e..9b78eba 100644<br>
> --- a/test/make-cairo-test-constructors.sh<br>
> +++ b/test/make-cairo-test-constructors.sh<br>
> @@ -1,5 +1,7 @@<br>
>  #! /bin/sh<br>
><br>
> +set -e<br>
> +<br>
<br>
</span>Can you verify that your sed exits with non-zero exit code in the<br>
86fad78f case?<br></blockquote><div><br></div><div>Yes, I tested that.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
>  if test $# -eq 0; then<br>
>      echo "$0: no input files." >&2<br>
>      exit 0<br>
<br>
</span>Shouldn't this exit 1 since it's another error condition?<br></blockquote><div><br></div><div>Yes, I think that's much better.<br><br>I just sent in a revised patch that should address your comments.<br><br></div><div>Andrea<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><font color="#888888"><br>
> --<br>
> 1.9.5 (Apple Git-50.3)<br>
><br>
> --<br>
> cairo mailing list<br>
> <a href="mailto:cairo@cairographics.org">cairo@cairographics.org</a><br>
> <a href="http://lists.cairographics.org/mailman/listinfo/cairo" target="_blank">http://lists.cairographics.org/mailman/listinfo/cairo</a><br>
</font></span></blockquote></div><br></div></div>