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

[Bruce Richardson <bruce.richardson@intel.com>]

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,