* Re: [PATCH] build/eal: add OS defines for C conditional checks
2021-12-10 14:53 [PATCH] build/eal: add OS defines for C conditional checks Bruce Richardson
@ 2021-12-13 7:21 ` Jerin Jacob
2021-12-13 22:58 ` Dmitry Kozlyuk
2021-12-16 15:21 ` [PATCH v2] " Bruce Richardson
2 siblings, 0 replies; 6+ messages in thread
From: Jerin Jacob @ 2021-12-13 7:21 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dpdk-dev, Dmitry Kozlyuk, Aaron Conole
On Fri, Dec 10, 2021 at 8:23 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> Define a set of macros in the build configuration to allow C runtime
> code to check the current OS environment. This saves the user having to
> use ifdefs for e.g. disabling particular tests on Windows. See included
> documentation changes for usage examples.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
> ---
> doc/guides/contributing/coding_style.rst | 42 ++++++++++++++++++++++--
> lib/eal/meson.build | 7 ++++
> 2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> index 0ec37c019b..e52ecb2b60 100644
> --- a/doc/guides/contributing/coding_style.rst
> +++ b/doc/guides/contributing/coding_style.rst
> @@ -136,6 +136,12 @@ For example:
> Conditional Compilation
> ~~~~~~~~~~~~~~~~~~~~~~~
>
> +.. note::
> +
> + Conditional compilation should be used only when absolutely necessary, as it increases the number of target binaries that need to be built and tested.
> + See below for details of some utility macros/defines available to allow ifdefs/macros to be replaced by C conditional in some cases.
> +
> +
> * When code is conditionally compiled using ``#ifdef`` or ``#if``, a comment may be added following the matching
> ``#endif`` or ``#else`` to permit the reader to easily discern where conditionally compiled code regions end.
> * This comment should be used only for (subjectively) long regions, regions greater than 20 lines, or where a series of nested ``#ifdef``'s may be confusing to the reader.
> @@ -165,9 +171,41 @@ Conditional Compilation
> /* Or here. */
> #endif /* !COMPAT_43 */
>
> -.. note::
> +Defines to Avoid Conditional Compilation
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +In many cases in DPDK, one wants to optionally compile code based on the target platform,
> +or runtime environment.
> +While this can be done using the conditional compilation directives,
> +e.g. ``#ifdef RTE_EXEC_ENV_LINUX``, present in DPDK for many releases,
> +this can also be done in many cases using regular ``if`` statements and the following defines:
> +
> +* ``RTE_ENV_FREEBSD``, ``RTE_ENV_LINUX``, ``RTE_ENV_WINDOWS`` - these define ids for each operating system environment.
> +* ``RTE_EXEC_ENV`` - this defines the id of the current environment, i.e. one of the items in list above.
> +* ``RTE_EXEC_ENV_IS_FREEBSD``, ``RTE_EXEC_ENV_IS_LINUX``, ``RTE_EXEC_ENV_IS_WINDOWS`` - 0/1 values indicating if the current environment is that specified,
> + shortcuts for checking e.g. ``RTE_EXEC_ENV == RTE_ENV_WINDOWS``
> +
> +Examples of use:
> +
> +.. code-block:: c
> +
> + /* report a unit tests as unsupported on Windows */
> + if (RTE_EXEC_ENV_IS_WINDOWS)
> + return TEST_SKIPPED;
> +
> + /* set different default values depending on OS Environment */
> + switch (RTE_EXEC_ENV) {
> + case RTE_ENV_FREEBSD:
> + default = x;
> + break;
> + case RTE_ENV_LINUX:
> + default = y;
> + break;
> + case RTE_ENV_WINDOWS:
> + default = z;
> + break;
> + }
>
> - Conditional compilation should be used only when absolutely necessary, as it increases the number of target binaries that need to be built and tested.
>
> C Types
> -------
> diff --git a/lib/eal/meson.build b/lib/eal/meson.build
> index 1722924f67..056beb9461 100644
> --- a/lib/eal/meson.build
> +++ b/lib/eal/meson.build
> @@ -10,6 +10,13 @@ if not is_windows
> subdir('unix')
> endif
>
> +exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
> +foreach env, id:exec_envs
> + dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
> + dpdk_conf.set10('RTE_EXEC_ENV_IS_' + env.to_upper(), (exec_env == env))
> +endforeach
> +dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
> +
> dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
> subdir(exec_env)
>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] build/eal: add OS defines for C conditional checks
2021-12-10 14:53 [PATCH] build/eal: add OS defines for C conditional checks Bruce Richardson
2021-12-13 7:21 ` Jerin Jacob
@ 2021-12-13 22:58 ` Dmitry Kozlyuk
2021-12-14 12:03 ` Bruce Richardson
2021-12-16 15:21 ` [PATCH v2] " Bruce Richardson
2 siblings, 1 reply; 6+ messages in thread
From: Dmitry Kozlyuk @ 2021-12-13 22:58 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev, aconole
2021-12-10 14:53 (UTC+0000), Bruce Richardson:
[...]
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
with one typo below and some considerations for the future in the bottom.
> +Defines to Avoid Conditional Compilation
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +In many cases in DPDK, one wants to optionally compile code based on the target platform,
> +or runtime environment.
"Compile" -> "run", that's the point to use conditionals instead of macros.
> +While this can be done using the conditional compilation directives,
> +e.g. ``#ifdef RTE_EXEC_ENV_LINUX``, present in DPDK for many releases,
> +this can also be done in many cases using regular ``if`` statements and the following defines:
> +
> +* ``RTE_ENV_FREEBSD``, ``RTE_ENV_LINUX``, ``RTE_ENV_WINDOWS`` - these define ids for each operating system environment.
> +* ``RTE_EXEC_ENV`` - this defines the id of the current environment, i.e. one of the items in list above.
> +* ``RTE_EXEC_ENV_IS_FREEBSD``, ``RTE_EXEC_ENV_IS_LINUX``, ``RTE_EXEC_ENV_IS_WINDOWS`` - 0/1 values indicating if the current environment is that specified,
> + shortcuts for checking e.g. ``RTE_EXEC_ENV == RTE_ENV_WINDOWS``
[...]
I wonder whether #if RTE_EXEC_ENV_IS_xxx
should be preferred over #ifdef RTE_EXEC_ENV_xxx,
so that all checks use the same symbol
(and then remove old macros one day).
Since C conditionals are preferred over #ifdef,
I suggest to give pointers when to use one or another mechanism:
If a code fragment can compile on all platforms,
but cannot run on some due to lack of support,
branch on constants.
If a code fragment cannot compile on all platforms
(e.g. use of OS-specific headers or macros),
but constitutes only a small fraction of the file,
use conditional compilation.
If a group of functions implement an interface
in an OS- or platform-specific way,
create a file for each of the supported environments
and plug an appropriate file from ``meson.build``.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] build/eal: add OS defines for C conditional checks
2021-12-13 22:58 ` Dmitry Kozlyuk
@ 2021-12-14 12:03 ` Bruce Richardson
0 siblings, 0 replies; 6+ messages in thread
From: Bruce Richardson @ 2021-12-14 12:03 UTC (permalink / raw)
To: Dmitry Kozlyuk; +Cc: dev, aconole
On Tue, Dec 14, 2021 at 01:58:03AM +0300, Dmitry Kozlyuk wrote:
> 2021-12-10 14:53 (UTC+0000), Bruce Richardson:
> [...]
>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> with one typo below and some considerations for the future in the bottom.
>
> > +Defines to Avoid Conditional Compilation
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +In many cases in DPDK, one wants to optionally compile code based on the target platform,
> > +or runtime environment.
>
> "Compile" -> "run", that's the point to use conditionals instead of macros.
>
> > +While this can be done using the conditional compilation directives,
> > +e.g. ``#ifdef RTE_EXEC_ENV_LINUX``, present in DPDK for many releases,
> > +this can also be done in many cases using regular ``if`` statements and the following defines:
>
> > +
> > +* ``RTE_ENV_FREEBSD``, ``RTE_ENV_LINUX``, ``RTE_ENV_WINDOWS`` - these define ids for each operating system environment.
> > +* ``RTE_EXEC_ENV`` - this defines the id of the current environment, i.e. one of the items in list above.
> > +* ``RTE_EXEC_ENV_IS_FREEBSD``, ``RTE_EXEC_ENV_IS_LINUX``, ``RTE_EXEC_ENV_IS_WINDOWS`` - 0/1 values indicating if the current environment is that specified,
> > + shortcuts for checking e.g. ``RTE_EXEC_ENV == RTE_ENV_WINDOWS``
> [...]
>
> I wonder whether #if RTE_EXEC_ENV_IS_xxx
> should be preferred over #ifdef RTE_EXEC_ENV_xxx,
> so that all checks use the same symbol
> (and then remove old macros one day).
>
> Since C conditionals are preferred over #ifdef,
> I suggest to give pointers when to use one or another mechanism:
>
> If a code fragment can compile on all platforms,
> but cannot run on some due to lack of support,
> branch on constants.
>
> If a code fragment cannot compile on all platforms
> (e.g. use of OS-specific headers or macros),
> but constitutes only a small fraction of the file,
> use conditional compilation.
>
> If a group of functions implement an interface
> in an OS- or platform-specific way,
> create a file for each of the supported environments
> and plug an appropriate file from ``meson.build``.
Good suggestions. I rework into a v2 when I get the chance.
/Bruce
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] build/eal: add OS defines for C conditional checks
2021-12-10 14:53 [PATCH] build/eal: add OS defines for C conditional checks Bruce Richardson
2021-12-13 7:21 ` Jerin Jacob
2021-12-13 22:58 ` Dmitry Kozlyuk
@ 2021-12-16 15:21 ` Bruce Richardson
2022-01-17 18:20 ` Thomas Monjalon
2 siblings, 1 reply; 6+ messages in thread
From: Bruce Richardson @ 2021-12-16 15:21 UTC (permalink / raw)
To: dev; +Cc: Bruce Richardson, Dmitry Kozlyuk, Jerin Jacob
Define a set of macros in the build configuration to allow C runtime
code to check the current OS environment. This saves the user having to
use ifdefs for e.g. disabling particular tests on Windows. See included
documentation changes for usage examples.
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
---
V2: Included feedback from Dmitry to expand on documentation to include
guidelines on when to use conditional compilation.
---
doc/guides/contributing/coding_style.rst | 53 +++++++++++++++++++++++-
lib/eal/meson.build | 7 ++++
2 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index 0ec37c019b..66803dcccd 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -136,6 +136,24 @@ For example:
Conditional Compilation
~~~~~~~~~~~~~~~~~~~~~~~
+.. note::
+
+ Conditional compilation should be used only when absolutely necessary, as it increases the number of target binaries that need to be built and tested.
+ See below for details of some utility macros/defines available to allow ifdefs/macros to be replaced by C conditional in some cases.
+
+Some high-level guidelines on the use of conditional compilation:
+
+* If code can compile on all platforms/systems, but cannot run on some due to lack of support,
+ then regular C conditionals, as described in the next section, should be used instead of conditional compilation.
+* If the code in question cannot compile on all systems, but constitutes only a small fragment of
+ a file, then conditional complication should be used, as described in this section.
+* If the code for conditional compilation implements an interface in an OS or platform-specific way,
+ then create a file for each OS or platform and select the appropriate file using the meson build system.
+ In most cases, these environment-specific files should be created inside the EAL library,
+ rather than having each library implement its own abstraction layer.
+
+Additional style guidance for the use of conditional compilation macros:
+
* When code is conditionally compiled using ``#ifdef`` or ``#if``, a comment may be added following the matching
``#endif`` or ``#else`` to permit the reader to easily discern where conditionally compiled code regions end.
* This comment should be used only for (subjectively) long regions, regions greater than 20 lines, or where a series of nested ``#ifdef``'s may be confusing to the reader.
@@ -165,9 +183,40 @@ Conditional Compilation
/* Or here. */
#endif /* !COMPAT_43 */
-.. note::
+Defines to Avoid Conditional Compilation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In many cases in DPDK, one wants to run code based on the target platform, or runtime environment.
+While this can be done using the conditional compilation directives,
+e.g. ``#ifdef RTE_EXEC_ENV_LINUX``, present in DPDK for many releases,
+this can also be done in many cases using regular ``if`` statements and the following defines:
+
+* ``RTE_ENV_FREEBSD``, ``RTE_ENV_LINUX``, ``RTE_ENV_WINDOWS`` - these define ids for each operating system environment.
+* ``RTE_EXEC_ENV`` - this defines the id of the current environment, i.e. one of the items in list above.
+* ``RTE_EXEC_ENV_IS_FREEBSD``, ``RTE_EXEC_ENV_IS_LINUX``, ``RTE_EXEC_ENV_IS_WINDOWS`` - 0/1 values indicating if the current environment is that specified,
+ shortcuts for checking e.g. ``RTE_EXEC_ENV == RTE_ENV_WINDOWS``
+
+Examples of use:
+
+.. code-block:: c
+
+ /* report a unit tests as unsupported on Windows */
+ if (RTE_EXEC_ENV_IS_WINDOWS)
+ return TEST_SKIPPED;
+
+ /* set different default values depending on OS Environment */
+ switch (RTE_EXEC_ENV) {
+ case RTE_ENV_FREEBSD:
+ default = x;
+ break;
+ case RTE_ENV_LINUX:
+ default = y;
+ break;
+ case RTE_ENV_WINDOWS:
+ default = z;
+ break;
+ }
- Conditional compilation should be used only when absolutely necessary, as it increases the number of target binaries that need to be built and tested.
C Types
-------
diff --git a/lib/eal/meson.build b/lib/eal/meson.build
index 1722924f67..056beb9461 100644
--- a/lib/eal/meson.build
+++ b/lib/eal/meson.build
@@ -10,6 +10,13 @@ if not is_windows
subdir('unix')
endif
+exec_envs = {'freebsd': 0, 'linux': 1, 'windows': 2}
+foreach env, id:exec_envs
+ dpdk_conf.set('RTE_ENV_' + env.to_upper(), id)
+ dpdk_conf.set10('RTE_EXEC_ENV_IS_' + env.to_upper(), (exec_env == env))
+endforeach
+dpdk_conf.set('RTE_EXEC_ENV', exec_envs[exec_env])
+
dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
subdir(exec_env)
--
2.32.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] build/eal: add OS defines for C conditional checks
2021-12-16 15:21 ` [PATCH v2] " Bruce Richardson
@ 2022-01-17 18:20 ` Thomas Monjalon
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2022-01-17 18:20 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev, Dmitry Kozlyuk, Jerin Jacob
16/12/2021 16:21, Bruce Richardson:
> Define a set of macros in the build configuration to allow C runtime
> code to check the current OS environment. This saves the user having to
> use ifdefs for e.g. disabling particular tests on Windows. See included
> documentation changes for usage examples.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Acked-by: Jerin Jacob <jerinj@marvell.com>
>
> ---
> V2: Included feedback from Dmitry to expand on documentation to include
> guidelines on when to use conditional compilation.
> ---
[...]
> +* If the code in question cannot compile on all systems, but constitutes only a small fragment of
> + a file, then conditional complication should be used, as described in this section.
complication -> compilation
[...]
> + switch (RTE_EXEC_ENV) {
> + case RTE_ENV_FREEBSD:
> + default = x;
> + break;
> + case RTE_ENV_LINUX:
> + default = y;
> + break;
> + case RTE_ENV_WINDOWS:
> + default = z;
> + break;
> + }
This is not the recommended indentation.
Applied with minor changes, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread