[RFA] Fix gdb snapshots

[RFA] Fix gdb snapshots

[Tom Tromey]

Joel pointed out that gdb snapshots were broken by my Makefile patch
series.  The bug is that rmdir in distclean was failing, because the
directories in question did not exist.  The simplest fix was to just use
"rm -rf", which won't fail if the directory is missing.

Tested using "src-release.sh gdb".

2017-11-29  Tom Tromey  <tom@tromey.com>

	* Makefile.in (distclean): Use "rm -rf", not "rmdir".
---
 gdb/ChangeLog   | 4 ++++
 gdb/Makefile.in | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ebb969998c..7532016499 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-11-29  Tom Tromey  <tom@tromey.com>
+
+	* Makefile.in (distclean): Use "rm -rf", not "rmdir".
+
 2017-11-27  Tom Tromey  <tom@tromey.com>
 
 	* Makefile.in (REMOTE_OBS): Remove.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 6e16bc6682..39f90bad9f 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1995,7 +1995,9 @@ distclean: clean
 	rm -f Makefile
 	rm -rf $(DEPDIR)
 	for i in $(CONFIG_SRC_SUBDIR); do \
-		rmdir $$i/$(DEPDIR); \
+		# Use rm -rf, not rmdir, to avoid errors when the \
+		# directory does not exist. \
+		rm -rf $$i/$(DEPDIR); \
 	done
 
 maintainer-clean: local-maintainer-clean do-maintainer-clean distclean
-- 
2.13.6

Re: [RFA] Fix gdb snapshots

[Simon Marchi]

On 2017-11-29 11:31, Tom Tromey wrote:
> Joel pointed out that gdb snapshots were broken by my Makefile patch
> series.  The bug is that rmdir in distclean was failing, because the
> directories in question did not exist.  The simplest fix was to just 
> use
> "rm -rf", which won't fail if the directory is missing.
> 
> Tested using "src-release.sh gdb".
> 
> 2017-11-29  Tom Tromey  <tom@tromey.com>
> 
> 	* Makefile.in (distclean): Use "rm -rf", not "rmdir".
> ---
>  gdb/ChangeLog   | 4 ++++
>  gdb/Makefile.in | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ebb969998c..7532016499 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-11-29  Tom Tromey  <tom@tromey.com>
> +
> +	* Makefile.in (distclean): Use "rm -rf", not "rmdir".
> +
>  2017-11-27  Tom Tromey  <tom@tromey.com>
> 
>  	* Makefile.in (REMOTE_OBS): Remove.
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 6e16bc6682..39f90bad9f 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1995,7 +1995,9 @@ distclean: clean
>  	rm -f Makefile
>  	rm -rf $(DEPDIR)
>  	for i in $(CONFIG_SRC_SUBDIR); do \
> -		rmdir $$i/$(DEPDIR); \
> +		# Use rm -rf, not rmdir, to avoid errors when the \
> +		# directory does not exist. \
> +		rm -rf $$i/$(DEPDIR); \
>  	done
> 
>  maintainer-clean: local-maintainer-clean do-maintainer-clean distclean

As always, I am really not comfortable with using rm -rf in scripts.

Ref: https://github.com/MrMEEE/bumblebee-Old-and-abbandoned/issues/123

Since we know that the .deps directories will only contain files, can we 
do something like this instead (not tested)?

rm -f $$i/$(DEPDIR)/*
rmdir $$i/$(DEPDIR)

Simon

Re: [RFA] Fix gdb snapshots

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> As always, I am really not comfortable with using rm -rf in scripts.

You'll be disappointed by the current Makefile then :)

Simon> Since we know that the .deps directories will only contain files, can
Simon> we do something like this instead (not tested)?
Simon> rm -f $$i/$(DEPDIR)/*
Simon> rmdir $$i/$(DEPDIR)

The issue is that this rmdir will fail because, in this situation, the
directory does not exist at all.

Perhaps rmdir||true will be more to your liking.

Tom

Re: [RFA] Fix gdb snapshots

[Tom Tromey]

Tom> Perhaps rmdir||true will be more to your liking.

How's this?

Tom

commit ea7e34a8e4a79231ea952f711d02729021cbabd0
Author: Tom Tromey <tom@tromey.com>
Date:   Wed Nov 29 09:27:40 2017 -0700

    Fix gdb snapshots
    
    Joel pointed out that gdb snapshots were broken by my Makefile patch
    series.  The bug is that rmdir in distclean was failing, because the
    directories in question did not exist.  This fixes the problem by
    ignoring errors from rmdir.
    
    Tested using "src-release.sh gdb".
    
    2017-11-29  Tom Tromey  <tom@tromey.com>
    
            * Makefile.in (distclean): Handle the case where rmdir fails.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ebb969998c..dbea503d02 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-11-29  Tom Tromey  <tom@tromey.com>
+
+	* Makefile.in (distclean): Handle the case where rmdir fails.
+
 2017-11-27  Tom Tromey  <tom@tromey.com>
 
 	* Makefile.in (REMOTE_OBS): Remove.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 6e16bc6682..1ecd5a76b3 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1995,7 +1995,7 @@ distclean: clean
 	rm -f Makefile
 	rm -rf $(DEPDIR)
 	for i in $(CONFIG_SRC_SUBDIR); do \
-		rmdir $$i/$(DEPDIR); \
+		rm -rf $$i/$(DEPDIR) || true; \
 	done
 
 maintainer-clean: local-maintainer-clean do-maintainer-clean distclean

Re: [RFA] Fix gdb snapshots

[Simon Marchi]

On 2017-11-29 12:00, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> As always, I am really not comfortable with using rm -rf in 
> scripts.
> 
> You'll be disappointed by the current Makefile then :)
> 
> Simon> Since we know that the .deps directories will only contain 
> files, can
> Simon> we do something like this instead (not tested)?
> Simon> rm -f $$i/$(DEPDIR)/*
> Simon> rmdir $$i/$(DEPDIR)
> 
> The issue is that this rmdir will fail because, in this situation, the
> directory does not exist at all.
> 
> Perhaps rmdir||true will be more to your liking.

It's too bad rmdir does not have a -f switch like rm...  To avoid 
printing an error message (No such file or directory) when the directory 
does not exist, you could do:

[ -d $$i/$(DEPDIR) ] && rmdir $$i/$(DEPDIR)

or

test -d $$i/$(DEPDIR) && $$i/$(DEPDIR)

Simon

Re: [RFA] Fix gdb snapshots

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> -		rmdir $$i/$(DEPDIR); \
Tom> +		rm -rf $$i/$(DEPDIR) || true; \

Simon pointed out I failed to actually make the change.
Haha.

Here's try 3.

Tom

commit c24e0f6a00df51160118c5020d90a1aeb92eefc6
Author: Tom Tromey <tom@tromey.com>
Date:   Wed Nov 29 09:27:40 2017 -0700

    Fix gdb snapshots
    
    Joel pointed out that gdb snapshots were broken by my Makefile patch
    series.  The bug is that rmdir in distclean was failing, because the
    directory did not exist.  This fixes the bug by only invoking rmdir when
    the directory exists.
    
    Tested using "src-release.sh gdb".
    
    2017-11-29  Tom Tromey  <tom@tromey.com>
    
            * Makefile.in (distclean): Handle the case where rmdir fails.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ebb969998c..dbea503d02 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-11-29  Tom Tromey  <tom@tromey.com>
+
+	* Makefile.in (distclean): Handle the case where rmdir fails.
+
 2017-11-27  Tom Tromey  <tom@tromey.com>
 
 	* Makefile.in (REMOTE_OBS): Remove.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 6e16bc6682..284559b030 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1995,7 +1995,7 @@ distclean: clean
 	rm -f Makefile
 	rm -rf $(DEPDIR)
 	for i in $(CONFIG_SRC_SUBDIR); do \
-		rmdir $$i/$(DEPDIR); \
+		if test -d $$i/$(DEPDIR); then rmdir $$i/$(DEPDIR); fi \
 	done
 
 maintainer-clean: local-maintainer-clean do-maintainer-clean distclean

Re: [RFA] Fix gdb snapshots

[Simon Marchi]

On 2017-11-29 12:27, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> -		rmdir $$i/$(DEPDIR); \
> Tom> +		rm -rf $$i/$(DEPDIR) || true; \
> 
> Simon pointed out I failed to actually make the change.
> Haha.
> 
> Here's try 3.
> 
> Tom
> 
> commit c24e0f6a00df51160118c5020d90a1aeb92eefc6
> Author: Tom Tromey <tom@tromey.com>
> Date:   Wed Nov 29 09:27:40 2017 -0700
> 
>     Fix gdb snapshots
> 
>     Joel pointed out that gdb snapshots were broken by my Makefile 
> patch
>     series.  The bug is that rmdir in distclean was failing, because 
> the
>     directory did not exist.  This fixes the bug by only invoking rmdir 
> when
>     the directory exists.
> 
>     Tested using "src-release.sh gdb".
> 
>     2017-11-29  Tom Tromey  <tom@tromey.com>
> 
>             * Makefile.in (distclean): Handle the case where rmdir 
> fails.
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ebb969998c..dbea503d02 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-11-29  Tom Tromey  <tom@tromey.com>
> +
> +	* Makefile.in (distclean): Handle the case where rmdir fails.
> +
>  2017-11-27  Tom Tromey  <tom@tromey.com>
> 
>  	* Makefile.in (REMOTE_OBS): Remove.
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 6e16bc6682..284559b030 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1995,7 +1995,7 @@ distclean: clean
>  	rm -f Makefile
>  	rm -rf $(DEPDIR)
>  	for i in $(CONFIG_SRC_SUBDIR); do \
> -		rmdir $$i/$(DEPDIR); \
> +		if test -d $$i/$(DEPDIR); then rmdir $$i/$(DEPDIR); fi \
>  	done

Since this is in distclean, we know that clean will have ran before, and 
thus .deps will be empty by now, is that right?  In my original reply, I 
got confused and thought that the problem was that .deps was non-empty, 
when the problem was actually that it was not present (my bad for not 
reading carefully your log).

If so, that version with the if looks good to me.  My suggestion to do 
"[ -d ... ] && rmdir ..." would not have worked, because it would have 
returned a non-zero exit code and stopped the execution just like the 
current code.

Thanks,

Simon

Re: [RFA] Fix gdb snapshots

[Tom Tromey]

Simon> Since this is in distclean, we know that clean will have ran before,
Simon> and thus .deps will be empty by now, is that right?

Yes.

Simon> In my original
Simon> reply, I got confused and thought that the problem was that .deps was
Simon> non-empty, when the problem was actually that it was not present (my
Simon> bad for not reading carefully your log).

Thanks, but no big deal, everything is fine.

Simon> If so, that version with the if looks good to me.

I'm checking it in.

Tom