[PULL,02/33] tests: Move tests/hex-loader-check-data/ to tests/data/hex-loader/

Message ID 20181105181353.39804-3-mst@redhat.com
State New
Headers show
Series
  • [PULL,01/33] tests: Move tests/acpi-test-data/ to tests/data/acpi/
Related show

Commit Message

Michael S. Tsirkin Nov. 5, 2018, 6:14 p.m.
From: Peter Maydell <peter.maydell@linaro.org>


Currently tests/hex-loader-check-data contains data files used
by the hexloader-test, and configure individually symlinks those
data files into the build directory using a wildcard.

Using a wildcard like this is a bad idea, because if a new
data file is added, nothing causes configure to be rerun,
and so no symlink is added for the new file. This can cause
tests to spuriously fail when they can't find their data.
Instead, it's better to symlink an entire directory of
data files. We already have such a directory: tests/data.

Move the data files from tests/hex-loader-check-data/ to
tests/data/hex-loader/, and remove the unnecessary symlinking.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 configure                                                 | 4 ----
 tests/hexloader-test.c                                    | 2 +-
 MAINTAINERS                                               | 2 +-
 tests/{hex-loader-check-data => data/hex-loader}/test.hex | 0
 4 files changed, 2 insertions(+), 6 deletions(-)
 rename tests/{hex-loader-check-data => data/hex-loader}/test.hex (100%)

-- 
MST

Comments

Philippe Mathieu-Daudé Nov. 6, 2018, 1:27 p.m. | #1
On 5/11/18 19:14, Michael S. Tsirkin wrote:
> From: Peter Maydell <peter.maydell@linaro.org>

> 

> Currently tests/hex-loader-check-data contains data files used

> by the hexloader-test, and configure individually symlinks those

> data files into the build directory using a wildcard.

> 

> Using a wildcard like this is a bad idea, because if a new

> data file is added, nothing causes configure to be rerun,

> and so no symlink is added for the new file. This can cause

> tests to spuriously fail when they can't find their data.

> Instead, it's better to symlink an entire directory of

> data files. We already have such a directory: tests/data.

> 

> Move the data files from tests/hex-loader-check-data/ to

> tests/data/hex-loader/, and remove the unnecessary symlinking.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


I reviewed/tested this patch too.

> ---

>   configure                                                 | 4 ----

>   tests/hexloader-test.c                                    | 2 +-

>   MAINTAINERS                                               | 2 +-

>   tests/{hex-loader-check-data => data/hex-loader}/test.hex | 0

>   4 files changed, 2 insertions(+), 6 deletions(-)

>   rename tests/{hex-loader-check-data => data/hex-loader}/test.hex (100%)

> 

> diff --git a/configure b/configure

> index 895b7483b8..bfdca8b814 100755

> --- a/configure

> +++ b/configure

> @@ -7421,10 +7421,6 @@ for bios_file in \

>   do

>       FILES="$FILES pc-bios/$(basename $bios_file)"

>   done

> -for test_file in $(find $source_path/tests/hex-loader-check-data -type f)

> -do

> -    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"

> -done

>   mkdir -p $DIRS

>   for f in $FILES ; do

>       if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then

> diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c

> index b653d44ba1..834ed52c22 100644

> --- a/tests/hexloader-test.c

> +++ b/tests/hexloader-test.c

> @@ -23,7 +23,7 @@ static void hex_loader_test(void)

>       const unsigned int base_addr = 0x00010000;

>   

>       QTestState *s = qtest_initf(

> -        "-M vexpress-a9 -nographic -device loader,file=tests/hex-loader-check-data/test.hex");

> +        "-M vexpress-a9 -nographic -device loader,file=tests/data/hex-loader/test.hex");

>   

>       for (i = 0; i < 256; ++i) {

>           uint8_t val = qtest_readb(s, base_addr + i);

> diff --git a/MAINTAINERS b/MAINTAINERS

> index 98a1856afc..cfabc14b59 100644

> --- a/MAINTAINERS

> +++ b/MAINTAINERS

> @@ -1370,7 +1370,7 @@ Intel Hexadecimal Object File Loader

>   M: Su Hang <suhang16@mails.ucas.ac.cn>

>   S: Maintained

>   F: tests/hexloader-test.c

> -F: tests/hex-loader-check-data/test.hex

> +F: tests/data/hex-loader/test.hex

>   

>   CHRP NVRAM

>   M: Thomas Huth <thuth@redhat.com>

> diff --git a/tests/hex-loader-check-data/test.hex b/tests/data/hex-loader/test.hex

> similarity index 100%

> rename from tests/hex-loader-check-data/test.hex

> rename to tests/data/hex-loader/test.hex

>
Michael S. Tsirkin Nov. 6, 2018, 2:13 p.m. | #2
On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:
> On 5/11/18 19:14, Michael S. Tsirkin wrote:

> > From: Peter Maydell <peter.maydell@linaro.org>

> > 

> > Currently tests/hex-loader-check-data contains data files used

> > by the hexloader-test, and configure individually symlinks those

> > data files into the build directory using a wildcard.

> > 

> > Using a wildcard like this is a bad idea, because if a new

> > data file is added, nothing causes configure to be rerun,

> > and so no symlink is added for the new file. This can cause

> > tests to spuriously fail when they can't find their data.

> > Instead, it's better to symlink an entire directory of

> > data files. We already have such a directory: tests/data.

> > 

> > Move the data files from tests/hex-loader-check-data/ to

> > tests/data/hex-loader/, and remove the unnecessary symlinking.

> > 

> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> 

> I reviewed/tested this patch too.



Thanks a lot Philippe!
It is unfortunately too late to update this patch info in git
commit history, however your help is still greatly appreciated!


> > ---

> >   configure                                                 | 4 ----

> >   tests/hexloader-test.c                                    | 2 +-

> >   MAINTAINERS                                               | 2 +-

> >   tests/{hex-loader-check-data => data/hex-loader}/test.hex | 0

> >   4 files changed, 2 insertions(+), 6 deletions(-)

> >   rename tests/{hex-loader-check-data => data/hex-loader}/test.hex (100%)

> > 

> > diff --git a/configure b/configure

> > index 895b7483b8..bfdca8b814 100755

> > --- a/configure

> > +++ b/configure

> > @@ -7421,10 +7421,6 @@ for bios_file in \

> >   do

> >       FILES="$FILES pc-bios/$(basename $bios_file)"

> >   done

> > -for test_file in $(find $source_path/tests/hex-loader-check-data -type f)

> > -do

> > -    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"

> > -done

> >   mkdir -p $DIRS

> >   for f in $FILES ; do

> >       if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then

> > diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c

> > index b653d44ba1..834ed52c22 100644

> > --- a/tests/hexloader-test.c

> > +++ b/tests/hexloader-test.c

> > @@ -23,7 +23,7 @@ static void hex_loader_test(void)

> >       const unsigned int base_addr = 0x00010000;

> >       QTestState *s = qtest_initf(

> > -        "-M vexpress-a9 -nographic -device loader,file=tests/hex-loader-check-data/test.hex");

> > +        "-M vexpress-a9 -nographic -device loader,file=tests/data/hex-loader/test.hex");

> >       for (i = 0; i < 256; ++i) {

> >           uint8_t val = qtest_readb(s, base_addr + i);

> > diff --git a/MAINTAINERS b/MAINTAINERS

> > index 98a1856afc..cfabc14b59 100644

> > --- a/MAINTAINERS

> > +++ b/MAINTAINERS

> > @@ -1370,7 +1370,7 @@ Intel Hexadecimal Object File Loader

> >   M: Su Hang <suhang16@mails.ucas.ac.cn>

> >   S: Maintained

> >   F: tests/hexloader-test.c

> > -F: tests/hex-loader-check-data/test.hex

> > +F: tests/data/hex-loader/test.hex

> >   CHRP NVRAM

> >   M: Thomas Huth <thuth@redhat.com>

> > diff --git a/tests/hex-loader-check-data/test.hex b/tests/data/hex-loader/test.hex

> > similarity index 100%

> > rename from tests/hex-loader-check-data/test.hex

> > rename to tests/data/hex-loader/test.hex

> >
Philippe Mathieu-Daudé Nov. 6, 2018, 3:15 p.m. | #3
On 6/11/18 15:13, Michael S. Tsirkin wrote:
> On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:

>> On 5/11/18 19:14, Michael S. Tsirkin wrote:

>>> From: Peter Maydell <peter.maydell@linaro.org>

>>>

>>> Currently tests/hex-loader-check-data contains data files used

>>> by the hexloader-test, and configure individually symlinks those

>>> data files into the build directory using a wildcard.

>>>

>>> Using a wildcard like this is a bad idea, because if a new

>>> data file is added, nothing causes configure to be rerun,

>>> and so no symlink is added for the new file. This can cause

>>> tests to spuriously fail when they can't find their data.

>>> Instead, it's better to symlink an entire directory of

>>> data files. We already have such a directory: tests/data.

>>>

>>> Move the data files from tests/hex-loader-check-data/ to

>>> tests/data/hex-loader/, and remove the unnecessary symlinking.

>>>

>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>>

>> I reviewed/tested this patch too.

> 

> 

> Thanks a lot Philippe!

> It is unfortunately too late to update this patch info in git

> commit history, however your help is still greatly appreciated!


No worry, I'm not mad at all, but there might be an issue in your git PR 
workflow, this series also missed your maintainer S-o-b.

Peter: Can you add a such check in your scripts? (during next merge 
window, no hurry).

Rather than your scripts, this should be in scripts a maintainer can run 
locally, such ./scripts/checkpatch.pl --maintainer or 
./scripts/checkseries.xx.

> 

> 

>>> ---

>>>    configure                                                 | 4 ----

>>>    tests/hexloader-test.c                                    | 2 +-

>>>    MAINTAINERS                                               | 2 +-

>>>    tests/{hex-loader-check-data => data/hex-loader}/test.hex | 0

>>>    4 files changed, 2 insertions(+), 6 deletions(-)

>>>    rename tests/{hex-loader-check-data => data/hex-loader}/test.hex (100%)

>>>

>>> diff --git a/configure b/configure

>>> index 895b7483b8..bfdca8b814 100755

>>> --- a/configure

>>> +++ b/configure

>>> @@ -7421,10 +7421,6 @@ for bios_file in \

>>>    do

>>>        FILES="$FILES pc-bios/$(basename $bios_file)"

>>>    done

>>> -for test_file in $(find $source_path/tests/hex-loader-check-data -type f)

>>> -do

>>> -    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"

>>> -done

>>>    mkdir -p $DIRS

>>>    for f in $FILES ; do

>>>        if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then

>>> diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c

>>> index b653d44ba1..834ed52c22 100644

>>> --- a/tests/hexloader-test.c

>>> +++ b/tests/hexloader-test.c

>>> @@ -23,7 +23,7 @@ static void hex_loader_test(void)

>>>        const unsigned int base_addr = 0x00010000;

>>>        QTestState *s = qtest_initf(

>>> -        "-M vexpress-a9 -nographic -device loader,file=tests/hex-loader-check-data/test.hex");

>>> +        "-M vexpress-a9 -nographic -device loader,file=tests/data/hex-loader/test.hex");

>>>        for (i = 0; i < 256; ++i) {

>>>            uint8_t val = qtest_readb(s, base_addr + i);

>>> diff --git a/MAINTAINERS b/MAINTAINERS

>>> index 98a1856afc..cfabc14b59 100644

>>> --- a/MAINTAINERS

>>> +++ b/MAINTAINERS

>>> @@ -1370,7 +1370,7 @@ Intel Hexadecimal Object File Loader

>>>    M: Su Hang <suhang16@mails.ucas.ac.cn>

>>>    S: Maintained

>>>    F: tests/hexloader-test.c

>>> -F: tests/hex-loader-check-data/test.hex

>>> +F: tests/data/hex-loader/test.hex

>>>    CHRP NVRAM

>>>    M: Thomas Huth <thuth@redhat.com>

>>> diff --git a/tests/hex-loader-check-data/test.hex b/tests/data/hex-loader/test.hex

>>> similarity index 100%

>>> rename from tests/hex-loader-check-data/test.hex

>>> rename to tests/data/hex-loader/test.hex

>>>
Peter Maydell Nov. 6, 2018, 3:31 p.m. | #4
On 6 November 2018 at 15:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> No worry, I'm not mad at all, but there might be an issue in your git PR

> workflow, this series also missed your maintainer S-o-b.

>

> Peter: Can you add a such check in your scripts? (during next merge window,

> no hurry).

>

> Rather than your scripts, this should be in scripts a maintainer can run

> locally, such ./scripts/checkpatch.pl --maintainer or

> ./scripts/checkseries.xx.


I have such a check in my own make-pullreq script which I use
for arm pull requests:
https://git.linaro.org/people/peter.maydell/misc-scripts.git/tree/make-pullreq#n98

I can't do much about other submaintainers' workflows
(and the block tree's "sub-submaintainers" setup means
"check all commits have a signoff from the same person as
the merge commit did" won't work either.)

thanks
-- PMM
Michael S. Tsirkin Nov. 6, 2018, 4:02 p.m. | #5
On Tue, Nov 06, 2018 at 03:31:08PM +0000, Peter Maydell wrote:
> On 6 November 2018 at 15:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> > No worry, I'm not mad at all, but there might be an issue in your git PR

> > workflow, this series also missed your maintainer S-o-b.

> >

> > Peter: Can you add a such check in your scripts? (during next merge window,

> > no hurry).

> >

> > Rather than your scripts, this should be in scripts a maintainer can run

> > locally, such ./scripts/checkpatch.pl --maintainer or

> > ./scripts/checkseries.xx.

> 

> I have such a check in my own make-pullreq script which I use

> for arm pull requests:

> https://git.linaro.org/people/peter.maydell/misc-scripts.git/tree/make-pullreq#n98

> 

> I can't do much about other submaintainers' workflows

> (and the block tree's "sub-submaintainers" setup means

> "check all commits have a signoff from the same person as

> the merge commit did" won't work either.)

> 

> thanks

> -- PMM


I don't have anything validating that but I agree it's a good idea.
Will add, thanks!
Michael S. Tsirkin Nov. 6, 2018, 4:08 p.m. | #6
On Tue, Nov 06, 2018 at 04:15:03PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/11/18 15:13, Michael S. Tsirkin wrote:

> > On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:

> > > On 5/11/18 19:14, Michael S. Tsirkin wrote:

> > > > From: Peter Maydell <peter.maydell@linaro.org>

> > > > 

> > > > Currently tests/hex-loader-check-data contains data files used

> > > > by the hexloader-test, and configure individually symlinks those

> > > > data files into the build directory using a wildcard.

> > > > 

> > > > Using a wildcard like this is a bad idea, because if a new

> > > > data file is added, nothing causes configure to be rerun,

> > > > and so no symlink is added for the new file. This can cause

> > > > tests to spuriously fail when they can't find their data.

> > > > Instead, it's better to symlink an entire directory of

> > > > data files. We already have such a directory: tests/data.

> > > > 

> > > > Move the data files from tests/hex-loader-check-data/ to

> > > > tests/data/hex-loader/, and remove the unnecessary symlinking.

> > > > 

> > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> > > 

> > > I reviewed/tested this patch too.

> > 

> > 

> > Thanks a lot Philippe!

> > It is unfortunately too late to update this patch info in git

> > commit history, however your help is still greatly appreciated!

> 

> No worry, I'm not mad at all, but there might be an issue in your git PR

> workflow, this series also missed your maintainer S-o-b.


It's just that I could not figure out the failures that were blocking
the pull, so when I saw that Peter finally posted the fix I rushed to
merge and test it and didn't look for any acks.  My mistake, sorry about
that. That's also why I forgot to sign it.

> Peter: Can you add a such check in your scripts? (during next merge window,

> no hurry).

> 

> Rather than your scripts, this should be in scripts a maintainer can run

> locally, such ./scripts/checkpatch.pl --maintainer or

> ./scripts/checkseries.xx.

> 

> > 

> > 

> > > > ---

> > > >    configure                                                 | 4 ----

> > > >    tests/hexloader-test.c                                    | 2 +-

> > > >    MAINTAINERS                                               | 2 +-

> > > >    tests/{hex-loader-check-data => data/hex-loader}/test.hex | 0

> > > >    4 files changed, 2 insertions(+), 6 deletions(-)

> > > >    rename tests/{hex-loader-check-data => data/hex-loader}/test.hex (100%)

> > > > 

> > > > diff --git a/configure b/configure

> > > > index 895b7483b8..bfdca8b814 100755

> > > > --- a/configure

> > > > +++ b/configure

> > > > @@ -7421,10 +7421,6 @@ for bios_file in \

> > > >    do

> > > >        FILES="$FILES pc-bios/$(basename $bios_file)"

> > > >    done

> > > > -for test_file in $(find $source_path/tests/hex-loader-check-data -type f)

> > > > -do

> > > > -    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"

> > > > -done

> > > >    mkdir -p $DIRS

> > > >    for f in $FILES ; do

> > > >        if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then

> > > > diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c

> > > > index b653d44ba1..834ed52c22 100644

> > > > --- a/tests/hexloader-test.c

> > > > +++ b/tests/hexloader-test.c

> > > > @@ -23,7 +23,7 @@ static void hex_loader_test(void)

> > > >        const unsigned int base_addr = 0x00010000;

> > > >        QTestState *s = qtest_initf(

> > > > -        "-M vexpress-a9 -nographic -device loader,file=tests/hex-loader-check-data/test.hex");

> > > > +        "-M vexpress-a9 -nographic -device loader,file=tests/data/hex-loader/test.hex");

> > > >        for (i = 0; i < 256; ++i) {

> > > >            uint8_t val = qtest_readb(s, base_addr + i);

> > > > diff --git a/MAINTAINERS b/MAINTAINERS

> > > > index 98a1856afc..cfabc14b59 100644

> > > > --- a/MAINTAINERS

> > > > +++ b/MAINTAINERS

> > > > @@ -1370,7 +1370,7 @@ Intel Hexadecimal Object File Loader

> > > >    M: Su Hang <suhang16@mails.ucas.ac.cn>

> > > >    S: Maintained

> > > >    F: tests/hexloader-test.c

> > > > -F: tests/hex-loader-check-data/test.hex

> > > > +F: tests/data/hex-loader/test.hex

> > > >    CHRP NVRAM

> > > >    M: Thomas Huth <thuth@redhat.com>

> > > > diff --git a/tests/hex-loader-check-data/test.hex b/tests/data/hex-loader/test.hex

> > > > similarity index 100%

> > > > rename from tests/hex-loader-check-data/test.hex

> > > > rename to tests/data/hex-loader/test.hex

> > > >
Laurent Vivier Nov. 6, 2018, 4:16 p.m. | #7
On 06/11/2018 16:15, Philippe Mathieu-Daudé wrote:
> On 6/11/18 15:13, Michael S. Tsirkin wrote:

>> On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:

>>> On 5/11/18 19:14, Michael S. Tsirkin wrote:

>>>> From: Peter Maydell <peter.maydell@linaro.org>

>>>>

>>>> Currently tests/hex-loader-check-data contains data files used

>>>> by the hexloader-test, and configure individually symlinks those

>>>> data files into the build directory using a wildcard.

>>>>

>>>> Using a wildcard like this is a bad idea, because if a new

>>>> data file is added, nothing causes configure to be rerun,

>>>> and so no symlink is added for the new file. This can cause

>>>> tests to spuriously fail when they can't find their data.

>>>> Instead, it's better to symlink an entire directory of

>>>> data files. We already have such a directory: tests/data.

>>>>

>>>> Move the data files from tests/hex-loader-check-data/ to

>>>> tests/data/hex-loader/, and remove the unnecessary symlinking.

>>>>

>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>>>

>>> I reviewed/tested this patch too.

>>

>>

>> Thanks a lot Philippe!

>> It is unfortunately too late to update this patch info in git

>> commit history, however your help is still greatly appreciated!

> 

> No worry, I'm not mad at all, but there might be an issue in your git PR

> workflow, this series also missed your maintainer S-o-b.

> 

> Peter: Can you add a such check in your scripts? (during next merge

> window, no hurry).

> 

> Rather than your scripts, this should be in scripts a maintainer can run

> locally, such ./scripts/checkpatch.pl --maintainer or

> ./scripts/checkseries.xx.


I think such tool already exists: with git-publish you can configure a
"pre-publish-send-email" hook, and check your S-o-B is present.

Thanks,
Laurent
Stefan Hajnoczi Nov. 8, 2018, 10:24 a.m. | #8
On Tue, Nov 06, 2018 at 05:16:14PM +0100, Laurent Vivier wrote:
> On 06/11/2018 16:15, Philippe Mathieu-Daudé wrote:

> > On 6/11/18 15:13, Michael S. Tsirkin wrote:

> >> On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:

> >>> On 5/11/18 19:14, Michael S. Tsirkin wrote:

> >>>> From: Peter Maydell <peter.maydell@linaro.org>

> >>>>

> >>>> Currently tests/hex-loader-check-data contains data files used

> >>>> by the hexloader-test, and configure individually symlinks those

> >>>> data files into the build directory using a wildcard.

> >>>>

> >>>> Using a wildcard like this is a bad idea, because if a new

> >>>> data file is added, nothing causes configure to be rerun,

> >>>> and so no symlink is added for the new file. This can cause

> >>>> tests to spuriously fail when they can't find their data.

> >>>> Instead, it's better to symlink an entire directory of

> >>>> data files. We already have such a directory: tests/data.

> >>>>

> >>>> Move the data files from tests/hex-loader-check-data/ to

> >>>> tests/data/hex-loader/, and remove the unnecessary symlinking.

> >>>>

> >>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> >>>

> >>> I reviewed/tested this patch too.

> >>

> >>

> >> Thanks a lot Philippe!

> >> It is unfortunately too late to update this patch info in git

> >> commit history, however your help is still greatly appreciated!

> > 

> > No worry, I'm not mad at all, but there might be an issue in your git PR

> > workflow, this series also missed your maintainer S-o-b.

> > 

> > Peter: Can you add a such check in your scripts? (during next merge

> > window, no hurry).

> > 

> > Rather than your scripts, this should be in scripts a maintainer can run

> > locally, such ./scripts/checkpatch.pl --maintainer or

> > ./scripts/checkseries.xx.

> 

> I think such tool already exists: with git-publish you can configure a

> "pre-publish-send-email" hook, and check your S-o-B is present.


For making my own commits .git/hooks/pre-commit is useful:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

For sending patch series, git-publish is useful:
https://github.com/stefanha/git-publish/blob/master/hooks/pre-publish-send-email.example
https://github.com/stefanha/git-publish/

Stefan
Laurent Vivier Nov. 8, 2018, 2:30 p.m. | #9
On 08/11/2018 11:24, Stefan Hajnoczi wrote:
> On Tue, Nov 06, 2018 at 05:16:14PM +0100, Laurent Vivier wrote:

>> On 06/11/2018 16:15, Philippe Mathieu-Daudé wrote:

>>> On 6/11/18 15:13, Michael S. Tsirkin wrote:

>>>> On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:

>>>>> On 5/11/18 19:14, Michael S. Tsirkin wrote:

>>>>>> From: Peter Maydell <peter.maydell@linaro.org>

>>>>>>

>>>>>> Currently tests/hex-loader-check-data contains data files used

>>>>>> by the hexloader-test, and configure individually symlinks those

>>>>>> data files into the build directory using a wildcard.

>>>>>>

>>>>>> Using a wildcard like this is a bad idea, because if a new

>>>>>> data file is added, nothing causes configure to be rerun,

>>>>>> and so no symlink is added for the new file. This can cause

>>>>>> tests to spuriously fail when they can't find their data.

>>>>>> Instead, it's better to symlink an entire directory of

>>>>>> data files. We already have such a directory: tests/data.

>>>>>>

>>>>>> Move the data files from tests/hex-loader-check-data/ to

>>>>>> tests/data/hex-loader/, and remove the unnecessary symlinking.

>>>>>>

>>>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>>>>>

>>>>> I reviewed/tested this patch too.

>>>>

>>>>

>>>> Thanks a lot Philippe!

>>>> It is unfortunately too late to update this patch info in git

>>>> commit history, however your help is still greatly appreciated!

>>>

>>> No worry, I'm not mad at all, but there might be an issue in your git PR

>>> workflow, this series also missed your maintainer S-o-b.

>>>

>>> Peter: Can you add a such check in your scripts? (during next merge

>>> window, no hurry).

>>>

>>> Rather than your scripts, this should be in scripts a maintainer can run

>>> locally, such ./scripts/checkpatch.pl --maintainer or

>>> ./scripts/checkseries.xx.

>>

>> I think such tool already exists: with git-publish you can configure a

>> "pre-publish-send-email" hook, and check your S-o-B is present.

> 

> For making my own commits .git/hooks/pre-commit is useful:

> http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

> 

> For sending patch series, git-publish is useful:

> https://github.com/stefanha/git-publish/blob/master/hooks/pre-publish-send-email.example

> https://github.com/stefanha/git-publish/


A pre-publish-send-email like this should do the trick:

#!/bin/bash

NAME=$(git config --get user.name)
EMAIL=$(git config --get user.email)

for PATCH in $1/*.patch; do
    if [ $(basename $PATCH) = "0000-cover-letter.patch" ]; then
        continue
    fi
    if ! grep -q "^Signed-off-by: $NAME <$EMAIL>" $PATCH; then
        echo "Error: Missing sender S-o-B in $PATCH"
        exit 1
    fi
done
exit 0
Philippe Mathieu-Daudé Nov. 8, 2018, 3:15 p.m. | #10
On Thu, Nov 8, 2018 at 3:30 PM Laurent Vivier <lvivier@redhat.com> wrote:
> On 08/11/2018 11:24, Stefan Hajnoczi wrote:

> > On Tue, Nov 06, 2018 at 05:16:14PM +0100, Laurent Vivier wrote:

> >> On 06/11/2018 16:15, Philippe Mathieu-Daudé wrote:

> >>> On 6/11/18 15:13, Michael S. Tsirkin wrote:

> >>>> On Tue, Nov 06, 2018 at 02:27:18PM +0100, Philippe Mathieu-Daudé wrote:

> >>>>> On 5/11/18 19:14, Michael S. Tsirkin wrote:

> >>>>>> From: Peter Maydell <peter.maydell@linaro.org>

> >>>>>>

> >>>>>> Currently tests/hex-loader-check-data contains data files used

> >>>>>> by the hexloader-test, and configure individually symlinks those

> >>>>>> data files into the build directory using a wildcard.

> >>>>>>

> >>>>>> Using a wildcard like this is a bad idea, because if a new

> >>>>>> data file is added, nothing causes configure to be rerun,

> >>>>>> and so no symlink is added for the new file. This can cause

> >>>>>> tests to spuriously fail when they can't find their data.

> >>>>>> Instead, it's better to symlink an entire directory of

> >>>>>> data files. We already have such a directory: tests/data.

> >>>>>>

> >>>>>> Move the data files from tests/hex-loader-check-data/ to

> >>>>>> tests/data/hex-loader/, and remove the unnecessary symlinking.

> >>>>>>

> >>>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> >>>>>

> >>>>> I reviewed/tested this patch too.

> >>>>

> >>>>

> >>>> Thanks a lot Philippe!

> >>>> It is unfortunately too late to update this patch info in git

> >>>> commit history, however your help is still greatly appreciated!

> >>>

> >>> No worry, I'm not mad at all, but there might be an issue in your git PR

> >>> workflow, this series also missed your maintainer S-o-b.

> >>>

> >>> Peter: Can you add a such check in your scripts? (during next merge

> >>> window, no hurry).

> >>>

> >>> Rather than your scripts, this should be in scripts a maintainer can run

> >>> locally, such ./scripts/checkpatch.pl --maintainer or

> >>> ./scripts/checkseries.xx.

> >>

> >> I think such tool already exists: with git-publish you can configure a

> >> "pre-publish-send-email" hook, and check your S-o-B is present.

> >

> > For making my own commits .git/hooks/pre-commit is useful:

> > http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

> >

> > For sending patch series, git-publish is useful:

> > https://github.com/stefanha/git-publish/blob/master/hooks/pre-publish-send-email.example

> > https://github.com/stefanha/git-publish/

>

> A pre-publish-send-email like this should do the trick:

>

> #!/bin/bash

>

> NAME=$(git config --get user.name)

> EMAIL=$(git config --get user.email)

>

> for PATCH in $1/*.patch; do

>     if [ $(basename $PATCH) = "0000-cover-letter.patch" ]; then

>         continue

>     fi


Maybe easier (to handle series versions > 1):

    case ${PATCH} in
    *-cover-letter.patch)
        continue
        ;;
    *)
    ...

>     if ! grep -q "^Signed-off-by: $NAME <$EMAIL>" $PATCH; then

>         echo "Error: Missing sender S-o-B in $PATCH"

>         exit 1

>     fi

> done

> exit 0

>

>

Patch

diff --git a/configure b/configure
index 895b7483b8..bfdca8b814 100755
--- a/configure
+++ b/configure
@@ -7421,10 +7421,6 @@  for bios_file in \
 do
     FILES="$FILES pc-bios/$(basename $bios_file)"
 done
-for test_file in $(find $source_path/tests/hex-loader-check-data -type f)
-do
-    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
-done
 mkdir -p $DIRS
 for f in $FILES ; do
     if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then
diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c
index b653d44ba1..834ed52c22 100644
--- a/tests/hexloader-test.c
+++ b/tests/hexloader-test.c
@@ -23,7 +23,7 @@  static void hex_loader_test(void)
     const unsigned int base_addr = 0x00010000;
 
     QTestState *s = qtest_initf(
-        "-M vexpress-a9 -nographic -device loader,file=tests/hex-loader-check-data/test.hex");
+        "-M vexpress-a9 -nographic -device loader,file=tests/data/hex-loader/test.hex");
 
     for (i = 0; i < 256; ++i) {
         uint8_t val = qtest_readb(s, base_addr + i);
diff --git a/MAINTAINERS b/MAINTAINERS
index 98a1856afc..cfabc14b59 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1370,7 +1370,7 @@  Intel Hexadecimal Object File Loader
 M: Su Hang <suhang16@mails.ucas.ac.cn>
 S: Maintained
 F: tests/hexloader-test.c
-F: tests/hex-loader-check-data/test.hex
+F: tests/data/hex-loader/test.hex
 
 CHRP NVRAM
 M: Thomas Huth <thuth@redhat.com>
diff --git a/tests/hex-loader-check-data/test.hex b/tests/data/hex-loader/test.hex
similarity index 100%
rename from tests/hex-loader-check-data/test.hex
rename to tests/data/hex-loader/test.hex