| Message ID | 20200626164307.3327380-5-dinechin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Move SPICE to a load module | expand |
On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote: > If we want to build spice as a separately loadable module, we need to > put all the spice code in one loadable module, because the build > system does not know how to deal with dependencies yet. > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> > --- > audio/Makefile.objs | 2 +- > chardev/Makefile.objs | 3 +-- > ui/Makefile.objs | 8 ++++---- > 3 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/audio/Makefile.objs b/audio/Makefile.objs > index b4a4c11f31..298c895ff5 100644 > --- a/audio/Makefile.objs > +++ b/audio/Makefile.objs > @@ -1,5 +1,5 @@ > common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o > -common-obj-$(CONFIG_SPICE) += spiceaudio.o > +spice-app.mo-objs += ../audio/spiceaudio.o Explicitly showing paths in the variables doesn't look right. The make recipes are supposed to automatically expand bare file names to add the right path. This is usually dealt with by a call to the "unnest-vars" function. > common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o > common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o > common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o > diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs > index fc9910d4f2..955fac0cf9 100644 > --- a/chardev/Makefile.objs > +++ b/chardev/Makefile.objs > @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o > baum.o-cflags := $(SDL_CFLAGS) > baum.o-libs := $(BRLAPI_LIBS) > > -common-obj-$(CONFIG_SPICE) += spice.mo > -spice.mo-objs := spice.o > +spice-app.mo-objs += ../chardev/spice.o > diff --git a/ui/Makefile.objs b/ui/Makefile.objs > index 504b196479..1ab515e23d 100644 > --- a/ui/Makefile.objs > +++ b/ui/Makefile.objs > @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o > common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o > common-obj-y += input-barrier.o > common-obj-$(CONFIG_LINUX) += input-linux.o > -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o > common-obj-$(CONFIG_COCOA) += cocoa.o > common-obj-$(CONFIG_VNC) += $(vnc-obj-y) > common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o > @@ -53,10 +52,11 @@ curses.mo-objs := curses.o > curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS) > curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS) > > -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy) > -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo > +common-obj-$(CONFIG_SPICE) += spice-app.mo > +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o > +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym) > +spice-app.mo-objs += spice-app.o > endif > -spice-app.mo-objs := spice-app.o > spice-app.mo-cflags := $(GIO_CFLAGS) > spice-app.mo-libs := $(GIO_LIBS) > > -- > 2.26.2 > > Regards, Daniel
On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote... > On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote: >> If we want to build spice as a separately loadable module, we need to >> put all the spice code in one loadable module, because the build >> system does not know how to deal with dependencies yet. >> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> >> --- >> audio/Makefile.objs | 2 +- >> chardev/Makefile.objs | 3 +-- >> ui/Makefile.objs | 8 ++++---- >> 3 files changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/audio/Makefile.objs b/audio/Makefile.objs >> index b4a4c11f31..298c895ff5 100644 >> --- a/audio/Makefile.objs >> +++ b/audio/Makefile.objs >> @@ -1,5 +1,5 @@ >> common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o >> -common-obj-$(CONFIG_SPICE) += spiceaudio.o >> +spice-app.mo-objs += ../audio/spiceaudio.o > > Explicitly showing paths in the variables doesn't look right. The > make recipes are supposed to automatically expand bare file names > to add the right path. This is usually dealt with by a call to > the "unnest-vars" function. I agree. I mentioned this in the cover letter: > - Adding various non-UI files into spice-app.so, which required a > couple of ../pwd/foo.o references in the makefile. Not very nice, > but I did not want to spend too much time fixing the makefiles. I did not find an elegant way to integrate a non-UI file into a .mo that is declared in the ui section. I considered various solutions: a) Having separate load modules for different source directories. This exposes details of the build system into the generated libs. b) Moving the source This breaks the logical organization of the sources c) Manually managing this specific .o one level up This seems even harder to read / understand So I thought I would dedicate a bit more time to add that capability to the unnest-vars function. I did not want to defer review for that, and vaguely hoped that there was an existing correct way to do it that someone more experienced in the build system could point me to. > >> common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o >> common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o >> common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o >> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs >> index fc9910d4f2..955fac0cf9 100644 >> --- a/chardev/Makefile.objs >> +++ b/chardev/Makefile.objs >> @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o >> baum.o-cflags := $(SDL_CFLAGS) >> baum.o-libs := $(BRLAPI_LIBS) >> >> -common-obj-$(CONFIG_SPICE) += spice.mo >> -spice.mo-objs := spice.o >> +spice-app.mo-objs += ../chardev/spice.o >> diff --git a/ui/Makefile.objs b/ui/Makefile.objs >> index 504b196479..1ab515e23d 100644 >> --- a/ui/Makefile.objs >> +++ b/ui/Makefile.objs >> @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o >> common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o >> common-obj-y += input-barrier.o >> common-obj-$(CONFIG_LINUX) += input-linux.o >> -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o >> common-obj-$(CONFIG_COCOA) += cocoa.o >> common-obj-$(CONFIG_VNC) += $(vnc-obj-y) >> common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o >> @@ -53,10 +52,11 @@ curses.mo-objs := curses.o >> curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS) >> curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS) >> >> -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy) >> -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo >> +common-obj-$(CONFIG_SPICE) += spice-app.mo >> +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o >> +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym) >> +spice-app.mo-objs += spice-app.o >> endif >> -spice-app.mo-objs := spice-app.o >> spice-app.mo-cflags := $(GIO_CFLAGS) >> spice-app.mo-libs := $(GIO_LIBS) >> >> -- >> 2.26.2 >> >> > > Regards, > Daniel -- Cheers, Christophe de Dinechin (IRC c3d)
On Mon, Jun 29, 2020 at 11:22:40AM +0200, Christophe de Dinechin wrote: > > On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote... > > On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote: > >> If we want to build spice as a separately loadable module, we need to > >> put all the spice code in one loadable module, because the build > >> system does not know how to deal with dependencies yet. > >> > >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> > >> --- > >> audio/Makefile.objs | 2 +- > >> chardev/Makefile.objs | 3 +-- > >> ui/Makefile.objs | 8 ++++---- > >> 3 files changed, 6 insertions(+), 7 deletions(-) > >> > >> diff --git a/audio/Makefile.objs b/audio/Makefile.objs > >> index b4a4c11f31..298c895ff5 100644 > >> --- a/audio/Makefile.objs > >> +++ b/audio/Makefile.objs > >> @@ -1,5 +1,5 @@ > >> common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o > >> -common-obj-$(CONFIG_SPICE) += spiceaudio.o > >> +spice-app.mo-objs += ../audio/spiceaudio.o > > > > Explicitly showing paths in the variables doesn't look right. The > > make recipes are supposed to automatically expand bare file names > > to add the right path. This is usually dealt with by a call to > > the "unnest-vars" function. > > I agree. I mentioned this in the cover letter: > > > - Adding various non-UI files into spice-app.so, which required a > > couple of ../pwd/foo.o references in the makefile. Not very nice, > > but I did not want to spend too much time fixing the makefiles. > > I did not find an elegant way to integrate a non-UI file into a .mo that is > declared in the ui section. > > I considered various solutions: > > a) Having separate load modules for different source directories. > This exposes details of the build system into the generated libs. > > b) Moving the source > This breaks the logical organization of the sources > > c) Manually managing this specific .o one level up > This seems even harder to read / understand I don't think any of these three should be required. The problem isn't the structure of the makefiles or layout of the source, rather it is a matter of expanding paths correctly in the build rules. > So I thought I would dedicate a bit more time to add that capability to the > unnest-vars function. I did not want to defer review for that, and vaguely > hoped that there was an existing correct way to do it that someone more > experienced in the build system could point me to. With QEMU's build system, regardless of where the rules are defined, they should get run in the context of the top level makefile, as all the sub-dir Makefiles are just includes. The unnest-vars function takes relative filenames and adds the correct directory prefix to them. IIUC, common-obj-m gets spice-app.mo added. common-obj-m is processed by unnest-vars, but spice-app.mo-objs is not, so all its .o files are relative. So we just need to fix the way the *.mo-objs variables are processed, such that unnest-vars is used to add the directory prefixes. Regards, Daniel
Hi, > common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o > -common-obj-$(CONFIG_SPICE) += spiceaudio.o > +spice-app.mo-objs += ../audio/spiceaudio.o Hmm. audio/audio.c will try to load audio-${backend}.so when you run qemu -audiodev ${backend}, so I suspect this is not going to work ... I guess we should better try to tackle modules depending on modules problem. Given that g_module_open() doesn't support a custom shared object search path I suspect we can't simply link audio-spice.so against ui-spice-core.so and let the dynamic linker handle this for us. Probably qemu needs a list of dependencies it'll check on module load ... take care, Gerd
diff --git a/audio/Makefile.objs b/audio/Makefile.objs index b4a4c11f31..298c895ff5 100644 --- a/audio/Makefile.objs +++ b/audio/Makefile.objs @@ -1,5 +1,5 @@ common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o -common-obj-$(CONFIG_SPICE) += spiceaudio.o +spice-app.mo-objs += ../audio/spiceaudio.o common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs index fc9910d4f2..955fac0cf9 100644 --- a/chardev/Makefile.objs +++ b/chardev/Makefile.objs @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o baum.o-cflags := $(SDL_CFLAGS) baum.o-libs := $(BRLAPI_LIBS) -common-obj-$(CONFIG_SPICE) += spice.mo -spice.mo-objs := spice.o +spice-app.mo-objs += ../chardev/spice.o diff --git a/ui/Makefile.objs b/ui/Makefile.objs index 504b196479..1ab515e23d 100644 --- a/ui/Makefile.objs +++ b/ui/Makefile.objs @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o common-obj-y += input-barrier.o common-obj-$(CONFIG_LINUX) += input-linux.o -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o common-obj-$(CONFIG_COCOA) += cocoa.o common-obj-$(CONFIG_VNC) += $(vnc-obj-y) common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o @@ -53,10 +52,11 @@ curses.mo-objs := curses.o curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS) curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS) -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy) -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo +common-obj-$(CONFIG_SPICE) += spice-app.mo +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym) +spice-app.mo-objs += spice-app.o endif -spice-app.mo-objs := spice-app.o spice-app.mo-cflags := $(GIO_CFLAGS) spice-app.mo-libs := $(GIO_LIBS)
If we want to build spice as a separately loadable module, we need to put all the spice code in one loadable module, because the build system does not know how to deal with dependencies yet. Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> --- audio/Makefile.objs | 2 +- chardev/Makefile.objs | 3 +-- ui/Makefile.objs | 8 ++++---- 3 files changed, 6 insertions(+), 7 deletions(-)