Use of non-portable find options in Makefile.in

[Philip Martin <ph...@wandisco.com>]

I've been experimenting with a SPARC Solaris build recently and the
non-portable use of find in Makefile.in is annoying, it means that the
'make clean' target fails.  There are two uses of

  find some/path -mindepth 1 -maxdepth 1 -print0 | xargs -0 rm -rf --

Solaris find doesn't support -mindepth, -maxdepth or -print0.

Are we using -print0 because problems with spaces in paths have been
observed or simply as some sort of future-proofing?  Don't we control
the paths thus allowing us to avoid spaces?

The use of -mindepth 1 -maxdepth 1 is also something I don't understand,
what advantage is there over simply using a shell expansion?  Like this:

  rm -rf some/path/*

The shell expansion doesn't appear to have a problem with spaces when I
test it.

There is also one use of

  find some/path -print0 | xargs -0 rm -rf --

looking for gcov files.  Have problems with spaces been observed or is
this also future-proofing?

I'd like to have a more portable Makefile.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: Use of non-portable find options in Makefile.in

[Stefan Sperling <st...@elego.de>]

On Mon, Mar 03, 2014 at 05:34:22PM +0000, Philip Martin wrote:
> I've been experimenting with a SPARC Solaris build recently and the
> non-portable use of find in Makefile.in is annoying, it means that the
> 'make clean' target fails.  There are two uses of
> 
>   find some/path -mindepth 1 -maxdepth 1 -print0 | xargs -0 rm -rf --
> 
> Solaris find doesn't support -mindepth, -maxdepth or -print0.
> 
> Are we using -print0 because problems with spaces in paths have been
> observed or simply as some sort of future-proofing?  Don't we control
> the paths thus allowing us to avoid spaces?
> 
> The use of -mindepth 1 -maxdepth 1 is also something I don't understand,

The first instance was added in r1421636, and the second one
copied from there in r1539215.

I agree that the Makefile should be as portable as possible
and support any of the alternatives you've mentioned.

Re: Use of non-portable find options in Makefile.in

[Peter Samuelson <pe...@p12n.org>]

[Peter Samuelson]
> Note also, if you _are_ worried about command line length limits, it's
> easy to buy yourself a lot more breathing room:
> 
> 	cd subversion/tests/cmdline/svn-test-work; rm -fr *

Of course I meant && in place of ; there.  Much safer in case the dir
itself may or may not exist.

> the find | xargs pattern itself was made somewhat obsolete by the
> find...-exec...+, which I believe is in POSIX.

While I don't have any older systems to hand, I did just now verify
that find...-exec...+ works on a 5-year-old Solaris 10 box.  (Hmmm, I
wonder if AIX 4.3 would still boot on my old RS/6000....)

Re: Use of non-portable find options in Makefile.in

[Peter Samuelson <pe...@p12n.org>]

[Ben Reser]
> I started this usage with r1421636.  The purpose is to avoid length limitations
> on the argument list not spaces creating problems.  We could change it to just:
> rm -rf subversion/tests/cmdline/svn-test-work/*

Note also, if you _are_ worried about command line length limits, it's
easy to buy yourself a lot more breathing room:

	cd subversion/tests/cmdline/svn-test-work; rm -fr *

Those long subdir prefixes on each filename argument really do add up.


> 	find . -name "*.gcda" -o -name "*.gcno" -print0 | xargs -0 rm -f --

Also, while -print0 and xargs -0 are not portable, the find | xargs
pattern itself was made somewhat obsolete by the find...-exec...+,
which I believe is in POSIX.  While find...-exec...';' runs one
instance per file, find...-exec...+, like xargs, processes as many
files at a time as will fit.

Re: Use of non-portable find options in Makefile.in

[Ben Reser <be...@reser.org>]

I reordered the quoted email a bit in order to make it easier to understand my
response.

On 3/3/14, 9:34 AM, Philip Martin wrote:
> The use of -mindepth 1 -maxdepth 1 is also something I don't understand,
> what advantage is there over simply using a shell expansion?  Like this:
>
>   rm -rf some/path/*
>
> The shell expansion doesn't appear to have a problem with spaces when I
> test it.

I started this usage with r1421636.  The purpose is to avoid length limitations
on the argument list not spaces creating problems.  We could change it to just:
rm -rf subversion/tests/cmdline/svn-test-work/*

We have other uses where we're using shell expansions like this that are far
more likely to run into argument list issues already.

I wasn't aware that -mindepth and -maxdepth were going to cause problems with
Solaris or I would have just let wildcard expansion do it.

> Are we using -print0 because problems with spaces in paths have been
> observed or simply as some sort of future-proofing?  Don't we control
> the paths thus allowing us to avoid spaces?

It's an attempt to avoid problems when people put their build tree on paths
that have spaces in them.  However, reviewing this it's clear we have other
problems.  Trying it, our build doesn't even work in other ways (most notably
cd without quoted arguments).

In some of these cases we're using relative paths within the tree and so then
the only potential issue would be people creating files with spaces in their
names (which we don't do).

So here are the find cases in our Makefile.in (these aren't exact literal
copies of the Makefile.in, I've inserted the target above the code snippet to
make it easier to find/understand in this email).

1)
fast-clean:
	find $(CTYPES_PYTHON_SRC_DIR) $(SWIG_PY_SRC_DIR) $(SWIG_PY_DIR) \
		$(abs_srcdir)/build $(top_srcdir)/subversion/tests/cmdline/svntest \
		-name "*.pyc" -exec rm {} ';'

This one uses absolute paths but doesn't quote the paths, breaks some of the
absolute paths end up having spaces in their names.  We need to quote the list
of search directories.

2)
clean-javahl:
	if [ -d $(javahl_test_rootdir) ]; then \
	  find $(javahl_test_rootdir) -mindepth 1 -maxdepth 1 \
               -print0 | xargs -0 rm -rf --;   \
	fi

Missing quoting on the search path, breaks when the build dir has a space in
the absolute path.  Probably should become:
rm -rf -- "$(javahl_test_rootdir)/*"

3)
gcov-clean:
	find . -name "*.gcda" -o -name "*.gcno" -print0 | xargs -0 rm -f --

This is relative paths so the only concern would be paths someone makes by hand
with spaces.  So probably can switch to just -print and -exec rm -f -- {} \;

4)
check-clean:
	if [ -d subversion/tests/cmdline/svn-test-work ]; then \
	  find subversion/tests/cmdline/svn-test-work -mindepth 1 -maxdepth 1 \
               -print0 | xargs -0 rm -rf --;   \
	fi

Again relative paths, so no issue with the absolute paths.  To avoid be more
portable can just become:
rm -rf subversion/tests/cmdline/svn-test-work/*

5)
clean-swig-py:
	find $(SWIG_PY_SRC_DIR) $(SWIG_PY_DIR) -name "*.pyc" -exec rm {} ';'

Unquoted absolute paths.