Re: [Qemu-devel] [PATCH v4] buildsys: Move crypto cflags/libs to per object variables

[Daniel P. Berrange]

On Fri, Sep 08, 2017 at 06:27:01PM +0800, Fam Zheng wrote:
> On Fri, 09/08 11:05, Daniel P. Berrange wrote:
> > On Wed, Sep 06, 2017 at 08:49:00PM +0800, Fam Zheng wrote:
> > > This patch groups the crypto objects into a few .mo objects based on
> > > functional submodules, and moves inclusion conditions to *-objs
> > > variables, then moves the global cflags/libs to the *-cflags and *-libs
> > > variables.
> > > 
> > > For init.o and cipher.o, which may or may not need the library flags
> > > depending on config, adding flags and libs unconditionally doesn't hurt,
> > > because if the library is not available, the variables are empty.  This
> > > makes less code.
> > > 
> > > Signed-off-by: Fam Zheng <address@hidden>
> > > 
> > > ---
> > > 
> > > v4: Merge into one patch which is supposedly easier to manage and
> > > review, and use .mo appraoch to avoid $(foreach) and $(eval) magics.
> > 
> > I don't think using  .mo is suitable here. You've used it as a generic
> > mechanism for grouping .o files, but that is not what it does. There
> > are special semantics around .mo rules that affect how the final
> > binaries are linked.
> 
> Using .mo is okay here, but after a hindsight I think grouping by library
> (nettle.mo, gcrypt.mo, etc.) is better than grouping by functionality, for
> modularization in the future. But that also means assigning the cflags/libs
> variable cannot be simplified like this.
> 
> > 
> > eg looking back at the description of .mo files 
> > 
> > [quote]
> > commit c261d774fb9093d00e0938a19f502fb220f62718
> > Author: Fam Zheng <address@hidden>
> > Date:   Mon Sep 1 18:35:10 2014 +0800
> > 
> > [...snip...]
> > 
> >     3) When linking an executable, those .mo files in its "-y" variables are
> >        filtered out, and replaced by one or more -Wl,-u,$symbol flags. This
> >        is done in the added macro "process-archive-undefs".
> >     
> >        These "-Wl,-u,$symbol" flags will force ld to pull in the function
> >        definition from the archives when linking.
> >     
> >        Note that the .mo objects, that are actually meant to be linked in
> >        the executables, are already expanded in unnest-vars, before the
> >        linking command. So we are safe to simply filter out .mo for the
> >        purpose of pulling undefined symbols.
> >     
> >        process-archive-undefs works as this: For each ".mo", find all the
> >        undefined symbols in it, filter ones that are defined in the
> >        archives. For each of these symbols, generate a "-Wl,-u,$symbol" in
> >        the link command, and put them before archive names in the command
> >        line.
> > [/quote]
> > 
> > Based on this, I don't think I can ack this patch, because it can
> > have unexpected consequences.
> 
> This described the process-archive-undefs semantics of .mo, but not the 
> essence
> of it.  Basically .mo is just partial linking with the additional services of
> -cflags, -libs and the above -Wl,-u thing. I cannot think of any unexpected
> consequences with this change. We've had sdl.mo in ui/Makefile.objs for long,
> just for the same purpose of this patch, with no problem.

While I'm in favour of moving the linker/compiler flags out of the global
vars, I'm not convinced this impl is a step forward.

We already have a mechanism for grouping object files - the 'NNNN-obj-y'
variables we use throughout our Makefiles.

This patch is adding a second level of grouping purely to work around the
fact that we can't set linker/compiler flags on the NNN-obj-y variables
we use. I think this second level of grouping makes the makefiles more
complex than they ought to be.

IOW, I'd rather see the rules fixed so that we can set variables against
the existing grouping we have. eg

   crypto-obj-y-cflags := ...
   crypto-obj-y-libs := ...

so we avoid having to introduce second level groups every time we want
to set these cflags/libs.

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 :|