diff mbox series

[v2,1/3] stubdom: fix build with disabled pv-grub

Message ID 20210909124924.1698-2-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series disable building of pv-grub and qemu-trad per default | expand

Commit Message

Jürgen Groß Sept. 9, 2021, 12:49 p.m. UTC 
Today the build will fail if --disable-pv-grub as a parameter of
configure, as the main Makefile will unconditionally try to build a
32-bit pv-grub stubdom.

Fix that by introducing a pv-grub32 target in stubdom/Makefile taking
care of this situation.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 Makefile         |  4 ++--
 stubdom/Makefile | 13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Ian Jackson Sept. 9, 2021, 1:23 p.m. UTC | #1 
Juergen Gross writes ("[PATCH v2 1/3] stubdom: fix build with disabled pv-grub"):
> Today the build will fail if --disable-pv-grub as a parameter of
> configure, as the main Makefile will unconditionally try to build a
> 32-bit pv-grub stubdom.
> 
> Fix that by introducing a pv-grub32 target in stubdom/Makefile taking
> care of this situation.

I approve of this whole series, with one resrvation:

I think the name "pv-grub32" for this target is confusing.  It's not
really specifically 32-bit.  The difference between the targets
"pv-grub32" and "pv-grub" is that "pv-grub32" is unconditionally
built but might mean nothing; it has a conditional dependency on
"pv-grub" which is conditionally built but always implies the actual
build.

I don't think the suffix "32" really conveys this :-).

How about "pv-grub-maybe" ?  Or something.

You can put my ack on patches 2 and 3 right away.

Thanks,
Ian.
Jürgen Groß Sept. 9, 2021, 1:59 p.m. UTC | #2 
On 09.09.21 15:23, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH v2 1/3] stubdom: fix build with disabled pv-grub"):
>> Today the build will fail if --disable-pv-grub as a parameter of
>> configure, as the main Makefile will unconditionally try to build a
>> 32-bit pv-grub stubdom.
>>
>> Fix that by introducing a pv-grub32 target in stubdom/Makefile taking
>> care of this situation.
> 
> I approve of this whole series, with one resrvation:
> 
> I think the name "pv-grub32" for this target is confusing.  It's not
> really specifically 32-bit.  The difference between the targets
> "pv-grub32" and "pv-grub" is that "pv-grub32" is unconditionally
> built but might mean nothing; it has a conditional dependency on
> "pv-grub" which is conditionally built but always implies the actual
> build.
> 
> I don't think the suffix "32" really conveys this :-).
> 
> How about "pv-grub-maybe" ?  Or something.

What about "pv-grub-if-enabled"?

And could that be done when committing, or should I send another round?

> 
> You can put my ack on patches 2 and 3 right away.

Thanks,


Juergen
Ian Jackson Sept. 9, 2021, 4:08 p.m. UTC | #3 
Juergen Gross writes ("Re: [PATCH v2 1/3] stubdom: fix build with disabled pv-grub"):
> On 09.09.21 15:23, Ian Jackson wrote:
> > How about "pv-grub-maybe" ?  Or something.
> 
> What about "pv-grub-if-enabled"?

Fine by me.

> And could that be done when committing, or should I send another round?

Please do send another round (sorry).

Thanks,
Ian.
Jürgen Groß Sept. 10, 2021, 5:48 a.m. UTC | #4 
On 09.09.21 18:08, Ian Jackson wrote:
> Juergen Gross writes ("Re: [PATCH v2 1/3] stubdom: fix build with disabled pv-grub"):
>> On 09.09.21 15:23, Ian Jackson wrote:
>>> How about "pv-grub-maybe" ?  Or something.
>>
>> What about "pv-grub-if-enabled"?
> 
> Fine by me.
> 
>> And could that be done when committing, or should I send another round?
> 
> Please do send another round (sorry).

NP.

BTW, you probably want to modify OSStest to use configure with:

--enable-pv-grub --enable-qemu-traditional

in order to not let the tests fail (applies to x86 only, of course).


Juergen
Ian Jackson Sept. 10, 2021, 3:32 p.m. UTC | #5 
Juergen Gross writes ("Re: [PATCH v2 1/3] stubdom: fix build with disabled pv-grub"):
> BTW, you probably want to modify OSStest to use configure with:
> 
> --enable-pv-grub --enable-qemu-traditional
> 
> in order to not let the tests fail (applies to x86 only, of course).

I think it might be better to disable those tests.  What do people
think ?

Ian.
Jürgen Groß Sept. 13, 2021, 4:51 a.m. UTC | #6 
On 10.09.21 17:32, Ian Jackson wrote:
> Juergen Gross writes ("Re: [PATCH v2 1/3] stubdom: fix build with disabled pv-grub"):
>> BTW, you probably want to modify OSStest to use configure with:
>>
>> --enable-pv-grub --enable-qemu-traditional
>>
>> in order to not let the tests fail (applies to x86 only, of course).
> 
> I think it might be better to disable those tests.  What do people
> think ?

Disabling pv-grub tests is okay (I think it happened already, right?).

Disabling qemu-trad tests with qemu running in dom0 is fine, too, IMO.

Disabling the stubdom tests is much more concerning, as there is quite
some code in the tools which would be untested then.


Juergen
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 96d32cfd50..5b5cef3e49 100644
--- a/Makefile
+++ b/Makefile
@@ -72,7 +72,7 @@  build-tools-oxenstored: build-tools-public-headers
 build-stubdom: mini-os-dir build-tools-public-headers
 	$(MAKE) -C stubdom build
 ifeq (x86_64,$(XEN_TARGET_ARCH))
-	XEN_TARGET_ARCH=x86_32 $(MAKE) -C stubdom pv-grub
+	XEN_TARGET_ARCH=x86_32 $(MAKE) -C stubdom pv-grub32
 endif
 
 .PHONY: build-docs
@@ -143,7 +143,7 @@  install-tools: install-tools-public-headers
 install-stubdom: mini-os-dir install-tools
 	$(MAKE) -C stubdom install
 ifeq (x86_64,$(XEN_TARGET_ARCH))
-	XEN_TARGET_ARCH=x86_32 $(MAKE) -C stubdom install-grub
+	XEN_TARGET_ARCH=x86_32 $(MAKE) -C stubdom install-grub32
 endif
 
 .PHONY: tools/firmware/seabios-dir-force-update
diff --git a/stubdom/Makefile b/stubdom/Makefile
index 06aa69d8bc..b339ae701c 100644
--- a/stubdom/Makefile
+++ b/stubdom/Makefile
@@ -531,6 +531,13 @@  vtpmmgr-stubdom: mini-os-$(XEN_TARGET_ARCH)-vtpmmgr vtpmmgr
 pv-grub: mini-os-$(XEN_TARGET_ARCH)-grub libxenguest grub
 	DEF_CPPFLAGS="$(TARGET_CPPFLAGS)" DEF_CFLAGS="$(TARGET_CFLAGS)" DEF_LDFLAGS="$(TARGET_LDFLAGS)" MINIOS_CONFIG="$(CURDIR)/grub/minios.cfg" $(MAKE) DESTDIR= -C $(MINI_OS) OBJ_DIR=$(CURDIR)/$< APP_OBJS=$(CURDIR)/grub-$(XEN_TARGET_ARCH)/main.a
 
+.PHONY: pv-grub32
+ifneq ($(filter grub,$(STUBDOM_TARGETS)),)
+pv-grub32: pv-grub
+else
+pv-grub32:
+endif
+
 .PHONY: xenstore-stubdom
 xenstore-stubdom: mini-os-$(XEN_TARGET_ARCH)-xenstore libxenguest xenstore
 	DEF_CPPFLAGS="$(TARGET_CPPFLAGS)" DEF_CFLAGS="$(TARGET_CFLAGS)" DEF_LDFLAGS="$(TARGET_LDFLAGS)" MINIOS_CONFIG="$(CURDIR)/xenstore-minios.cfg" $(MAKE) DESTDIR= -C $(MINI_OS) OBJ_DIR=$(CURDIR)/$< APP_OBJS=$(CURDIR)/xenstore/xenstored.a
@@ -560,6 +567,12 @@  install-grub: pv-grub
 	$(INSTALL_DIR) "$(DESTDIR)$(XENFIRMWAREDIR)"
 	$(INSTALL_DATA) mini-os-$(XEN_TARGET_ARCH)-grub/mini-os.gz "$(DESTDIR)$(XENFIRMWAREDIR)/pv-grub-$(XEN_TARGET_ARCH).gz"
 
+ifneq ($(filter grub,$(STUBDOM_TARGETS)),)
+install-grub32: install-grub
+else
+install-grub32:
+endif
+
 install-c: c-stubdom
 
 install-caml: caml-stubdom