Re: [PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 11, 2021 at 05:00:12PM +0100, Jan Beulich wrote:
> On 11.03.2021 16:29, Roger Pau Monné wrote:
> > On Thu, Mar 11, 2021 at 03:40:05PM +0100, Jan Beulich wrote:
> >> The first thing the "xen-dir" rule does is delete the entire xen/
> >> subtree. Obviously this includes deleting xen/lib/x86/*autogen.h. As a
> >> result there's no original version for $(move-if-changed ...) to compare
> >> against, and hence the file and all its consumers would get rebuilt
> >> every time. Instead only find and delete all the symlinks.
> >>
> >> Fixes: eddf9559c977 ("libx86: generate cpuid-autogen.h in the libx86 
> >> include dir")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> v2: Different approach.
> >> ---
> >> Ian did suggest to pass -0r to xargs (and -print0 to find), but I
> >> couldn't convince myself that these are standard compliant options. We
> >> don't use any special characters in file names, so -print0 / -0
> >> shouldn't be necessary at all. The stray rm invocation when there is no
> >> output from find can be taken care of by passing -f to it.
> > 
> > Why not use `-exec rm -f {} +` instead? That seems to be part of
> > POSIX and is likely nicer than piping to xargs?
> 
> Hmm, I avoided it because I was under the impression that there
> are (compatibility) issues with it, and Ian suggesting xargs
> seemed to support that. I'd be more than happy to avoid xargs,
> of which I've never been a friend.

All I can tell is that '-exec ... {} +'  is part of POSIX [0], and I
can confirm it works on FreeBSD. I have a slight preference for -exec
instead of xargs because I think it's cleaner, but I think your
current one is correct, so:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

For either one.

> >> --- a/tools/include/Makefile
> >> +++ b/tools/include/Makefile
> >> @@ -19,7 +19,7 @@ xen-foreign:
> >>    $(MAKE) -C xen-foreign
> >>  
> >>  xen-dir:
> >> -  @rm -rf xen acpi
> >> +  find xen/ acpi/ -type l 2>/dev/null | xargs rm -f --
> > 
> > Do we care about leaving an empty xen/libelf directory behind?
> 
> Why would we? It'll get created immediately afterwards if it's
> not there, and it'll initially be empty (not for long of course).

Right, also the 'clean' target will still rm the whole directory.

Thanks, Roger.

[0] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/find.html