Re: [dpdk-dev] [PATCH 1/8] build: add maths library to libs in pkg-config file

[Luca Boccassi <bluca@debian.org>]

On Wed, 2017-10-18 at 15:20 +0100, Bruce Richardson wrote:
> On Wed, Oct 18, 2017 at 01:24:54PM +0100, Bruce Richardson wrote:
> > On Wed, Oct 18, 2017 at 11:14:07AM +0100, Luca Boccassi wrote:
> > > On Wed, 2017-10-18 at 10:51 +0100, Bruce Richardson wrote:
> > > > On Wed, Oct 18, 2017 at 10:35:48AM +0100, Bruce Richardson
> > > > wrote:
> > > > > On Tue, Oct 17, 2017 at 07:17:09PM +0100, Luca Boccassi
> > > > > wrote:
> > > > > > On Tue, 2017-10-17 at 19:11 +0100, Luca Boccassi wrote:
> > > > > > > On Tue, 2017-10-17 at 17:12 +0100, Bruce Richardson
> > > > > > > wrote:
> > > > > > > > Since a number of libraries depend on the maths lib, as
> > > > > > > > well
> > > > > > > > as
> > > > > > > > adding it
> > > > > > > > to the project args, we also need to add it to the
> > > > > > > > pkgconfig
> > > > > > > > file
> > > > > > > > args.
> > > > > > > > 
> > > > > > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel
> > > > > > > > .com>
> > > > > > > > ---
> > > > > > > >  config/meson.build | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > 
> > > > > > > > diff --git a/config/meson.build b/config/meson.build
> > > > > > > > index db68a08d4..542fea4de 100644
> > > > > > > > --- a/config/meson.build
> > > > > > > > +++ b/config/meson.build
> > > > > > > > @@ -35,6 +35,7 @@ dpdk_conf.set('RTE_MACHINE', machine)
> > > > > > > >  add_project_arguments('-march=@0@'.format(machine),
> > > > > > > > language: 'c')
> > > > > > > >  # some libs depend on maths lib
> > > > > > > >  add_project_link_arguments('-lm', language: 'c')
> > > > > > > > +dpdk_extra_ldflags += '-lm'
> > > > > > > >  
> > > > > > > >  # add -include rte_config to cflags
> > > > > > > >  add_project_arguments('-include', 'rte_config.h',
> > > > > > > > language:
> > > > > > > > 'c')
> > > > > > > 
> > > > > > > This is for static builds, right? If so it should go into
> > > > > > > the
> > > > > > > Libs.private section of the .pc file, so that it's only
> > > > > > > used
> > > > > > > when
> > > > > > > calling pkg-config --static --libs
> > > > > > 
> > > > > > Bit of a brain fart - what I meant is, in order to have
> > > > > > static
> > > > > > builds
> > > > > > work out of the box with pkg-config --static, -lm (and any
> > > > > > other
> > > > > > dependency used internally) could also be added to
> > > > > > Libs.private
> > > > > > in the
> > > > > > .pc
> > > > > 
> > > > > Does that not assume that both static and dynamic libs are
> > > > > installed
> > > > > side-by-side? In DPDK case, we will either build static libs
> > > > > or
> > > > > shared
> > > > > libs, but not both. If we require applications to use
> > > > > different
> > > > > pkg-config commands depending on the type of DPDK build that
> > > > > was
> > > > > done,
> > > > > it makes things awkward for the apps. Right now by putting
> > > > > all libs
> > > > > and
> > > > > flags into the libs section of pkgconfig, and having the
> > > > > build
> > > > > system
> > > > > track whether it's static or dynamic and therefore what is
> > > > > actually
> > > > > necessary, we end up in a case where apps can be built
> > > > > against DPDK
> > > > > irrespective of the actual build type done. For this
> > > > > particular -lm
> > > > > flag, for instance, it only appears in the .pc file for
> > > > > static
> > > > > builds.
> > > > > 
> > > > > See the patches for the sample app Makefiles. Not sure how
> > > > > that can
> > > > > be
> > > > > made to work if we use different pkg-config settings for
> > > > > different
> > > > > build
> > > > > types.
> > > > > 
> > > > > Your input and suggestions here would be welcome.
> > > 
> > > Yes that works nicely when the libraries are rebuilt locally -
> > > then it
> > > can be chosen whether to build the static or dynamic ones.
> > > 
> > > In the packaging case though, at least in Debian and Ubuntu (not
> > > sure
> > > about RHEL/SUSE), we do ship both static and dynamic libraries at
> > > the
> > > same time, following best practices. So we'd have to choose and
> > > either
> > > cause applications linking dynamically to over-link (which is
> > > what we
> > > currently do in Debian/Ubuntu and many developers really don't
> > > like
> > > that) or to cause applications that use the static libraries to
> > > have to
> > > manually add the missing flags.
> > > 
> > > From what I can see, the most common workflow for applications
> > > that
> > > want to do static builds when using packaged libraries is to use
> > > the --
> > > static flag of pkgconfig.
> > > This has the advantage of being a ""standardised"" workflow,
> > > expected
> > > to work across any dependency, not just DPDK. Of course that's
> > > not
> > > always the case, but one can dream :-)
> > > 
> > > If you prefer to optimise for the local build workflow that's
> > > fine, we
> > > can patch it in the distro, it's not too much overhead (I need to
> > > do it
> > > anyway for the old build system at some point).
> > > 
> > > > 
> > > > +Thomas, Olivier, Sergio to get more input
> > > > 
> > > > Thinking about it some more, is there any reason why we can't
> > > > or
> > > > shouldn't do both static and dynamic libs in all builds, and
> > > > then let
> > > > apps use pkg-config to determine what they want to link
> > > > against? It
> > > > wouldn't be a massive change to the new build system to do
> > > > that, I
> > > > think.
> > > > 
> > > > /Bruce
> > > 
> > > Yes that would be ideal! Right now in Debian/Ubuntu we have to
> > > build
> > > twice, doubling the required time unfortunately.
> > > 
> > > In theory the same objects could be packed into .ar and .so but
> > > sadly
> > > Meson doesn't support that yet like autotools/cmake do:
> > > 
> > > https://github.com/mesonbuild/meson/issues/484
> > > 
> > > (please do add some feedback there as developers!)
> > > 
> > > It looks like it could be possible with some extensive hacking,
> > > not
> > > sure if it would be worth it.
> > > 
> > 
> > Actually, I think it's workable using extract_objects. Build the
> > static
> > lib first, then extract_all_objects() and then use those to build
> > the .so.
> > I can prototype it very easily by changing lib/meson.build and see
> > what
> > happens.
> > 
> 
> Seems to work fine. This change makes all the libraries (bar EAL,
> which
> is special-cased) appear as both .a and .so. Testpmd links against
> the
> dynamic versions and still works.
> 
> When I get the chance I'll see about doing the same for EAL and
> drivers,
> and then having the pkgconfig file reflect both possibilities. Then I
> can test with some examples linking both dynamically and statically
> and
> check all is as expected.
> 
> /Bruce
> 
> diff --git a/lib/meson.build b/lib/meson.build
> index f04cb2791..29c88548b 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -76,6 +76,20 @@ foreach l:libraries
>                         dep_objs += [get_variable('dep_rte_' + d)]
>                 endforeach
> 
> +               libname = 'rte_' + name
> +
> +# first build static library
> +               static_lib = static_library(libname, sources,
> +                       objects: objs,
> +                       c_args: cflags,
> +                       dependencies: dep_objs,
> +                       include_directories:
> include_directories(dir_name),
> +                       install: true)
> +
> +# now use those objects to build dynamic library
> +               sources = []
> +               objs += static_lib.extract_all_objects()
> +
>                 if get_option('per_library_versions')
>                         lib_version = '@0@.1'.format(version)
>                         so_version = '@0@'.format(version)
> @@ -87,8 +101,7 @@ foreach l:libraries
> 
>                 version_map = '@0@/@1@/rte_@2@_version.map'.format(
>                                 meson.current_source_dir(), dir_name,
> name)
> -               libname = 'rte_' + name
> -               lib = library(libname,
> +               lib = shared_library(libname,
>                                 sources,
>                                 objects: objs,
>                                 c_args: cflags,

Nice! Much easier than I thought reading that thread :-)

I'll try to give it a test run in the next few days. Thanks!

-- 
Kind regards,
Luca Boccassi