Messages in this thread |  | | From | Alexander Kapshuk <> | Date | Tue, 24 Jul 2018 08:20:07 +0100 | Subject | Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables |
| |
<div dir="auto"><div><div class="gmail_quote"><div dir="ltr">On Mon, Jul 23, 2018, 08:01 Jiri Olsa <<a href="mailto:jolsa@redhat.com">jolsa@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Jul 20, 2018 at 06:22:49PM +0300, Alexander Kapshuk wrote:<br> > On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa <<a href="mailto:jolsa@redhat.com" target="_blank" rel="noreferrer">jolsa@redhat.com</a>> wrote:<br> > ><br> > > On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:<br> > > > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:<br> > > > > The warning message in check_w function uses wrongly<br> > > > > the $file variable instead of $file1 and $file2.<br> > > ><br> > > > Humm,<br> > > ><br> > > > Before:<br> > > ><br> > > > Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'<br> > > ><br> > > > After:<br> > > ><br> > > > Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'<br> > > ><br> > > ><br> > > > The previous version is better, I can then just use:<br> > > ><br> > > > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h<br> > > ><br> > > > and get what changed, with your change I have to go to tools/perf before<br> > > > doing that diff, which is an unnecessary extra step in at least my<br> > > > workflow.<br> > ><br> > > so all paths output based in kernel tree root then, will change<br> > ><br> > > jirka<br> > <br> > I was going to ask about this in a separate email initially, but then<br> > thought I'd use this email exchange instead, as my question is about<br> > the code in question. Hope you don't mind.<br> > <br> > If I'm reading this right, the intended behavoir of the block of code<br> > below is to test file2 for existance, and if it exists, to evaluate $cmd.<br> > If file1 and file2 are found to differ, print the warning.<br> > <br> > test -f $file2 &&<br> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'<br> > differs from latest version at '$file'" >&2<br> > <br> > The '||' path of execution is however also taken if file2 doesn't exist,<br> > which is probably very unlikely to happen. See below.<br> > <br> > % file1=file1; file2=file2<br> > % cmd="echo diff $file1 $file2"<br> > % test -f $file2 &&<br> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'<br> > differs from latest version at '$file2'" >&2<br> > Warning: Kernel ABI header at 'tools/file1' differs from latest<br> > version at 'file2'<br> > <br> > Is this something you would rather leave as is, or perhaps use something<br> > along the lines of the code below instead:<br> > <br> > test -f $file2 && {<br> > eval $cmd ||<br> > echo "Warning: Kernel ABI header at 'tools/$file' differs from latest<br> > version at '$file'" >&2<br> > }<br> <br> hi,<br> yea, probably.. please feel free to post a patch.. just make sure all<br> the displayed files paths are based on the kernel root<br> <br> thanks,<br> jirka<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I'm away traveling till August 10th, and I may not be able to send the patch in until I get back. Is that OK?</div><div dir="auto">Thanks.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div></div></div>
|  |