diff mbox series

[1/2] drivers: suggestion on meson without version file

Message ID 20221006071923.755507-1-omer.yamac@ceng.metu.edu.tr (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series [1/2] drivers: suggestion on meson without version file | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Abdullah Ömer Yamaç Oct. 6, 2022, 7:19 a.m. UTC
Most of the drivers don't have a special version.map file. They just
included due to the compilation issue and needs to be updated for each
release.

These version.map files include:
DPDK_23 {
  local: *;
};

In this patch, we removed the necessity of the version files and
you don't need to update these files for each release, you can just
remove them.

Signed-off-by: Abdullah Ömer Yamaç <omer.yamac@ceng.metu.edu.tr>
Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>

---
Depends on: patch-116222 ("build: increase minimum meson version to 0.53")
---
 drivers/meson.build | 63 ++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

Comments

Bruce Richardson Oct. 7, 2022, 10:30 a.m. UTC | #1
The title of this patch needs an update - I would suggest something
like:

"build: make version file optional for drivers"

More comments inline below.

On Thu, Oct 06, 2022 at 10:19:22AM +0300, Abdullah Ömer Yamaç wrote:
> Most of the drivers don't have a special version.map file. They just
> included due to the compilation issue and needs to be updated for each
> release.
> 
> These version.map files include:
> DPDK_23 {
>   local: *;
> };
> 
> In this patch, we removed the necessity of the version files and
> you don't need to update these files for each release, you can just
> remove them.
> 
> Signed-off-by: Abdullah Ömer Yamaç <omer.yamac@ceng.metu.edu.tr>
> Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> ---
> Depends on: patch-116222 ("build: increase minimum meson version to 0.53")
> ---

Thanks for splitting the patch. It is a lot easier to review now,
especially if we apply and use "diff -w".

For any other reviewers, the "diff -w" for this patch is:

--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017-2019 Intel Corporation
 
+fs = import('fs')
+
 # Defines the order of dependencies evaluation
 subdirs = [
         'common',
@@ -193,6 +195,7 @@ foreach subpath:subdirs
         version_map = '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
         implib = 'lib' + lib_name + '.dll.a'
 
+        if fs.is_file(version_map)
             def_file = custom_target(lib_name + '_def',
                     command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
                     input: version_map,
@@ -227,6 +230,8 @@ foreach subpath:subdirs
                 endif
             endif
 
+        endif
+
         shared_lib = shared_library(lib_name, sources,
                 objects: objs,
                 include_directories: includes,


>  drivers/meson.build | 63 ++++++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/meson.build b/drivers/meson.build
> index f6ba5ba4fb..6ef03e14c7 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2017-2019 Intel Corporation
>  
> +fs = import('fs')
> +
>  # Defines the order of dependencies evaluation
>  subdirs = [
>          'common',
> @@ -193,38 +195,41 @@ foreach subpath:subdirs
>          version_map = '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
>          implib = 'lib' + lib_name + '.dll.a'
>  
<snip>
> +                            capture: true,
> +                            input: static_lib,
> +                            output: lib_name + '.sym_chk')
> +                endif
>              endif
> +
>          endif
>  
>          shared_lib = shared_library(lib_name, sources,

Beware that the shared_lib calls use both lk_deps and lk_args parameters,
which are only set inside the "if" block you added.
This will cause problems in that:
1. If the first driver doesn't have a version.map file, these variables
   will be undefined and you'll get a build error.
2. For any subsequent drivers that don't have a version.map file, the old
   values of the variables from the previous driver will be used.

Therefore, at the start of processing each driver, you need to assign empty
values to these variable.

Regards,
/Bruce
Abdullah Ömer Yamaç Oct. 10, 2022, 7:41 a.m. UTC | #2
On 07.10.2022 13:30, Bruce Richardson wrote:
> The title of this patch needs an update - I would suggest something
> like:
> 
> "build: make version file optional for drivers"
Thank you, it will make more sense
> 
> More comments inline below.
> 
> On Thu, Oct 06, 2022 at 10:19:22AM +0300, Abdullah Ömer Yamaç wrote:
>> Most of the drivers don't have a special version.map file. They just
>> included due to the compilation issue and needs to be updated for each
>> release.
>> 
>> These version.map files include:
>> DPDK_23 {
>>   local: *;
>> };
>> 
>> In this patch, we removed the necessity of the version files and
>> you don't need to update these files for each release, you can just
>> remove them.
>> 
>> Signed-off-by: Abdullah Ömer Yamaç <omer.yamac@ceng.metu.edu.tr>
>> Suggested-by: Ferruh Yigit <ferruh.yigit@amd.com>
>> 
>> ---
>> Depends on: patch-116222 ("build: increase minimum meson version to 
>> 0.53")
>> ---
> 
> Thanks for splitting the patch. It is a lot easier to review now,
> especially if we apply and use "diff -w".
> 
> For any other reviewers, the "diff -w" for this patch is:
> 
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2017-2019 Intel Corporation
> 
> +fs = import('fs')
> +
>  # Defines the order of dependencies evaluation
>  subdirs = [
>          'common',
> @@ -193,6 +195,7 @@ foreach subpath:subdirs
>          version_map = 
> '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
>          implib = 'lib' + lib_name + '.dll.a'
> 
> +        if fs.is_file(version_map)
>              def_file = custom_target(lib_name + '_def',
>                      command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
>                      input: version_map,
> @@ -227,6 +230,8 @@ foreach subpath:subdirs
>                  endif
>              endif
> 
> +        endif
> +
>          shared_lib = shared_library(lib_name, sources,
>                  objects: objs,
>                  include_directories: includes,
> 
> 
>>  drivers/meson.build | 63 
>> ++++++++++++++++++++++++---------------------
>>  1 file changed, 34 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/meson.build b/drivers/meson.build
>> index f6ba5ba4fb..6ef03e14c7 100644
>> --- a/drivers/meson.build
>> +++ b/drivers/meson.build
>> @@ -1,6 +1,8 @@
>>  # SPDX-License-Identifier: BSD-3-Clause
>>  # Copyright(c) 2017-2019 Intel Corporation
>> 
>> +fs = import('fs')
>> +
>>  # Defines the order of dependencies evaluation
>>  subdirs = [
>>          'common',
>> @@ -193,38 +195,41 @@ foreach subpath:subdirs
>>          version_map = 
>> '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
>>          implib = 'lib' + lib_name + '.dll.a'
>> 
> <snip>
>> +                            capture: true,
>> +                            input: static_lib,
>> +                            output: lib_name + '.sym_chk')
>> +                endif
>>              endif
>> +
>>          endif
>> 
>>          shared_lib = shared_library(lib_name, sources,
> 
> Beware that the shared_lib calls use both lk_deps and lk_args 
> parameters,
> which are only set inside the "if" block you added.
> This will cause problems in that:
> 1. If the first driver doesn't have a version.map file, these variables
>    will be undefined and you'll get a build error.
> 2. For any subsequent drivers that don't have a version.map file, the 
> old
>    values of the variables from the previous driver will be used.
> 
You're right, I missed that part and I will add a "else condition" to 
set variables as empty. Then everything is OK?
> Therefore, at the start of processing each driver, you need to assign 
> empty
> values to these variable.
> 
> Regards,
> /Bruce
Thank you
Bruce Richardson Oct. 10, 2022, 8:34 a.m. UTC | #3
On Mon, Oct 10, 2022 at 10:41:57AM +0300, Omer Yamac wrote:
> 
<snip>
> > Beware that the shared_lib calls use both lk_deps and lk_args
> > parameters,
> > which are only set inside the "if" block you added.
> > This will cause problems in that:
> > 1. If the first driver doesn't have a version.map file, these variables
> >    will be undefined and you'll get a build error.
> > 2. For any subsequent drivers that don't have a version.map file, the
> > old
> >    values of the variables from the previous driver will be used.
> > 
> You're right, I missed that part and I will add a "else condition" to set
> variables as empty. Then everything is OK?

Don't add an "else" leg as it will complicate things. Instead, initialize
them to empty at the start of the per-driver block, where we initialize
the other variables. It doesn't matter having them assigned twice in the
one block.

Thanks,
/Bruce
diff mbox series

Patch

diff --git a/drivers/meson.build b/drivers/meson.build
index f6ba5ba4fb..6ef03e14c7 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -1,6 +1,8 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017-2019 Intel Corporation
 
+fs = import('fs')
+
 # Defines the order of dependencies evaluation
 subdirs = [
         'common',
@@ -193,38 +195,41 @@  foreach subpath:subdirs
         version_map = '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
         implib = 'lib' + lib_name + '.dll.a'
 
-        def_file = custom_target(lib_name + '_def',
-                command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
-                input: version_map,
-                output: '@0@_exports.def'.format(lib_name))
-
-        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 = [version_map, def_file, mingw_map]
-        if is_windows
-            if is_ms_linker
-                lk_args = ['-Wl,/def:' + def_file.full_path()]
-                if meson.version().version_compare('<0.54.0')
-                    lk_args += ['-Wl,/implib:drivers\\' + implib]
+        if fs.is_file(version_map)
+            def_file = custom_target(lib_name + '_def',
+                    command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
+                    input: version_map,
+                    output: '@0@_exports.def'.format(lib_name))
+
+            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 = [version_map, def_file, mingw_map]
+            if is_windows
+                if is_ms_linker
+                    lk_args = ['-Wl,/def:' + def_file.full_path()]
+                    if meson.version().version_compare('<0.54.0')
+                        lk_args += ['-Wl,/implib:drivers\\' + implib]
+                    endif
+                else
+                    lk_args = ['-Wl,--version-script=' + mingw_map.full_path()]
                 endif
             else
-                lk_args = ['-Wl,--version-script=' + mingw_map.full_path()]
-            endif
-        else
-            lk_args = ['-Wl,--version-script=' + version_map]
-            if developer_mode
-                # on unix systems check the output of the
-                # check-symbols.sh script, using it as a
-                # dependency of the .so build
-                lk_deps += custom_target(lib_name + '.sym_chk',
-                        command: [check_symbols, version_map, '@INPUT@'],
-                        capture: true,
-                        input: static_lib,
-                        output: lib_name + '.sym_chk')
+                lk_args = ['-Wl,--version-script=' + version_map]
+                if developer_mode
+                    # on unix systems check the output of the
+                    # check-symbols.sh script, using it as a
+                    # dependency of the .so build
+                    lk_deps += custom_target(lib_name + '.sym_chk',
+                            command: [check_symbols, version_map, '@INPUT@'],
+                            capture: true,
+                            input: static_lib,
+                            output: lib_name + '.sym_chk')
+                endif
             endif
+
         endif
 
         shared_lib = shared_library(lib_name, sources,