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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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.

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

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

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