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

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

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?

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

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

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

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

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

<snip for brevity>