[1/2] drivers: suggestion on meson without version file
Checks
Commit Message
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
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
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
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
@@ -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,