Makefile: remove DESTDIR from firmware file content

[Qemu-devel] [PATCH v1] Makefile: remove DESTDIR from firmware file content

Posted by Olaf Hering 4 years, 11 months ago

The resulting firmware files should only contain the runtime path.
Fixes commit 26ce90fde5c ("Makefile: install the edk2 firmware images
and their descriptors")

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index f0be624f47..61267bf1a4 100644
--- a/Makefile
+++ b/Makefile
@@ -844,7 +844,7 @@ ifneq ($(DESCS),)
 	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/firmware"
 	set -e; tmpf=$$(mktemp); trap 'rm -f -- "$$tmpf"' EXIT; \
 	for x in $(DESCS); do \
-		sed -e 's,@DATADIR@,$(DESTDIR)$(qemu_datadir),' \
+		sed -e 's,@DATADIR@,$(qemu_datadir),' \
 			"$(SRC_PATH)/pc-bios/descriptors/$$x" > "$$tmpf"; \
 		$(INSTALL_DATA) "$$tmpf" \
 			"$(DESTDIR)$(qemu_datadir)/firmware/$$x"; \

Re: [Qemu-devel] [PATCH v1] Makefile: remove DESTDIR from firmware file content

Posted by Daniel P. Berrangé 4 years, 11 months ago

On Thu, May 30, 2019 at 09:28:11PM +0200, Olaf Hering wrote:
> The resulting firmware files should only contain the runtime path.
> Fixes commit 26ce90fde5c ("Makefile: install the edk2 firmware images
> and their descriptors")
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index f0be624f47..61267bf1a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -844,7 +844,7 @@ ifneq ($(DESCS),)
>  	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/firmware"
>  	set -e; tmpf=$$(mktemp); trap 'rm -f -- "$$tmpf"' EXIT; \
>  	for x in $(DESCS); do \
> -		sed -e 's,@DATADIR@,$(DESTDIR)$(qemu_datadir),' \
> +		sed -e 's,@DATADIR@,$(qemu_datadir),' \
>  			"$(SRC_PATH)/pc-bios/descriptors/$$x" > "$$tmpf"; \
>  		$(INSTALL_DATA) "$$tmpf" \
>  			"$(DESTDIR)$(qemu_datadir)/firmware/$$x"; \

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v1] Makefile: remove DESTDIR from firmware file content

Posted by Philippe Mathieu-Daudé 4 years, 11 months ago

[Odd, I'm pretty sure I replied to this last week but can't find it, so
again]
Cc'ing Laszlo since he wrote this.

On 5/30/19 9:28 PM, Olaf Hering wrote:
> The resulting firmware files should only contain the runtime path.
> Fixes commit 26ce90fde5c ("Makefile: install the edk2 firmware images
> and their descriptors")
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index f0be624f47..61267bf1a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -844,7 +844,7 @@ ifneq ($(DESCS),)
>  	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/firmware"
>  	set -e; tmpf=$$(mktemp); trap 'rm -f -- "$$tmpf"' EXIT; \
>  	for x in $(DESCS); do \
> -		sed -e 's,@DATADIR@,$(DESTDIR)$(qemu_datadir),' \
> +		sed -e 's,@DATADIR@,$(qemu_datadir),' \

I'm fine with this patch.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Laszlo, did you use the full path for a particular reason?

>  			"$(SRC_PATH)/pc-bios/descriptors/$$x" > "$$tmpf"; \
>  		$(INSTALL_DATA) "$$tmpf" \
>  			"$(DESTDIR)$(qemu_datadir)/firmware/$$x"; \
>

Re: [Qemu-devel] [PATCH v1] Makefile: remove DESTDIR from firmware file content

Posted by Laszlo Ersek 4 years, 11 months ago

On 06/05/19 16:46, Philippe Mathieu-Daudé wrote:
> [Odd, I'm pretty sure I replied to this last week but can't find it, so
> again]
> Cc'ing Laszlo since he wrote this.
> 
> On 5/30/19 9:28 PM, Olaf Hering wrote:
>> The resulting firmware files should only contain the runtime path.
>> Fixes commit 26ce90fde5c ("Makefile: install the edk2 firmware images
>> and their descriptors")
>>
>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>> ---
>>  Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index f0be624f47..61267bf1a4 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -844,7 +844,7 @@ ifneq ($(DESCS),)
>>  	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/firmware"
>>  	set -e; tmpf=$$(mktemp); trap 'rm -f -- "$$tmpf"' EXIT; \
>>  	for x in $(DESCS); do \
>> -		sed -e 's,@DATADIR@,$(DESTDIR)$(qemu_datadir),' \
>> +		sed -e 's,@DATADIR@,$(qemu_datadir),' \
> 
> I'm fine with this patch.
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Laszlo, did you use the full path for a particular reason?

Yes. The reason was that I had no idea what DESTDIR stood for. After
grepping the QEMU source tree for DESTDIR, I *still* have no idea what
it stands for.

DESTDIR seems to be an installation prefix that is not controlled through

  ./configre --prefix=...

Based on memories from other projects -- with better documentation --
though, DESTDIR looks like an install-time-only pre-prefix. In that
sense, the patch looks correct (and the vague expression "runtime path"
in the commit message starts to make some sense). Presumably DESTDIR is
used in package build scripts... Yup, the RHEL spec files use it too,
for "make install".

So, the patch is correct IMO:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Too bad QEMU is chronically under-documented (and the commit message on
the patch doesn't help much in that regard).

Thanks
Laszlo

>>  			"$(SRC_PATH)/pc-bios/descriptors/$$x" > "$$tmpf"; \
>>  		$(INSTALL_DATA) "$$tmpf" \
>>  			"$(DESTDIR)$(qemu_datadir)/firmware/$$x"; \
>>

Re: [Qemu-devel] [PATCH v1] Makefile: remove DESTDIR from firmware file content

Posted by Philippe Mathieu-Daudé 4 years, 11 months ago

On 6/5/19 6:31 PM, Laszlo Ersek wrote:
> On 06/05/19 16:46, Philippe Mathieu-Daudé wrote:
>> [Odd, I'm pretty sure I replied to this last week but can't find it, so
>> again]
>> Cc'ing Laszlo since he wrote this.
>>
>> On 5/30/19 9:28 PM, Olaf Hering wrote:
>>> The resulting firmware files should only contain the runtime path.
>>> Fixes commit 26ce90fde5c ("Makefile: install the edk2 firmware images
>>> and their descriptors")
>>>
>>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>>> ---
>>>  Makefile | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index f0be624f47..61267bf1a4 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -844,7 +844,7 @@ ifneq ($(DESCS),)
>>>  	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/firmware"
>>>  	set -e; tmpf=$$(mktemp); trap 'rm -f -- "$$tmpf"' EXIT; \
>>>  	for x in $(DESCS); do \
>>> -		sed -e 's,@DATADIR@,$(DESTDIR)$(qemu_datadir),' \
>>> +		sed -e 's,@DATADIR@,$(qemu_datadir),' \
>>
>> I'm fine with this patch.
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Laszlo, did you use the full path for a particular reason?
> 
> Yes. The reason was that I had no idea what DESTDIR stood for. After
> grepping the QEMU source tree for DESTDIR, I *still* have no idea what
> it stands for.
> 
> DESTDIR seems to be an installation prefix that is not controlled through
> 
>   ./configre --prefix=...
> 
> Based on memories from other projects -- with better documentation --
> though, DESTDIR looks like an install-time-only pre-prefix. In that
> sense, the patch looks correct (and the vague expression "runtime path"
> in the commit message starts to make some sense). Presumably DESTDIR is
> used in package build scripts... Yup, the RHEL spec files use it too,
> for "make install".
> 
> So, the patch is correct IMO:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Too bad QEMU is chronically under-documented (and the commit message on
> the patch doesn't help much in that regard).

At least the git history is public :)