All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Michael Tokarev <mjt@tls.msk.ru>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [RFC PATCH 0/4] per-object libraries
Date: Wed, 19 Jun 2013 18:58:29 +0200	[thread overview]
Message-ID: <51C1E335.9010304@redhat.com> (raw)
In-Reply-To: <1371576844-28743-1-git-send-email-mjt@msgid.tls.msk.ru>

Il 18/06/2013 19:34, Michael Tokarev ha scritto:
> The following working patchset demonstrates a one step to plugins system:
> it moves various dependent libraries and stuff out from libs_softmmu or
> libs_tools to object-specific variables.  When that object is linked
> into final executable, corresponding libs are expanded and appended to
> the linking command line.
> 
> I took block/curl.o as an example (in the last patch).
> 
> This is a working example which can be used as is as a starting point
> to convert other similar cases.

I was thinking of this problem too while drafting the modules feature
page.  My solution had some duplication:

libs-$(CONFIG_CURL) += $(CURL_LIBS)
$(obj)/curl.so: libs-$(CONFIG_CURL) += $(CURL_LIBS)

where the .so rule would use $(libs-y) $(libs-m).

> There are a few questions still.
> 
> I'm not sure whenever this $(obj)foo.o syntax (instead of $(obj)/foo.o)
> is okay, using the slash is more natural, but when you realize that
> objects can be stored in current dir it may be okay.  However, it is
> easy to make mistakes here in new code -- probably trivially catchable.
> 
> The foo.cflags isn't really necessary, but when you specify one
> thing one way (target-specific variable), and another thing completely
> different way, resulting code does not look nice.  In particular, the
> two curl definitions in block/Makefile.objs look somewhat funky if
> curl.cflags isn't used.

I like this solution, and I agree that consistency between cflags and
libs is good.  However, it would be great if you could do it without
changing the meaning of $(obj).  It is not clear to me (from reading the
patches) why you need that.

Also, for the inevitable bikeshedding, I would prefer

   cflags-$(obj)/curl.o-y
   libs-$(obj)/curl.o-y

> It is quite a bit ugly to strip out ../ in the link line, but it is
> also ugly to add that ../ in the first place.  Maybe I should add a
> comment in Makefile.target where it adds ../ to also refer to
> rules.mak, and back, for clarity.

The ../ is ugly indeed.  Perhaps we can instead use something like

common.o: $(patsubst %,../%, $(common-obj-y))
	$(LD) -r -o $@ $^

and then link common.o into the QEMU target.  Libtool can also be used
to abstract "ld -r".  Making libtool mandatory wouldn't be a problem IMO
(we'd need it anyway for modules) as long as you do not need libtool to
start QEMU or gdb from the build directory.

> In configure, we may define $(obj)curl.libs directly, but for that
> to work we should know where exactly the curl.o is located.
> 
> At the same time, we may just as well use basenames (without paths)
> to define per-object variables -- this way we wont need to strip
> ./ from $(obj) or any other black magic, but we should ensure that
> all basenames are unique, or else bad things may happen.

This would be worse than the disease, I think.

> When you build a library from an object, its libs aren't propagated.
> It is fixable, by creating library.libs variables and expanding them
> at executable link time, but since not all objects from the lib may
> be necessary, this isn't nice.
> 
> Generally, is is expected that obj.libs variables will be used only
> for common optional objects like "plugins".

Not necessarily! :)

Paolo

> Michael Tokarev (4):
>   build-sys: strip leading ./ from $(obj)
>   build-sys: allow object-specific libraries to be used to link executables
>   build-sys: allow per-object foo.cflags variables
>   build-sys: move -lcurl out of libs and specify it for curl.o
> 
>  audio/Makefile.objs    |    4 ++--
>  backends/Makefile.objs |    2 +-
>  block/Makefile.objs    |    4 ++--
>  configure              |    3 +--
>  hw/audio/Makefile.objs |    2 +-
>  rules.mak              |   16 ++++++++++------
>  trace/Makefile.objs    |   26 +++++++++++++-------------
>  ui/Makefile.objs       |    6 +++---
>  8 files changed, 33 insertions(+), 30 deletions(-)
> 

  parent reply	other threads:[~2013-06-19 16:58 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-18 17:34 [Qemu-devel] [RFC PATCH 0/4] per-object libraries Michael Tokarev
2013-06-18 17:34 ` [Qemu-devel] [RFC PATCH 1/4] build-sys: strip leading ./ from $(obj) Michael Tokarev
2013-06-18 17:34 ` [Qemu-devel] [RFC PATCH 2/4] build-sys: allow object-specific libraries to be used to link executables Michael Tokarev
2013-06-18 17:34 ` [Qemu-devel] [RFC PATCH 3/4] build-sys: allow per-object foo.cflags variables Michael Tokarev
2013-06-18 17:34 ` [Qemu-devel] [RFC PATCH 4/4] build-sys: move -lcurl out of libs and specify it for curl.o Michael Tokarev
2013-06-19  0:41 ` [Qemu-devel] [RFC PATCH 0/4] per-object libraries Michael Tokarev
2013-06-19 14:16 ` Stefan Hajnoczi
2013-06-19 14:58   ` Michael Tokarev
2013-06-19 16:46   ` Paolo Bonzini
2013-06-19 16:58 ` Paolo Bonzini [this message]
2013-06-19 18:18   ` Michael Tokarev
2013-06-19 18:52     ` Paolo Bonzini
2013-06-19 19:31       ` Richard Henderson
     [not found]         ` <51C2D03E.2030505@redhat.com>
2013-06-20 10:06           ` Peter Maydell
2013-06-20 12:39             ` Paolo Bonzini
2013-06-20 12:50               ` Peter Maydell
2013-06-20 17:09           ` Richard Henderson
2013-06-19 20:00       ` Michael Tokarev
2013-06-20 10:09         ` Paolo Bonzini
2013-06-30 15:23   ` Michael Tokarev
2013-07-01 10:08     ` Paolo Bonzini
2013-07-01 10:10       ` Michael Tokarev
2013-07-01 10:18         ` Paolo Bonzini
2013-06-30 15:28 ` Andreas Färber
2013-06-30 15:36   ` Michael Tokarev
2013-06-30 15:51     ` Peter Maydell
2013-06-30 16:49       ` Michael Tokarev
2013-07-01  8:00         ` Stefan Hajnoczi
2013-06-30 15:56     ` Andreas Färber
2013-07-01 13:39     ` Paolo Bonzini
2013-07-01 14:43       ` Michael Tokarev
2013-07-01 14:46         ` Andreas Färber
2013-07-01 14:52           ` Michael Tokarev
2013-07-01 14:53           ` Paolo Bonzini
2013-07-01 15:06             ` Michael Tokarev
2013-07-01 15:20               ` Paolo Bonzini
2013-07-01 15:52                 ` Michael Tokarev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51C1E335.9010304@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=mjt@tls.msk.ru \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.