Re: [PATCH v2 1/2] build: build only one library type on Windows

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

On Fri, Apr 12, 2024 at 04:04:12PM -0700, Tyler Retzlaff wrote:
> On Fri, Apr 12, 2024 at 02:00:43PM +0100, Bruce Richardson wrote:
> > On Thu, Mar 14, 2024 at 11:30:21PM -0700, Tyler Retzlaff wrote:
> > > MSVC is the only compiler that can produce usable shared libraries for
> > > DPDK on Windows because of the use of exported TLS variables.
> > > 
> > > Disable building of shared libraries with LLVM and MinGW so that
> > > remaining __declspec macros needed for the functional libraries built by
> > > MSVC can be used without triggering errors in LLVM and MinGW builds.
> > > 
> > > For Windows only install the default_library type to avoid confusion.
> > > Windows builds cannot build both shared and static in a single pass so
> > > install only the functional variant.
> > > 
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > 
> > Comments inline below.
> > 
> > /Bruce
> > 
> > > ---
> > >  app/meson.build                       |  6 +++
> > >  config/meson.build                    | 27 ++++++++++++
> > >  drivers/meson.build                   | 72 +++++++++++++++----------------
> > >  drivers/net/octeontx/base/meson.build |  2 +-
> > >  examples/meson.build                  |  6 +++
> > >  lib/meson.build                       | 79 +++++++++++++++--------------------
> > >  6 files changed, 108 insertions(+), 84 deletions(-)
> > > 
> > > diff --git a/app/meson.build b/app/meson.build
> > > index 21b6da2..f4ed0f1 100644
> > > --- a/app/meson.build
> > > +++ b/app/meson.build
> > > @@ -46,6 +46,8 @@ default_cflags = machine_args + ['-DALLOW_EXPERIMENTAL_API']
> > >  default_ldflags = []
> > >  if get_option('default_library') == 'static' and not is_windows
> > >      default_ldflags += ['-Wl,--export-dynamic']
> > > +elif get_option('default_library') == 'shared' and is_ms_compiler
> > > +    default_ldflags += ['/experimental:tlsDllInterface']
> > >  endif
> > >  
> > 
> > Is this necessary, given that you add this flag as a project-wide argument
> > below in config/meson.build?
> 
> yes, i need to provide tlsDllInterface as an argument to both the
> compiler and the linker commands. project-wide argument from
> config/meson.build only adds the argument to the compiler command line
> but not the linker. there doesn't appear to be a global variable that
> can be used as the same for the linker so it has been added to the
> various ldflags/lkflags variables in individual meson.build files.
> 

The add_project_link_arguments() function should be what you are looking
for, I think.

https://mesonbuild.com/Reference-manual_functions.html#add_project_link_arguments

> > >  foreach app:apps
> > > @@ -104,6 +106,10 @@ foreach app:apps
> > >          link_libs = dpdk_static_libraries + dpdk_drivers
> > >      endif
> > >  
> > > +    if is_windows and is_shared_enabled
> > > +        cflags += '-DRTE_BUILD_SHARED_LIB'
> > > +    endif
> > > +
> > >      exec = executable('dpdk-' + name,
> > >              sources,
> > >              c_args: cflags,
> > > diff --git a/config/meson.build b/config/meson.build
> > > index 8c8b019..dd7971e 100644
> > > --- a/config/meson.build
> > > +++ b/config/meson.build
> > > @@ -53,6 +53,9 @@ if is_ms_compiler
> > >  
> > >      # enable statement expressions extension
> > >      add_project_arguments('/experimental:statementExpressions', language: 'c')
> > > +
> > > +    # enable export of thread_local variables
> > > +    add_project_arguments('/experimental:tlsDllInterface', language: 'c')
> > >  endif
> 
> as per above, this only causes the argument to be added to the compiler
> not the linker.
> 
> > >  
> > >  # set the major version, which might be used by drivers and libraries
> > > @@ -516,4 +519,28 @@ if get_option('default_library') == 'both'
> > >   NOTE: DPDK always builds both shared and static libraries.  Please set
> > >   "default_library" to either "static" or "shared" to select default linkage
> > >   for apps and any examples.''')
> > > +elif get_option('default_library') == 'shared' and is_windows and not is_ms_compiler
> > > +    error( '''
> > > + Unsupported value "shared" for "default_library" option.
> > > +
> > > + NOTE: DPDK Windows shared is only supported when building with MSVC. Please set
> > > + "default_library" to either "static" or use MSVC.''')
> > > +endif
> > > +
> > > +if is_windows
> > > +    if is_ms_compiler and get_option('default_library') == 'shared'
> > > +        is_shared_enabled=true
> > > +    else
> > > +        is_shared_enabled=false
> > > +    endif
> > > +else
> > > +    is_shared_enabled=true
> > > +endif
> > > +
> > 
> > This can be shortened to:
> > 
> > is_shared_enabled=true
> > if is_windows and (not is_ms_compiler or not get_option(...) == 'shared')
> > 	is_shared_enabled=false
> > endif
> > 
> > 
> > > +if is_windows
> > > +    install_static = get_option('default_library') == 'static'
> > > +    install_shared = get_option('default_library') == 'shared'
> > > +else
> > > +    install_static=true
> > > +    install_shared=true
> > >  endif
> > 
> > Might be better merged with the block above:
> > 
> > is_shared_enabled = true
> > install_static = true
> > install_shared = true
> > if is_windows
> >     install_static = get_option('default_library') == 'static'
> >     install_shared = get_option('default_library') == 'shared'
> > 
> >     if not is_ms_compiler or not install_shared
> >         is_shared_enabled = false
> >     endif
> > endif
> 
> done in v3, much nicer thanks.
> 
> > 
> > > diff --git a/drivers/meson.build b/drivers/meson.build
> > > index 66931d4..c8b2d13 100644
> > > --- a/drivers/meson.build
> > > +++ b/drivers/meson.build
> > > @@ -80,7 +80,7 @@ foreach subpath:subdirs
> > >          subdir(class)
> > >          skip_class = false
> > >          foreach d:std_deps
> > > -            if not is_variable('shared_rte_' + d)
> > > +            if not is_variable('static_rte_' + d)
> > >                  skip_class = true
> > >                  reason = 'missing internal dependency, "@0@"'.format(d)
> > >                  if dpdk_libs_deprecated.contains(d)
> > > @@ -173,7 +173,7 @@ foreach subpath:subdirs
> > >                  if not build
> > >                      break
> > >                  endif
> > > -                if not is_variable('shared_rte_' + d)
> > > +                if not is_variable('static_rte_' + d)
> > >                      build = false
> > >                      reason = 'missing internal dependency, "@0@"'.format(d)
> > >                      if dpdk_libs_deprecated.contains(d)
> > > @@ -188,7 +188,9 @@ foreach subpath:subdirs
> > >                                  +'\tPlease enable missing dependency "@0@"'.format(d))
> > >                      endif
> > >                  else
> > > -                    shared_deps += [get_variable('shared_rte_' + d)]
> > > +                    if is_shared_enabled
> > > +                        shared_deps += [get_variable('shared_rte_' + d)]
> > > +                    endif
> > >                      static_deps += [get_variable('static_rte_' + d)]
> > >                  endif
> > >              endforeach
> > > @@ -208,6 +210,9 @@ foreach subpath:subdirs
> > >          enabled_drivers += name
> > >          lib_name = '_'.join(['rte', class, name])
> > >          cflags += '-DRTE_LOG_DEFAULT_LOGTYPE=' + '.'.join([log_prefix, name])
> > > +        if is_windows and is_shared_enabled
> > > +            cflags += '-DRTE_BUILD_SHARED_LIB'
> > > +        endif
> > 
> > Rather than setting this in cflags in the drivers or lib meson.build files,
> > can we set this once as a project-level cflag and be done with it?
> 
> i don't think so, there are drivers (notably mellanox) that appear to
> assume it is not always set by default even with a shared build. i think
> this one might best be cleaned up and clarified in a separate series.
> 

Oh, how does this assumption show itself or what impact has it? Using git
grep in the source code for the macro only shows the expected usage in
function_versioning header, and in lib/meson.build.

> > 
> > >          if annotate_locks and cc.get_id() == 'clang' and cc.version().version_compare('>=3.5.0')
> > >              cflags += '-DRTE_ANNOTATE_LOCKS'
> > >              cflags += '-Wthread-safety'
> > > @@ -247,7 +252,7 @@ foreach subpath:subdirs
> > >                  include_directories: includes,
> > >                  dependencies: static_deps,
> > >                  c_args: cflags,
> > > -                install: true)
> > > +                install: install_static)
> > >  
> > >          # now build the shared driver
> > >          version_map = '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
> > > @@ -271,55 +276,46 @@ foreach subpath:subdirs
> > >              endif
> > >          endif
> > >  
> > > -        if is_windows
> > > -            if is_ms_linker
> > > +        if is_shared_enabled
> > > +            if is_ms_compiler
> > >                  def_file = custom_target(lib_name + '_def',
> > >                          command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
> > >                          input: version_map,
> > >                          output: '@0@_exports.def'.format(lib_name))
> > >                  lk_deps += [def_file]
> > > -
> > > -                lk_args = ['-Wl,/def:' + def_file.full_path()]
> > > -                if meson.version().version_compare('<0.54.0')
> > > -                    lk_args += ['-Wl,/implib:drivers\\lib' + lib_name + '.dll.a']
> > > -                endif
> > > +                lk_args = ['/experimental:tlsDllInterface', '/def:' + def_file.full_path()]
> > 
> > Again, the /experimental flag shouldn't be necessary, since it's set as
> > project option in config/meson.build.
> 
> as per above, let me know if what i said seems crazy.
> 
> > 
> > >              else
> > > -                mingw_map = custom_target(lib_name + '_mingw',
> > > -                        command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
> > > -                        input: version_map,
> > > -                        output: '@0@_mingw.map'.format(lib_name))
> > > -                lk_deps += [mingw_map]
> > > -
> > > -                lk_args = ['-Wl,--version-script=' + mingw_map.full_path()]
> > > +                lk_args = ['-Wl,--version-script=' + version_map]
> > >              endif
> > > -        else
> > > -            lk_args = ['-Wl,--version-script=' + version_map]
> > >          endif
> > >  
> > > -        shared_lib = shared_library(lib_name, sources,
> > > -                objects: objs,
> > > -                include_directories: includes,
> > > -                dependencies: shared_deps,
> > > -                c_args: cflags,
> > > -                link_args: lk_args,
> > > -                link_depends: lk_deps,
> > > -                version: abi_version,
> > > -                soversion: so_version,
> > > -                install: true,
> > > -                install_dir: driver_install_path)
> > > -
> > > -        # create a dependency object and add it to the global dictionary so
> > > -        # testpmd or other built-in apps can find it if necessary
> > > -        shared_dep = declare_dependency(link_with: shared_lib,
> > > -                include_directories: includes,
> > > -                dependencies: shared_deps)
> > > +        if is_shared_enabled
> > > +            shared_lib = shared_library(lib_name, sources,
> > > +                    objects: objs,
> > > +                    include_directories: includes,
> > > +                    dependencies: shared_deps,
> > > +                    c_args: cflags,
> > > +                    link_args: lk_args,
> > > +                    link_depends: lk_deps,
> > > +                    version: abi_version,
> > > +                    soversion: so_version,
> > > +                    install: install_shared,
> > > +                    install_dir: driver_install_path)
> > > +
> > > +            # create a dependency object and add it to the global dictionary so
> > > +            # testpmd or other built-in apps can find it if necessary
> > > +            shared_dep = declare_dependency(link_with: shared_lib,
> > > +                    include_directories: includes,
> > > +                    dependencies: shared_deps)
> > > +
> > > +            set_variable('shared_@0@'.format(lib_name), shared_dep)
> > 
> > Just a thought - could you avoid some of the changes throughout this set,
> > if you add an else-leg to the "is_shared_enabled" check, where, if shared
> > is not enabled, you assign shared_dep to be an empty string, or an empty
> > object or something.
> > 
> > That would mean that you wouldn't need to move the set_variable line as
> > here, you also wouldn't need to put conditionals around all the
> > 
> > Just a thought - could you avoid some of the changes throughout this set,
> > if you add an else-leg to the "is_shared_enabled" check, where, if shared
> > is not enabled, you assign shared_dep to be an empty string, or an empty
> > object or something.
> > 
> > That would mean that you wouldn't need to move the set_variable line as
> > here, you also wouldn't need to put conditionals around all the
> > 'shared_deps +=' lines when processing deps for a component, and lastly,
> > you may not even need to change all the searches for shared_rte_ to
> > static_rte, since the variables would exist for shared - they'd just be
> > empty!
> 
> did this in v3, still requires a diff -b to read what actually changed
> but appears to have worked out as you anticipated. so i think probably
> it was a good improvement.
> 

Great, thanks for trying it!

> > 
> > > +        endif
> > >          static_dep = declare_dependency(
> > >                  include_directories: includes,
> > >                  dependencies: static_deps)
> > >  
> > >          dpdk_drivers += static_lib
> > 
> > <snip for brevity>