[Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target

Eric Farman posted 1 patch 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170814204450.24118-1-farman@linux.vnet.ibm.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
pc-bios/s390-ccw/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Eric Farman 6 years, 8 months ago
How often does one really do a "make clean" ?  Rather infrequently,
as I only stumbled on this today.

Perhaps I have missed the RM variable somewhere, as I see similar syntax
in some of the tests/tcg/ Makefiles, but I don't see it being set here.
My configure statement isn't terribly interesting, just enabling debug
for an s390x target, and as such there's no RM variable in its output.
I'll trust that Thomas will chime in with where it should have been.
In the meantime, this does the trick for me.

I didn't rebuild the image file after this change, because it didn't seem
necessary.  Let me know if that was a bad assumption, and I can submit one.

Eric Farman (1):
  pc-bios/s390-ccw: Use rm command during make clean

 pc-bios/s390-ccw/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.11.2


Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Thomas Huth 6 years, 8 months ago
On 14.08.2017 22:44, Eric Farman wrote:
> How often does one really do a "make clean" ?  Rather infrequently,
> as I only stumbled on this today.
> 
> Perhaps I have missed the RM variable somewhere, as I see similar syntax
> in some of the tests/tcg/ Makefiles, but I don't see it being set here.
> My configure statement isn't terribly interesting, just enabling debug
> for an s390x target, and as such there's no RM variable in its output.
> I'll trust that Thomas will chime in with where it should have been.
> In the meantime, this does the trick for me.

RM is one of the variables that should be pre-initialized by Make, and
AFAIK should be used to increase portability (well, it's likely not
important for QEMU since we require a posix-shell like built environment
anyway).

According to the info page of Make, chapter "10.3 Variables Used by
Implicit Rules":

`RM'
     Command to remove a file; default `rm -f'.

I've also checked it again and "make clean" works fine here (using GNU
Make 3.82). Which version of Make (and Linux distro) are you using?

Anyway, maybe I also simply missed something, so I'm certainly also fine
with the patch to revert it to "rm -f".

 Thomas

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Cornelia Huck 6 years, 8 months ago
On Tue, 15 Aug 2017 07:02:10 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 14.08.2017 22:44, Eric Farman wrote:
> > How often does one really do a "make clean" ?  Rather infrequently,
> > as I only stumbled on this today.
> > 
> > Perhaps I have missed the RM variable somewhere, as I see similar syntax
> > in some of the tests/tcg/ Makefiles, but I don't see it being set here.
> > My configure statement isn't terribly interesting, just enabling debug
> > for an s390x target, and as such there's no RM variable in its output.
> > I'll trust that Thomas will chime in with where it should have been.
> > In the meantime, this does the trick for me.  
> 
> RM is one of the variables that should be pre-initialized by Make, and
> AFAIK should be used to increase portability (well, it's likely not
> important for QEMU since we require a posix-shell like built environment
> anyway).
> 
> According to the info page of Make, chapter "10.3 Variables Used by
> Implicit Rules":
> 
> `RM'
>      Command to remove a file; default `rm -f'.
> 
> I've also checked it again and "make clean" works fine here (using GNU
> Make 3.82). Which version of Make (and Linux distro) are you using?

Interesting. It fails for me with GNU Make 3.82 on my RHEL guest as
well.

> Anyway, maybe I also simply missed something, so I'm certainly also fine
> with the patch to revert it to "rm -f".

Given that other bios makefiles use rm -f as well, let's just change
back until we figure out what's wrong.

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Thomas Huth 6 years, 8 months ago
On 15.08.2017 09:03, Cornelia Huck wrote:
> On Tue, 15 Aug 2017 07:02:10 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 14.08.2017 22:44, Eric Farman wrote:
>>> How often does one really do a "make clean" ?  Rather infrequently,
>>> as I only stumbled on this today.
>>>
>>> Perhaps I have missed the RM variable somewhere, as I see similar syntax
>>> in some of the tests/tcg/ Makefiles, but I don't see it being set here.
>>> My configure statement isn't terribly interesting, just enabling debug
>>> for an s390x target, and as such there's no RM variable in its output.
>>> I'll trust that Thomas will chime in with where it should have been.
>>> In the meantime, this does the trick for me.  
>>
>> RM is one of the variables that should be pre-initialized by Make, and
>> AFAIK should be used to increase portability (well, it's likely not
>> important for QEMU since we require a posix-shell like built environment
>> anyway).
>>
>> According to the info page of Make, chapter "10.3 Variables Used by
>> Implicit Rules":
>>
>> `RM'
>>      Command to remove a file; default `rm -f'.
>>
>> I've also checked it again and "make clean" works fine here (using GNU
>> Make 3.82). Which version of Make (and Linux distro) are you using?
> 
> Interesting. It fails for me with GNU Make 3.82 on my RHEL guest as
> well.
> 
>> Anyway, maybe I also simply missed something, so I'm certainly also fine
>> with the patch to revert it to "rm -f".
> 
> Given that other bios makefiles use rm -f as well, let's just change
> back until we figure out what's wrong.

I just discovered that it fails for me as well when I do "make clean"
from the top directory. So far I was only doing "make clean" after doing
a "cd pc-bios/s390-ccw" first, and that works fine. Weird. Something
seems to unset the RM variable in our build system, but I fail to find
the spot where this happens...

 Thomas



Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Thomas Huth 6 years, 8 months ago
On 15.08.2017 10:01, Thomas Huth wrote:
> On 15.08.2017 09:03, Cornelia Huck wrote:
>> On Tue, 15 Aug 2017 07:02:10 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 14.08.2017 22:44, Eric Farman wrote:
>>>> How often does one really do a "make clean" ?  Rather infrequently,
>>>> as I only stumbled on this today.
>>>>
>>>> Perhaps I have missed the RM variable somewhere, as I see similar syntax
>>>> in some of the tests/tcg/ Makefiles, but I don't see it being set here.
>>>> My configure statement isn't terribly interesting, just enabling debug
>>>> for an s390x target, and as such there's no RM variable in its output.
>>>> I'll trust that Thomas will chime in with where it should have been.
>>>> In the meantime, this does the trick for me.  
>>>
>>> RM is one of the variables that should be pre-initialized by Make, and
>>> AFAIK should be used to increase portability (well, it's likely not
>>> important for QEMU since we require a posix-shell like built environment
>>> anyway).
>>>
>>> According to the info page of Make, chapter "10.3 Variables Used by
>>> Implicit Rules":
>>>
>>> `RM'
>>>      Command to remove a file; default `rm -f'.
>>>
>>> I've also checked it again and "make clean" works fine here (using GNU
>>> Make 3.82). Which version of Make (and Linux distro) are you using?
>>
>> Interesting. It fails for me with GNU Make 3.82 on my RHEL guest as
>> well.
>>
>>> Anyway, maybe I also simply missed something, so I'm certainly also fine
>>> with the patch to revert it to "rm -f".
>>
>> Given that other bios makefiles use rm -f as well, let's just change
>> back until we figure out what's wrong.
> 
> I just discovered that it fails for me as well when I do "make clean"
> from the top directory. So far I was only doing "make clean" after doing
> a "cd pc-bios/s390-ccw" first, and that works fine. Weird. Something
> seems to unset the RM variable in our build system, but I fail to find
> the spot where this happens...

Ok, just found it: It's this line in rules.mak:

MAKEFLAGS += -rR

The parameter -R disables the built-in variables, so RM can indeed not
work here. Sorry, I wasn't aware of that setting yet, so your patch is
indeed the right fix here (or we should maybe define RM in rules.mak, too).

 Thomas

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Cornelia Huck 6 years, 8 months ago
On Tue, 15 Aug 2017 10:26:18 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 15.08.2017 10:01, Thomas Huth wrote:
> > On 15.08.2017 09:03, Cornelia Huck wrote:  
> >> On Tue, 15 Aug 2017 07:02:10 +0200
> >> Thomas Huth <thuth@redhat.com> wrote:
> >>  
> >>> On 14.08.2017 22:44, Eric Farman wrote:  
> >>>> How often does one really do a "make clean" ?  Rather infrequently,
> >>>> as I only stumbled on this today.
> >>>>
> >>>> Perhaps I have missed the RM variable somewhere, as I see similar syntax
> >>>> in some of the tests/tcg/ Makefiles, but I don't see it being set here.
> >>>> My configure statement isn't terribly interesting, just enabling debug
> >>>> for an s390x target, and as such there's no RM variable in its output.
> >>>> I'll trust that Thomas will chime in with where it should have been.
> >>>> In the meantime, this does the trick for me.    
> >>>
> >>> RM is one of the variables that should be pre-initialized by Make, and
> >>> AFAIK should be used to increase portability (well, it's likely not
> >>> important for QEMU since we require a posix-shell like built environment
> >>> anyway).
> >>>
> >>> According to the info page of Make, chapter "10.3 Variables Used by
> >>> Implicit Rules":
> >>>
> >>> `RM'
> >>>      Command to remove a file; default `rm -f'.
> >>>
> >>> I've also checked it again and "make clean" works fine here (using GNU
> >>> Make 3.82). Which version of Make (and Linux distro) are you using?  
> >>
> >> Interesting. It fails for me with GNU Make 3.82 on my RHEL guest as
> >> well.
> >>  
> >>> Anyway, maybe I also simply missed something, so I'm certainly also fine
> >>> with the patch to revert it to "rm -f".  
> >>
> >> Given that other bios makefiles use rm -f as well, let's just change
> >> back until we figure out what's wrong.  
> > 
> > I just discovered that it fails for me as well when I do "make clean"
> > from the top directory. So far I was only doing "make clean" after doing
> > a "cd pc-bios/s390-ccw" first, and that works fine. Weird. Something
> > seems to unset the RM variable in our build system, but I fail to find
> > the spot where this happens...  
> 
> Ok, just found it: It's this line in rules.mak:
> 
> MAKEFLAGS += -rR
> 
> The parameter -R disables the built-in variables, so RM can indeed not
> work here. Sorry, I wasn't aware of that setting yet, so your patch is
> indeed the right fix here (or we should maybe define RM in rules.mak, too).

The other users are all in tests/tcg/.

Would there be value in using the built-in variables generally? (Well,
value that outweighs having to go through the qemu build system...)

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Thomas Huth 6 years, 8 months ago
On 15.08.2017 10:32, Cornelia Huck wrote:
> On Tue, 15 Aug 2017 10:26:18 +0200
> Thomas Huth <thuth@redhat.com> wrote:
[...]
>> The parameter -R disables the built-in variables, so RM can indeed not
>> work here. Sorry, I wasn't aware of that setting yet, so your patch is
>> indeed the right fix here (or we should maybe define RM in rules.mak, too).
> 
> The other users are all in tests/tcg/.
> 
> Would there be value in using the built-in variables generally? (Well,
> value that outweighs having to go through the qemu build system...)

There are some non-posix shells where "rm -f" does not work (e.g.
COMMAND.COM on Windows), and this is where $(RM) is really useful. But
since we require a posix-style built environment anyway, it's not that
important for the QEMU build system. OTOH, $(RM) is a standard Make
variable, so other people might step into this trap as well - so it's
maybe better to define it in rules.mak to avoid future confusion?

 Thomas

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Eric Farman 6 years, 8 months ago

On 08/15/2017 04:26 AM, Thomas Huth wrote:
> On 15.08.2017 10:01, Thomas Huth wrote:
>> On 15.08.2017 09:03, Cornelia Huck wrote:
>>> On Tue, 15 Aug 2017 07:02:10 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> On 14.08.2017 22:44, Eric Farman wrote:
>>>>> How often does one really do a "make clean" ?  Rather infrequently,
>>>>> as I only stumbled on this today.
>>>>>
>>>>> Perhaps I have missed the RM variable somewhere, as I see similar syntax
>>>>> in some of the tests/tcg/ Makefiles, but I don't see it being set here.
>>>>> My configure statement isn't terribly interesting, just enabling debug
>>>>> for an s390x target, and as such there's no RM variable in its output.
>>>>> I'll trust that Thomas will chime in with where it should have been.
>>>>> In the meantime, this does the trick for me.
>>>>
>>>> RM is one of the variables that should be pre-initialized by Make, and
>>>> AFAIK should be used to increase portability (well, it's likely not
>>>> important for QEMU since we require a posix-shell like built environment
>>>> anyway).
>>>>
>>>> According to the info page of Make, chapter "10.3 Variables Used by
>>>> Implicit Rules":
>>>>
>>>> `RM'
>>>>       Command to remove a file; default `rm -f'.
>>>>
>>>> I've also checked it again and "make clean" works fine here (using GNU
>>>> Make 3.82). Which version of Make (and Linux distro) are you using?

Not that it matters now, but make 4.1 on F24

>>>
>>> Interesting. It fails for me with GNU Make 3.82 on my RHEL guest as
>>> well.
>>>
>>>> Anyway, maybe I also simply missed something, so I'm certainly also fine
>>>> with the patch to revert it to "rm -f".
>>>
>>> Given that other bios makefiles use rm -f as well, let's just change
>>> back until we figure out what's wrong.
>>
>> I just discovered that it fails for me as well when I do "make clean"
>> from the top directory. So far I was only doing "make clean" after doing
>> a "cd pc-bios/s390-ccw" first, and that works fine. Weird. Something
>> seems to unset the RM variable in our build system, but I fail to find
>> the spot where this happens...
> 
> Ok, just found it: It's this line in rules.mak:
> 
> MAKEFLAGS += -rR
> 
> The parameter -R disables the built-in variables, so RM can indeed not
> work here. Sorry, I wasn't aware of that setting yet, so your patch is
> indeed the right fix here (or we should maybe define RM in rules.mak, too).

Excellent find, Thomas!  I was not aware of that either.  Thanks to both 
you and Cornelia for this.

  - Eric


Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Peter Maydell 6 years, 8 months ago
On 14 August 2017 at 21:44, Eric Farman <farman@linux.vnet.ibm.com> wrote:
> How often does one really do a "make clean" ?  Rather infrequently,
> as I only stumbled on this today.

FWIW, one of my standard pre-merge build tests does a
"make clean" and then a make (mostly as a check that we
do build from clean as well as incrementally). Not sure
why it didn't catch this...

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Cornelia Huck 6 years, 8 months ago
On Tue, 15 Aug 2017 11:00:51 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 14 August 2017 at 21:44, Eric Farman <farman@linux.vnet.ibm.com> wrote:
> > How often does one really do a "make clean" ?  Rather infrequently,
> > as I only stumbled on this today.  
> 
> FWIW, one of my standard pre-merge build tests does a
> "make clean" and then a make (mostly as a check that we
> do build from clean as well as incrementally). Not sure
> why it didn't catch this...

Probably because you don't build on s390x... I'll add it to my
workflow, though.

Re: [Qemu-devel] [PATCH for-2.10 0/1] Fix "make clean" for s390 target
Posted by Peter Maydell 6 years, 8 months ago
On 15 August 2017 at 12:07, Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, 15 Aug 2017 11:00:51 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 14 August 2017 at 21:44, Eric Farman <farman@linux.vnet.ibm.com> wrote:
>> > How often does one really do a "make clean" ?  Rather infrequently,
>> > as I only stumbled on this today.
>>
>> FWIW, one of my standard pre-merge build tests does a
>> "make clean" and then a make (mostly as a check that we
>> do build from clean as well as incrementally). Not sure
>> why it didn't catch this...
>
> Probably because you don't build on s390x...

I do, but not with a 'make clean' step :-)

thanks
-- PMM