| Message ID | 1b8aee19-9194-153c-8dbb-0aec3412e255@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/x86: adjust populating of tools/include/xen/ | expand |
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@suse.com> > --- > 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? > > --- 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? Thanks, Roger.
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@suse.com> >> --- >> 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. >> --- 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). Jan
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@suse.com> > >> --- > >> 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@citrix.com> 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
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@suse.com> >> --- >> 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? I've looked around some more and also remembered an aspect because of the looking around. 1) xargs has special provisions to not overrun argument space (leaving a gap of 2048 bytes). Nothing like this is said in the spec for find, and in fact I recall having had issues when the accumulated arguments needed splitting. This was on an old distro, but I wanted to mention it. 2) I've found (old) indications that the {} may be troublesome to some distros (Solaris was mentioned) and might need quoting. (This would of course be easy to deal with.) Jan
On Fri, Mar 12, 2021 at 09:45:35AM +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@suse.com> > >> --- > >> 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? > > I've looked around some more and also remembered an aspect because > of the looking around. > > 1) xargs has special provisions to not overrun argument space (leaving > a gap of 2048 bytes). Nothing like this is said in the spec for find, > and in fact I recall having had issues when the accumulated arguments > needed splitting. This was on an old distro, but I wanted to mention > it. Oh, the POSIX spec does note this: "The size of any set of two or more pathnames shall be limited such that execution of the utility does not cause the system's {ARG_MAX} limit to be exceeded." I would argue that not doing this is a bug. > 2) I've found (old) indications that the {} may be troublesome to > some distros (Solaris was mentioned) and might need quoting. (This > would of course be easy to deal with.) Hm, OK. I won't insist then. I'm not sure we should still consider Solaris as supported anyway. It's likely to explode in a lot of ways when trying to build. Do you really need the stderr redirection to /dev/null of find output when using xargs though? As that will just drop error messages, making failure diagnostic harder. Thanks, Roger.
On 12.03.2021 10:17, Roger Pau Monné wrote: > On Fri, Mar 12, 2021 at 09:45:35AM +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@suse.com> >>>> --- >>>> 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? >> >> I've looked around some more and also remembered an aspect because >> of the looking around. >> >> 1) xargs has special provisions to not overrun argument space (leaving >> a gap of 2048 bytes). Nothing like this is said in the spec for find, >> and in fact I recall having had issues when the accumulated arguments >> needed splitting. This was on an old distro, but I wanted to mention >> it. > > Oh, the POSIX spec does note this: > > "The size of any set of two or more pathnames shall be limited such > that execution of the utility does not cause the system's {ARG_MAX} > limit to be exceeded." Well, yes. Hence my referral to the extra precautions in xargs: "The xargs utility shall limit the command line length such that when the command line is invoked, the combined argument and environment lists (see the exec family of functions in the System Interfaces volume of IEEE Std 1003.1-2001) shall not exceed {ARG_MAX}-2048 bytes." > I would argue that not doing this is a bug. I agree, of course. >> 2) I've found (old) indications that the {} may be troublesome to >> some distros (Solaris was mentioned) and might need quoting. (This >> would of course be easy to deal with.) > > Hm, OK. I won't insist then. I'm not sure we should still consider > Solaris as supported anyway. It's likely to explode in a lot of ways > when trying to build. Right, I understand Solaris isn't the best example. I'd like to see what Ian says towards either of the options being better than the other. > Do you really need the stderr redirection to /dev/null of find output > when using xargs though? As that will just drop error messages, making > failure diagnostic harder. Well, yes, upon first build there would be error messages otherwise for xen/ and acpi/ not existing (yet). Jan
Jan Beulich writes ("[PATCH v2 1/2][4.15] tools/x86: don't rebuild cpuid-autogen.h every time"):
> 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.
I addressed these portability questions in my original mail.
I said that I had checked FreeBSD find and xargs have all these
options. I didn't check the other *BSDs but I see no reason to think
they would be different.
find -print0 and xargs -0 are not in SuS (for bad reasons) but these
are obviously necessary (unless one thinks people are supposed to use
-exec printf "%s\n" '{}' \;
when the output is going to something more complicated an just some
command's arguments.
IOW we are IMO find to rely on both these options.
I would be OK with find -exec + in this case. The {} must be quoted.
> --- 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 --
I disapprove of the 2>/dev/null and the -f because I disapprove of
suppressing.
Ian.
--- 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 -- mkdir -p xen/libelf acpi ln -s $(XEN_ROOT)/xen/include/public/COPYING xen/ ln -s $(XEN_ROOT)/xen/include/public/*.h xen/
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@suse.com> --- 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.