* [PATCH 0/5] Extend optional libraries list
@ 2021-11-10 16:48 David Marchand
2021-11-10 16:48 ` [PATCH 1/5] ci: test build with minimum configuration David Marchand
` (6 more replies)
0 siblings, 7 replies; 70+ messages in thread
From: David Marchand @ 2021-11-10 16:48 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
This series extends the optional libraries list adding to them testpmd
non essential dependencies.
We were not testing the disable_libs option, so let's add a minimum target
in the the public CI script (mainly for GHA) and in test-meson-builds.sh
script.
The last patch is an idea for enhancing the optional libraries selection.
--
David Marchand
David Marchand (5):
ci: test build with minimum configuration
build: make GRO/GSO optional
build: make metrics libraries optional
build: make pdump optional
build: select optional libraries
.ci/linux-build.sh | 3 +++
.github/workflows/build.yml | 5 +++++
app/meson.build | 18 ++++++++++-----
app/proc-info/main.c | 16 +++++++++++++
app/proc-info/meson.build | 5 ++++-
app/test-pmd/cmdline.c | 14 ++++++++++++
app/test-pmd/config.c | 6 +++++
app/test-pmd/csumonly.c | 35 ++++++++++++++++++++++++-----
app/test-pmd/meson.build | 11 ++++++++-
app/test-pmd/testpmd.c | 18 +++++++++++++++
app/test-pmd/testpmd.h | 18 +++++++++++++++
app/test/meson.build | 34 ++++++++++++++++------------
buildtools/chkincs/meson.build | 2 +-
devtools/test-meson-builds.sh | 4 +++-
lib/meson.build | 41 +++++++++++++++++++++-------------
meson.build | 3 ++-
meson_options.txt | 2 ++
17 files changed, 189 insertions(+), 46 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/5] ci: test build with minimum configuration
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
@ 2021-11-10 16:48 ` David Marchand
2021-11-16 17:06 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 2/5] build: make GRO/GSO optional David Marchand
` (5 subsequent siblings)
6 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-10 16:48 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
Disabling optional libraries was not tested.
Add a new target in test-meson-builds.sh and GHA.
The Bluefield target is removed from test-meson-builds.sh to save space
and compilation time in exchange of the new target.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
.ci/linux-build.sh | 3 +++
.github/workflows/build.yml | 5 +++++
devtools/test-meson-builds.sh | 4 +++-
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index ef0bd099be..e7ed648099 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -87,6 +87,9 @@ OPTS="$OPTS -Dplatform=generic"
OPTS="$OPTS --default-library=$DEF_LIB"
OPTS="$OPTS --buildtype=debugoptimized"
OPTS="$OPTS -Dcheck_includes=true"
+if [ "$NO_OPTIONAL_LIBS" = "true" ]; then
+ OPTS="$OPTS -Ddisable_libs=*"
+fi
meson build --werror $OPTS
ninja -C build
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 4151cafee7..346cc75c20 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -21,6 +21,7 @@ jobs:
CC: ccache ${{ matrix.config.compiler }}
DEF_LIB: ${{ matrix.config.library }}
LIBABIGAIL_VERSION: libabigail-1.8
+ NO_OPTIONAL_LIBS: ${{ matrix.config.no_optional_libs != '' }}
PPC64LE: ${{ matrix.config.cross == 'ppc64le' }}
REF_GIT_TAG: none
RUN_TESTS: ${{ contains(matrix.config.checks, 'tests') }}
@@ -32,6 +33,10 @@ jobs:
- os: ubuntu-18.04
compiler: gcc
library: static
+ - os: ubuntu-18.04
+ compiler: gcc
+ library: shared
+ no_optional_libs: no-optional-libs
- os: ubuntu-18.04
compiler: gcc
library: shared
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 9ec8e2bc7e..36ecf63ec6 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -220,6 +220,8 @@ for c in gcc clang ; do
done
done
+build build-x86-no-optional-libs cc skipABI $use_shared -Ddisable_libs=*
+
# test compilation with minimal x86 instruction set
# Set the install path for libraries to "lib" explicitly to prevent problems
# with pkg-config prefixes if installed in "lib/x86_64-linux-gnu" later.
@@ -258,7 +260,7 @@ export CC="clang"
build build-arm64-host-clang $f ABI $use_shared
unset CC
# some gcc/arm configurations
-for f in $srcdir/config/arm/arm64_[bdo]*gcc ; do
+for f in $srcdir/config/arm/arm64_[do]*gcc ; do
export CC="$CCACHE gcc"
targetdir=build-$(basename $f | tr '_' '-' | cut -d'-' -f-2)
build $targetdir $f skipABI $use_shared
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 2/5] build: make GRO/GSO optional
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
2021-11-10 16:48 ` [PATCH 1/5] ci: test build with minimum configuration David Marchand
@ 2021-11-10 16:48 ` David Marchand
2021-11-16 17:11 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 3/5] build: make metrics libraries optional David Marchand
` (4 subsequent siblings)
6 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-10 16:48 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Xiaoyun Li
GRO and GSO integration in testpmd is relatively self contained and easy
to extract.
Those libraries can be made optional as they provide standalone
features.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test-pmd/cmdline.c | 14 ++++++++++++++
app/test-pmd/config.c | 6 ++++++
app/test-pmd/csumonly.c | 35 +++++++++++++++++++++++++++++------
app/test-pmd/meson.build | 8 +++++++-
app/test-pmd/testpmd.c | 14 ++++++++++++++
app/test-pmd/testpmd.h | 18 ++++++++++++++++++
lib/meson.build | 2 ++
7 files changed, 90 insertions(+), 7 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 4f51b259fe..1a3c0a56df 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -35,7 +35,9 @@
#include <rte_string_fns.h>
#include <rte_devargs.h>
#include <rte_flow.h>
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
#include <rte_mbuf_dyn.h>
#include <cmdline_rdline.h>
@@ -459,6 +461,7 @@ static void cmd_help_long_parsed(void *parsed_result,
"tso show (portid)"
" Display the status of TCP Segmentation Offload.\n\n"
+#ifdef RTE_LIB_GRO
"set port (port_id) gro on|off\n"
" Enable or disable Generic Receive Offload in"
" csum forwarding engine.\n\n"
@@ -469,7 +472,9 @@ static void cmd_help_long_parsed(void *parsed_result,
"set gro flush (cycles)\n"
" Set the cycle to flush GROed packets from"
" reassembly tables.\n\n"
+#endif
+#ifdef RTE_LIB_GSO
"set port (port_id) gso (on|off)"
" Enable or disable Generic Segmentation Offload in"
" csum forwarding engine.\n\n"
@@ -480,6 +485,7 @@ static void cmd_help_long_parsed(void *parsed_result,
"show port (port_id) gso\n"
" Show GSO configuration.\n\n"
+#endif
"set fwd (%s)\n"
" Set packet forwarding mode.\n\n"
@@ -5150,6 +5156,7 @@ cmdline_parse_inst_t cmd_tunnel_tso_show = {
},
};
+#ifdef RTE_LIB_GRO
/* *** SET GRO FOR A PORT *** */
struct cmd_gro_enable_result {
cmdline_fixed_string_t cmd_set;
@@ -5293,7 +5300,9 @@ cmdline_parse_inst_t cmd_gro_flush = {
NULL,
},
};
+#endif /* RTE_LIB_GRO */
+#ifdef RTE_LIB_GSO
/* *** ENABLE/DISABLE GSO *** */
struct cmd_gso_enable_result {
cmdline_fixed_string_t cmd_set;
@@ -5460,6 +5469,7 @@ cmdline_parse_inst_t cmd_gso_show = {
NULL,
},
};
+#endif /* RTE_LIB_GSO */
/* *** ENABLE/DISABLE FLUSH ON RX STREAMS *** */
struct cmd_set_flush_rx {
@@ -17661,12 +17671,16 @@ cmdline_parse_ctx_t main_ctx[] = {
(cmdline_parse_inst_t *)&cmd_tso_show,
(cmdline_parse_inst_t *)&cmd_tunnel_tso_set,
(cmdline_parse_inst_t *)&cmd_tunnel_tso_show,
+#ifdef RTE_LIB_GRO
(cmdline_parse_inst_t *)&cmd_gro_enable,
(cmdline_parse_inst_t *)&cmd_gro_flush,
(cmdline_parse_inst_t *)&cmd_gro_show,
+#endif
+#ifdef RTE_LIB_GSO
(cmdline_parse_inst_t *)&cmd_gso_enable,
(cmdline_parse_inst_t *)&cmd_gso_size,
(cmdline_parse_inst_t *)&cmd_gso_show,
+#endif
(cmdline_parse_inst_t *)&cmd_link_flow_control_set,
(cmdline_parse_inst_t *)&cmd_link_flow_control_set_rx,
(cmdline_parse_inst_t *)&cmd_link_flow_control_set_tx,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 26cadf39f7..9ece9cd8ba 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -49,7 +49,9 @@
#ifdef RTE_NET_BNXT
#include <rte_pmd_bnxt.h>
#endif
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
#include <rte_hexdump.h>
#include "testpmd.h"
@@ -4192,6 +4194,7 @@ set_tx_pkt_times(unsigned int *tx_times)
tx_pkt_times_intra = tx_times[1];
}
+#ifdef RTE_LIB_GRO
void
setup_gro(const char *onoff, portid_t port_id)
{
@@ -4273,7 +4276,9 @@ show_gro(portid_t port_id)
} else
printf("Port %u doesn't enable GRO.\n", port_id);
}
+#endif /* RTE_LIB_GRO */
+#ifdef RTE_LIB_GSO
void
setup_gso(const char *mode, portid_t port_id)
{
@@ -4297,6 +4302,7 @@ setup_gso(const char *mode, portid_t port_id)
gso_ports[port_id].enable = 0;
}
}
+#endif /* RTE_LIB_GSO */
char*
list_pkt_forwarding_modes(void)
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 8526d9158a..4b6c6759a4 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -41,8 +41,12 @@
#include <rte_prefetch.h>
#include <rte_string_fns.h>
#include <rte_flow.h>
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
+#ifdef RTE_LIB_GSO
#include <rte_gso.h>
+#endif
#include <rte_geneve.h>
#include "testpmd.h"
@@ -69,7 +73,9 @@ uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
/* structure that caches offload info for the current packet */
struct testpmd_offload_info {
uint16_t ethertype;
+#ifdef RTE_LIB_GSO
uint8_t gso_enable;
+#endif
uint16_t l2_len;
uint16_t l3_len;
uint16_t l4_len;
@@ -511,8 +517,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
info->ethertype);
}
}
+#ifdef RTE_LIB_GSO
if (info->gso_enable)
ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
+#endif
} else if (info->l4_proto == IPPROTO_TCP) {
tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
if (tso_segsz)
@@ -525,8 +533,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
get_udptcp_checksum(l3_hdr, tcp_hdr,
info->ethertype);
}
+#ifdef RTE_LIB_GSO
if (info->gso_enable)
ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
+#endif
} else if (info->l4_proto == IPPROTO_SCTP) {
sctp_hdr = (struct rte_sctp_hdr *)
((char *)l3_hdr + info->l3_len);
@@ -795,16 +805,20 @@ static void
pkt_burst_checksum_forward(struct fwd_stream *fs)
{
struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
+#ifdef RTE_LIB_GSO
struct rte_mbuf *gso_segments[GSO_MAX_PKT_BURST];
struct rte_gso_ctx *gso_ctx;
+#endif
struct rte_mbuf **tx_pkts_burst;
struct rte_port *txp;
struct rte_mbuf *m, *p;
struct rte_ether_hdr *eth_hdr;
void *l3_hdr = NULL, *outer_l3_hdr = NULL; /* can be IPv4 or IPv6 */
+#ifdef RTE_LIB_GRO
void **gro_ctx;
uint16_t gro_pkts_num;
uint8_t gro_enable;
+#endif
uint16_t nb_rx;
uint16_t nb_tx;
uint16_t nb_prep;
@@ -817,8 +831,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
uint32_t rx_bad_outer_l4_csum;
uint32_t rx_bad_outer_ip_csum;
struct testpmd_offload_info info;
- uint16_t nb_segments = 0;
- int ret;
uint64_t start_tsc = 0;
@@ -836,15 +848,19 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
rx_bad_l4_csum = 0;
rx_bad_outer_l4_csum = 0;
rx_bad_outer_ip_csum = 0;
+#ifdef RTE_LIB_GRO
gro_enable = gro_ports[fs->rx_port].enable;
+#endif
txp = &ports[fs->tx_port];
tx_offloads = txp->dev_conf.txmode.offloads;
memset(&info, 0, sizeof(info));
info.tso_segsz = txp->tso_segsz;
info.tunnel_tso_segsz = txp->tunnel_tso_segsz;
+#ifdef RTE_LIB_GSO
if (gso_ports[fs->tx_port].enable)
info.gso_enable = 1;
+#endif
for (i = 0; i < nb_rx; i++) {
if (likely(i < nb_rx - 1))
@@ -1053,6 +1069,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
}
}
+#ifdef RTE_LIB_GRO
if (unlikely(gro_enable)) {
if (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) {
nb_rx = rte_gro_reassemble_burst(pkts_burst, nb_rx,
@@ -1074,13 +1091,17 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
}
}
}
+#endif
+
+#ifdef RTE_LIB_GSO
+ if (gso_ports[fs->tx_port].enable != 0) {
+ uint16_t nb_segments = 0;
- if (gso_ports[fs->tx_port].enable == 0)
- tx_pkts_burst = pkts_burst;
- else {
gso_ctx = &(current_fwd_lcore()->gso_ctx);
gso_ctx->gso_size = gso_max_segment_size;
for (i = 0; i < nb_rx; i++) {
+ int ret;
+
ret = rte_gso_segment(pkts_burst[i], gso_ctx,
&gso_segments[nb_segments],
GSO_MAX_PKT_BURST - nb_segments);
@@ -1102,7 +1123,9 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
tx_pkts_burst = gso_segments;
nb_rx = nb_segments;
- }
+ } else
+#endif
+ tx_pkts_burst = pkts_burst;
nb_prep = rte_eth_tx_prepare(fs->tx_port, fs->tx_queue,
tx_pkts_burst, nb_rx);
diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
index d5df52c470..eba03b572c 100644
--- a/app/test-pmd/meson.build
+++ b/app/test-pmd/meson.build
@@ -32,7 +32,7 @@ if dpdk_conf.has('RTE_HAS_JANSSON')
ext_deps += jansson_dep
endif
-deps += ['ethdev', 'gro', 'gso', 'cmdline', 'metrics', 'bus_pci']
+deps += ['ethdev', 'cmdline', 'metrics', 'bus_pci']
if dpdk_conf.has('RTE_CRYPTO_SCHEDULER')
deps += 'crypto_scheduler'
endif
@@ -43,6 +43,12 @@ if dpdk_conf.has('RTE_LIB_BPF')
sources += files('bpf_cmd.c')
deps += 'bpf'
endif
+if dpdk_conf.has('RTE_LIB_GRO')
+ deps += 'gro'
+endif
+if dpdk_conf.has('RTE_LIB_GSO')
+ deps += 'gso'
+endif
if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
deps += 'latencystats'
endif
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index a66dfb297c..e2998b066b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -518,8 +518,10 @@ lcoreid_t bitrate_lcore_id;
uint8_t bitrate_enabled;
#endif
+#ifdef RTE_LIB_GRO
struct gro_status gro_ports[RTE_MAX_ETHPORTS];
uint8_t gro_flush_cycles = GRO_DEFAULT_FLUSH_CYCLES;
+#endif
/*
* hexadecimal bitmask of RX mq mode can be enabled.
@@ -658,8 +660,10 @@ static void fill_xstats_display_info(void);
*/
static int all_ports_started(void);
+#ifdef RTE_LIB_GSO
struct gso_status gso_ports[RTE_MAX_ETHPORTS];
uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN;
+#endif
/* Holds the registered mbuf dynamic flags names. */
char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
@@ -1633,8 +1637,12 @@ init_config(void)
struct rte_mempool *mbp;
unsigned int nb_mbuf_per_pool;
lcoreid_t lc_id;
+#ifdef RTE_LIB_GRO
struct rte_gro_param gro_param;
+#endif
+#ifdef RTE_LIB_GSO
uint32_t gso_types;
+#endif
/* Configuration of logical cores. */
fwd_lcores = rte_zmalloc("testpmd: fwd_lcores",
@@ -1717,8 +1725,10 @@ init_config(void)
init_port_config();
+#ifdef RTE_LIB_GSO
gso_types = RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO;
+#endif
/*
* Records which Mbuf pool to use by each logical core, if needed.
*/
@@ -1729,6 +1739,7 @@ init_config(void)
if (mbp == NULL)
mbp = mbuf_pool_find(0, 0);
fwd_lcores[lc_id]->mbp = mbp;
+#ifdef RTE_LIB_GSO
/* initialize GSO context */
fwd_lcores[lc_id]->gso_ctx.direct_pool = mbp;
fwd_lcores[lc_id]->gso_ctx.indirect_pool = mbp;
@@ -1736,10 +1747,12 @@ init_config(void)
fwd_lcores[lc_id]->gso_ctx.gso_size = RTE_ETHER_MAX_LEN -
RTE_ETHER_CRC_LEN;
fwd_lcores[lc_id]->gso_ctx.flag = 0;
+#endif
}
fwd_config_setup();
+#ifdef RTE_LIB_GRO
/* create a gro context for each lcore */
gro_param.gro_types = RTE_GRO_TCP_IPV4;
gro_param.max_flow_num = GRO_MAX_FLUSH_CYCLES;
@@ -1753,6 +1766,7 @@ init_config(void)
"rte_gro_ctx_create() failed\n");
}
}
+#endif
}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 669ce1e87d..b1dfd097c7 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -9,8 +9,12 @@
#include <rte_pci.h>
#include <rte_bus_pci.h>
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
+#ifdef RTE_LIB_GSO
#include <rte_gso.h>
+#endif
#include <rte_os_shim.h>
#include <cmdline.h>
#include <sys/queue.h>
@@ -143,7 +147,9 @@ struct fwd_stream {
/**< received packets has bad outer l4 checksum */
uint64_t rx_bad_outer_ip_csum;
/**< received packets having bad outer ip checksum */
+#ifdef RTE_LIB_GRO
unsigned int gro_times; /**< GRO operation times */
+#endif
uint64_t core_cycles; /**< used for RX and TX processing */
struct pkt_burst_stats rx_burst_stats;
struct pkt_burst_stats tx_burst_stats;
@@ -264,9 +270,13 @@ struct rte_port {
* CPU id. configuration table.
*/
struct fwd_lcore {
+#ifdef RTE_LIB_GSO
struct rte_gso_ctx gso_ctx; /**< GSO context */
+#endif
struct rte_mempool *mbp; /**< The mbuf pool to use by this core */
+#ifdef RTE_LIB_GRO
void *gro_ctx; /**< GRO context */
+#endif
streamid_t stream_idx; /**< index of 1st stream in "fwd_streams" */
streamid_t stream_nb; /**< number of streams in "fwd_streams" */
lcoreid_t cpuid_idx; /**< index of logical core in CPU id table */
@@ -560,6 +570,7 @@ extern struct rte_ether_addr peer_eth_addrs[RTE_MAX_ETHPORTS];
extern uint32_t burst_tx_delay_time; /**< Burst tx delay time(us) for mac-retry. */
extern uint32_t burst_tx_retry_num; /**< Burst tx retry number for mac-retry. */
+#ifdef RTE_LIB_GRO
#define GRO_DEFAULT_ITEM_NUM_PER_FLOW 32
#define GRO_DEFAULT_FLOW_NUM (RTE_GRO_MAX_BURST_ITEM_NUM / \
GRO_DEFAULT_ITEM_NUM_PER_FLOW)
@@ -573,13 +584,16 @@ struct gro_status {
};
extern struct gro_status gro_ports[RTE_MAX_ETHPORTS];
extern uint8_t gro_flush_cycles;
+#endif /* RTE_LIB_GRO */
+#ifdef RTE_LIB_GSO
#define GSO_MAX_PKT_BURST 2048
struct gso_status {
uint8_t enable;
};
extern struct gso_status gso_ports[RTE_MAX_ETHPORTS];
extern uint16_t gso_max_segment_size;
+#endif /* RTE_LIB_GSO */
/* VXLAN encap/decap parameters. */
struct vxlan_encap_conf {
@@ -1006,10 +1020,14 @@ void port_rss_hash_key_update(portid_t port_id, char rss_type[],
uint8_t *hash_key, uint8_t hash_key_len);
int rx_queue_id_is_invalid(queueid_t rxq_id);
int tx_queue_id_is_invalid(queueid_t txq_id);
+#ifdef RTE_LIB_GRO
void setup_gro(const char *onoff, portid_t port_id);
void setup_gro_flush_cycles(uint8_t cycles);
void show_gro(portid_t port_id);
+#endif
+#ifdef RTE_LIB_GSO
void setup_gso(const char *mode, portid_t port_id);
+#endif
int eth_dev_info_get_print_err(uint16_t port_id,
struct rte_eth_dev_info *dev_info);
int eth_dev_conf_get_print_err(uint16_t port_id,
diff --git a/lib/meson.build b/lib/meson.build
index 8537a5ab80..2766c02bd2 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -66,6 +66,8 @@ libraries = [
]
optional_libs = [
+ 'gro',
+ 'gso',
'kni',
'power',
'vhost',
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 3/5] build: make metrics libraries optional
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
2021-11-10 16:48 ` [PATCH 1/5] ci: test build with minimum configuration David Marchand
2021-11-10 16:48 ` [PATCH 2/5] build: make GRO/GSO optional David Marchand
@ 2021-11-10 16:48 ` David Marchand
2021-11-16 17:12 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 4/5] build: make pdump optional David Marchand
` (3 subsequent siblings)
6 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-10 16:48 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Maryam Tahhan, Reshma Pattan,
Xiaoyun Li
metrics, bitratestats, jobstats and latencystats libraries can be made
optional as they provide standalone features.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/proc-info/main.c | 16 ++++++++++++++++
app/proc-info/meson.build | 5 ++++-
app/test-pmd/meson.build | 5 ++++-
app/test-pmd/testpmd.c | 4 ++++
app/test/meson.build | 24 +++++++++++++++---------
lib/meson.build | 4 ++++
6 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index a4271047e6..19223d034c 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -30,7 +30,9 @@
#include <rte_atomic.h>
#include <rte_branch_prediction.h>
#include <rte_string_fns.h>
+#ifdef RTE_LIB_METRICS
#include <rte_metrics.h>
+#endif
#include <rte_cycles.h>
#ifdef RTE_LIB_SECURITY
#include <rte_security.h>
@@ -59,8 +61,10 @@ static uint32_t enable_collectd_format;
static int stdout_fd;
/**< Host id process is running on */
static char host_id[MAX_LONG_OPT_SZ];
+#ifdef RTE_LIB_METRICS
/**< Enable metrics. */
static uint32_t enable_metrics;
+#endif
/**< Enable stats reset. */
static uint32_t reset_stats;
/**< Enable xstats reset. */
@@ -108,8 +112,10 @@ proc_info_usage(const char *prgname)
" --stats: to display port statistics, enabled by default\n"
" --xstats: to display extended port statistics, disabled by "
"default\n"
+#ifdef RTE_LIB_METRICS
" --metrics: to display derived metrics of the ports, disabled by "
"default\n"
+#endif
" --xstats-name NAME: to display single xstat id by NAME\n"
" --xstats-ids IDLIST: to display xstat values by id. "
"The argument is comma-separated list of xstat ids to print out.\n"
@@ -218,7 +224,9 @@ proc_info_parse_args(int argc, char **argv)
{"stats", 0, NULL, 0},
{"stats-reset", 0, NULL, 0},
{"xstats", 0, NULL, 0},
+#ifdef RTE_LIB_METRICS
{"metrics", 0, NULL, 0},
+#endif
{"xstats-reset", 0, NULL, 0},
{"xstats-name", required_argument, NULL, 1},
{"collectd-format", 0, NULL, 0},
@@ -260,10 +268,12 @@ proc_info_parse_args(int argc, char **argv)
else if (!strncmp(long_option[option_index].name, "xstats",
MAX_LONG_OPT_SZ))
enable_xstats = 1;
+#ifdef RTE_LIB_METRICS
else if (!strncmp(long_option[option_index].name,
"metrics",
MAX_LONG_OPT_SZ))
enable_metrics = 1;
+#endif
/* Reset stats */
if (!strncmp(long_option[option_index].name, "stats-reset",
MAX_LONG_OPT_SZ))
@@ -593,6 +603,7 @@ nic_xstats_clear(uint16_t port_id)
printf("\n NIC extended statistics for port %d cleared\n", port_id);
}
+#ifdef RTE_LIB_METRICS
static void
metrics_display(int port_id)
{
@@ -653,6 +664,7 @@ metrics_display(int port_id)
rte_free(metrics);
rte_free(names);
}
+#endif
static void
show_security_context(uint16_t portid, bool inline_offload)
@@ -1522,14 +1534,18 @@ main(int argc, char **argv)
else if (nb_xstats_ids > 0)
nic_xstats_by_ids_display(i, xstats_ids,
nb_xstats_ids);
+#ifdef RTE_LIB_METRICS
else if (enable_metrics)
metrics_display(i);
+#endif
}
+#ifdef RTE_LIB_METRICS
/* print port independent stats */
if (enable_metrics)
metrics_display(RTE_METRICS_GLOBAL);
+#endif
/* show information for PMD */
if (enable_shw_port)
diff --git a/app/proc-info/meson.build b/app/proc-info/meson.build
index 1062e0ef86..1563ce656a 100644
--- a/app/proc-info/meson.build
+++ b/app/proc-info/meson.build
@@ -8,4 +8,7 @@ if is_windows
endif
sources = files('main.c')
-deps += ['ethdev', 'metrics', 'security']
+deps += ['ethdev', 'security']
+if dpdk_conf.has('RTE_LIB_METRICS')
+ deps += 'metrics'
+endif
diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
index eba03b572c..43130c8856 100644
--- a/app/test-pmd/meson.build
+++ b/app/test-pmd/meson.build
@@ -32,7 +32,7 @@ if dpdk_conf.has('RTE_HAS_JANSSON')
ext_deps += jansson_dep
endif
-deps += ['ethdev', 'cmdline', 'metrics', 'bus_pci']
+deps += ['ethdev', 'cmdline', 'bus_pci']
if dpdk_conf.has('RTE_CRYPTO_SCHEDULER')
deps += 'crypto_scheduler'
endif
@@ -52,6 +52,9 @@ endif
if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
deps += 'latencystats'
endif
+if dpdk_conf.has('RTE_LIB_METRICS')
+ deps += 'metrics'
+endif
if dpdk_conf.has('RTE_LIB_PDUMP')
deps += 'pdump'
endif
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e2998b066b..73b8ba7236 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -55,7 +55,9 @@
#include <rte_pdump.h>
#endif
#include <rte_flow.h>
+#ifdef RTE_LIB_METRICS
#include <rte_metrics.h>
+#endif
#ifdef RTE_LIB_BITRATESTATS
#include <rte_bitrate.h>
#endif
@@ -4231,8 +4233,10 @@ main(int argc, char** argv)
port_id, rte_strerror(-ret));
}
+#ifdef RTE_LIB_METRICS
/* Init metrics library */
rte_metrics_init(rte_socket_id());
+#endif
#ifdef RTE_LIB_LATENCYSTATS
if (latencystats_enabled != 0) {
diff --git a/app/test/meson.build b/app/test/meson.build
index a968abac76..b5f1a89edf 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -98,7 +98,6 @@ test_sources = files(
'test_mempool_perf.c',
'test_memzone.c',
'test_meter.c',
- 'test_metrics.c',
'test_mcslock.c',
'test_mp_secondary.c',
'test_per_lcore.c',
@@ -162,7 +161,6 @@ test_deps = [
'acl',
'bus_pci',
'bus_vdev',
- 'bitratestats',
'bpf',
'cfgfile',
'cmdline',
@@ -177,10 +175,8 @@ test_deps = [
'graph',
'hash',
'ipsec',
- 'latencystats',
'lpm',
'member',
- 'metrics',
'node',
'pipeline',
'port',
@@ -281,7 +277,6 @@ fast_tests = [
['kni_autotest', false],
['kvargs_autotest', true],
['member_autotest', true],
- ['metrics_autotest', true],
['power_cpufreq_autotest', false],
['power_autotest', true],
['power_kvm_vm_autotest', false],
@@ -378,6 +373,11 @@ endif
if dpdk_conf.has('RTE_EVENT_SKELETON')
test_deps += 'event_skeleton'
endif
+if dpdk_conf.has('RTE_LIB_METRICS')
+ test_deps += 'metrics'
+ test_sources += ['test_metrics.c']
+ fast_tests += [['metrics_autotest', true]]
+endif
if dpdk_conf.has('RTE_LIB_TELEMETRY')
test_sources += ['test_telemetry_json.c', 'test_telemetry_data.c']
fast_tests += [['telemetry_json_autotest', true], ['telemetry_data_autotest', true]]
@@ -399,16 +399,22 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_pmd_ring_perf.c'
test_sources += 'test_pmd_ring.c'
test_sources += 'test_event_eth_tx_adapter.c'
- test_sources += 'test_bitratestats.c'
- test_sources += 'test_latencystats.c'
test_sources += 'sample_packet_forward.c'
test_sources += 'test_pdump.c'
fast_tests += [['ring_pmd_autotest', true]]
perf_test_names += 'ring_pmd_perf_autotest'
fast_tests += [['event_eth_tx_adapter_autotest', false]]
- fast_tests += [['bitratestats_autotest', true]]
- fast_tests += [['latencystats_autotest', true]]
fast_tests += [['pdump_autotest', true]]
+ if dpdk_conf.has('RTE_LIB_BITRATESTATS')
+ test_deps += 'bitratestats'
+ test_sources += 'test_bitratestats.c'
+ fast_tests += [['bitratestats_autotest', true]]
+ endif
+ if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
+ test_deps += 'latencystats'
+ test_sources += 'test_latencystats.c'
+ fast_tests += [['latencystats_autotest', true]]
+ endif
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
diff --git a/lib/meson.build b/lib/meson.build
index 2766c02bd2..961b95f4ad 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -66,9 +66,13 @@ libraries = [
]
optional_libs = [
+ 'bitratestats',
'gro',
'gso',
'kni',
+ 'jobstats',
+ 'latencystats',
+ 'metrics',
'power',
'vhost',
]
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 4/5] build: make pdump optional
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
` (2 preceding siblings ...)
2021-11-10 16:48 ` [PATCH 3/5] build: make metrics libraries optional David Marchand
@ 2021-11-10 16:48 ` David Marchand
2021-11-16 17:14 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 5/5] build: select optional libraries David Marchand
` (2 subsequent siblings)
6 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-10 16:48 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
This library can be made optional.
dumpcap and pdump applications depend on this library, check for
dependencies like what we have for examples.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/meson.build | 18 +++++++++++++-----
app/test/meson.build | 10 +++++-----
lib/meson.build | 1 +
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/app/meson.build b/app/meson.build
index 310e83076f..93d8c15032 100644
--- a/app/meson.build
+++ b/app/meson.build
@@ -43,15 +43,23 @@ foreach app:apps
subdir(name)
+ if build
+ dep_objs = []
+ foreach d:deps
+ var_name = get_option('default_library') + '_rte_' + d
+ if not is_variable(var_name)
+ build = false
+ message('Missing dependency "@0@" for app "@1@"'.format(d, name))
+ break
+ endif
+ dep_objs += [get_variable(var_name)]
+ endforeach
+ endif
+
if not build
continue
endif
- dep_objs = []
- foreach d:deps
- dep_objs += get_variable(get_option('default_library') + '_rte_' + d)
- endforeach
-
link_libs = []
if get_option('default_library') == 'static'
link_libs = dpdk_static_libraries + dpdk_drivers
diff --git a/app/test/meson.build b/app/test/meson.build
index b5f1a89edf..07fe4870b0 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -400,11 +400,9 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_pmd_ring.c'
test_sources += 'test_event_eth_tx_adapter.c'
test_sources += 'sample_packet_forward.c'
- test_sources += 'test_pdump.c'
fast_tests += [['ring_pmd_autotest', true]]
perf_test_names += 'ring_pmd_perf_autotest'
fast_tests += [['event_eth_tx_adapter_autotest', false]]
- fast_tests += [['pdump_autotest', true]]
if dpdk_conf.has('RTE_LIB_BITRATESTATS')
test_deps += 'bitratestats'
test_sources += 'test_bitratestats.c'
@@ -415,6 +413,11 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_latencystats.c'
fast_tests += [['latencystats_autotest', true]]
endif
+ if dpdk_conf.has('RTE_LIB_PDUMP')
+ test_deps += 'pdump'
+ test_sources += 'test_pdump.c'
+ fast_tests += [['pdump_autotest', true]]
+ endif
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
@@ -431,9 +434,6 @@ endif
if dpdk_conf.has('RTE_LIB_KNI')
test_deps += 'kni'
endif
-if dpdk_conf.has('RTE_LIB_PDUMP')
- test_deps += 'pdump'
-endif
if cc.has_argument('-Wno-format-truncation')
cflags += '-Wno-format-truncation'
diff --git a/lib/meson.build b/lib/meson.build
index 961b95f4ad..dad9fce14d 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -73,6 +73,7 @@ optional_libs = [
'jobstats',
'latencystats',
'metrics',
+ 'pdump',
'power',
'vhost',
]
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 5/5] build: select optional libraries
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
` (3 preceding siblings ...)
2021-11-10 16:48 ` [PATCH 4/5] build: make pdump optional David Marchand
@ 2021-11-10 16:48 ` David Marchand
2021-11-10 17:34 ` Bruce Richardson
2021-11-10 17:34 ` [PATCH 0/5] Extend optional libraries list Bruce Richardson
2021-11-17 11:28 ` [PATCH v2 " David Marchand
6 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-10 16:48 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
There is currently no way to know which libraries are optional.
Introduce a enable_libs option (close to what we have for drivers) so
that packagers or projects consuming DPDK can more easily select the
optional libraries that matter to them and disable other optional
libraries.
Note: the enabled_libs variable is renamed for sake of consistency.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
buildtools/chkincs/meson.build | 2 +-
lib/meson.build | 34 ++++++++++++++++++----------------
meson.build | 3 ++-
meson_options.txt | 2 ++
4 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 5ffca89761..6b93d5f46c 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -18,7 +18,7 @@ sources = files('main.c')
sources += gen_c_files.process(dpdk_chkinc_headers)
deps = []
-foreach l:enabled_libs
+foreach l:dpdk_libs_enabled
deps += get_variable('static_rte_' + l)
endforeach
diff --git a/lib/meson.build b/lib/meson.build
index dad9fce14d..47c857c0fc 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -78,17 +78,10 @@ optional_libs = [
'vhost',
]
-disabled_libs = []
-opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
+enable_libs = run_command(list_dir_globs, get_option('enable_libs'),
+ check: true).stdout().split()
+disable_libs = run_command(list_dir_globs, get_option('disable_libs'),
check: true).stdout().split()
-foreach l:opt_disabled_libs
- if not optional_libs.contains(l)
- warning('Cannot disable mandatory library "@0@"'.format(l))
- continue
- endif
- disabled_libs += l
-endforeach
-
default_cflags = machine_args
default_cflags += ['-DALLOW_EXPERIMENTAL_API']
@@ -98,8 +91,6 @@ if cc.has_argument('-Wno-format-truncation')
default_cflags += '-Wno-format-truncation'
endif
-enabled_libs = [] # used to print summary at the end
-
foreach l:libraries
build = true
reason = '<unknown reason>' # set if build == false to explain why
@@ -123,10 +114,21 @@ foreach l:libraries
deps += ['eal']
endif
- if disabled_libs.contains(l)
- build = false
- reason = 'explicitly disabled via build config'
+ if optional_libs.contains(l)
+ # check for enabled libraries only if one is set in config
+ if enable_libs.length() != 0 and not enable_libs.contains(l)
+ build = false
+ reason = 'not in enabled libraries build config'
+ elif disable_libs.contains(l)
+ build = false
+ reason = 'explicitly disabled via build config'
+ endif
else
+ if disable_libs.contains(l)
+ warning('Cannot disable mandatory library "@0@"'.format(l))
+ endif
+ endif
+ if build
subdir(l)
endif
if name != l
@@ -150,7 +152,7 @@ foreach l:libraries
static_deps += [get_variable('static_rte_' + d)]
endforeach
- enabled_libs += name
+ dpdk_libs_enabled += name
dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
install_headers(headers)
install_headers(indirect_headers)
diff --git a/meson.build b/meson.build
index 12cb6e0e83..65855ceee8 100644
--- a/meson.build
+++ b/meson.build
@@ -35,6 +35,7 @@ dpdk_driver_classes = []
dpdk_drivers = []
dpdk_extra_ldflags = []
dpdk_libs_disabled = []
+dpdk_libs_enabled = []
dpdk_drvs_disabled = []
abi_version_file = files('ABI_VERSION')
@@ -102,7 +103,7 @@ subdir('buildtools/pkg-config')
output_message = '\n=================\nLibraries Enabled\n=================\n'
output_message += '\nlibs:\n\t'
output_count = 0
-foreach lib:enabled_libs
+foreach lib:dpdk_libs_enabled
output_message += lib + ', '
output_count += 1
if output_count == 8
diff --git a/meson_options.txt b/meson_options.txt
index 7c220ad68d..ccb92cff7a 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -16,6 +16,8 @@ option('enable_docs', type: 'boolean', value: false, description:
'build documentation')
option('enable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to build. If unspecified, build all drivers.')
+option('enable_libs', type: 'string', value: '', description:
+ 'Comma-separated list of libraries to explicitly enable.')
option('enable_driver_sdk', type: 'boolean', value: false, description:
'Install headers to build drivers.')
option('enable_kmods', type: 'boolean', value: false, description:
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 5/5] build: select optional libraries
2021-11-10 16:48 ` [PATCH 5/5] build: select optional libraries David Marchand
@ 2021-11-10 17:34 ` Bruce Richardson
2021-11-16 17:25 ` Thomas Monjalon
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2021-11-10 17:34 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
On Wed, Nov 10, 2021 at 05:48:14PM +0100, David Marchand wrote:
> There is currently no way to know which libraries are optional.
> Introduce a enable_libs option (close to what we have for drivers) so
> that packagers or projects consuming DPDK can more easily select the
> optional libraries that matter to them and disable other optional
> libraries.
>
> Note: the enabled_libs variable is renamed for sake of consistency.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
This is the only patch of this set I would have some concerns about. I'm
just not sure that it makes sense to have this option for libraries
compared to drivers.
Specifically:
* We have over 200 drivers in DPDK (rough count using find), of which 2 are
mandatory, and therefore specifying just 1 or 2 that you want can make
sense.
* On the other hand, we have 53 libraries, of which only 7 or so (after
this patchset) are optional. This means that use of the term
"enable_libs" is misleading - at least to me - in that it's only a very
small proportion of the libs which would be affected by that flag
(compared to 99% of the drivers)
* Also, while the number of mandatory drivers is unlikely to change much
(since there are only 2), it should be fairly safe to do builds using
"--enable-drivers". On the other hand, the list of libraries affected by
"--enable-libs" is likely to change, so short of each user naming each
and every lib they use (and each library those depend on), to the list,
it's quite possible that any --enable-libs use could lead to a broken
build in future if a library changes from mandatory to optional.
Overall, I'm just concerned that this flag is premature, and would prefer
to keep it just to the disable option until we are confident that out
"optional library" list is relatively settled.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 0/5] Extend optional libraries list
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
` (4 preceding siblings ...)
2021-11-10 16:48 ` [PATCH 5/5] build: select optional libraries David Marchand
@ 2021-11-10 17:34 ` Bruce Richardson
2021-11-17 11:28 ` [PATCH v2 " David Marchand
6 siblings, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2021-11-10 17:34 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
On Wed, Nov 10, 2021 at 05:48:09PM +0100, David Marchand wrote:
> This series extends the optional libraries list adding to them testpmd
> non essential dependencies.
>
> We were not testing the disable_libs option, so let's add a minimum target
> in the the public CI script (mainly for GHA) and in test-meson-builds.sh
> script.
>
> The last patch is an idea for enhancing the optional libraries selection.
>
> --
> David Marchand
>
> David Marchand (5):
> ci: test build with minimum configuration
> build: make GRO/GSO optional
> build: make metrics libraries optional
> build: make pdump optional
> build: select optional libraries
>
I have concerns about patch 5, but for the rest
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/5] ci: test build with minimum configuration
2021-11-10 16:48 ` [PATCH 1/5] ci: test build with minimum configuration David Marchand
@ 2021-11-16 17:06 ` Thomas Monjalon
2021-11-16 20:39 ` David Marchand
0 siblings, 1 reply; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-16 17:06 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
10/11/2021 17:48, David Marchand:
> Disabling optional libraries was not tested.
> Add a new target in test-meson-builds.sh and GHA.
>
> The Bluefield target is removed from test-meson-builds.sh to save space
> and compilation time in exchange of the new target.
OK to remove build-arm64-bluefield.
We should also remove build-arm64-host-clang which has no benefit.
And instead of adding a new target, can we reuse an existing one,
like build-clang-static or build-arm64-dpaa?
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 2/5] build: make GRO/GSO optional
2021-11-10 16:48 ` [PATCH 2/5] build: make GRO/GSO optional David Marchand
@ 2021-11-16 17:11 ` Thomas Monjalon
0 siblings, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-16 17:11 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Xiaoyun Li
10/11/2021 17:48, David Marchand:
> GRO and GSO integration in testpmd is relatively self contained and easy
> to extract.
> Those libraries can be made optional as they provide standalone
> features.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 3/5] build: make metrics libraries optional
2021-11-10 16:48 ` [PATCH 3/5] build: make metrics libraries optional David Marchand
@ 2021-11-16 17:12 ` Thomas Monjalon
0 siblings, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-16 17:12 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Maryam Tahhan, Reshma Pattan,
Xiaoyun Li
10/11/2021 17:48, David Marchand:
> metrics, bitratestats, jobstats and latencystats libraries can be made
> optional as they provide standalone features.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 4/5] build: make pdump optional
2021-11-10 16:48 ` [PATCH 4/5] build: make pdump optional David Marchand
@ 2021-11-16 17:14 ` Thomas Monjalon
0 siblings, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-16 17:14 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
10/11/2021 17:48, David Marchand:
> This library can be made optional.
> dumpcap and pdump applications depend on this library, check for
> dependencies like what we have for examples.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 5/5] build: select optional libraries
2021-11-10 17:34 ` Bruce Richardson
@ 2021-11-16 17:25 ` Thomas Monjalon
2021-11-17 10:47 ` Bruce Richardson
0 siblings, 1 reply; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-16 17:25 UTC (permalink / raw)
To: David Marchand, Bruce Richardson
Cc: dev, bluca, tredaelli, i.maximets, james.r.harris, mohammed
10/11/2021 18:34, Bruce Richardson:
> On Wed, Nov 10, 2021 at 05:48:14PM +0100, David Marchand wrote:
> > There is currently no way to know which libraries are optional.
> > Introduce a enable_libs option (close to what we have for drivers) so
> > that packagers or projects consuming DPDK can more easily select the
> > optional libraries that matter to them and disable other optional
> > libraries.
> >
> > Note: the enabled_libs variable is renamed for sake of consistency.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> This is the only patch of this set I would have some concerns about. I'm
> just not sure that it makes sense to have this option for libraries
> compared to drivers.
>
> Specifically:
> * We have over 200 drivers in DPDK (rough count using find), of which 2 are
> mandatory, and therefore specifying just 1 or 2 that you want can make
> sense.
> * On the other hand, we have 53 libraries, of which only 7 or so (after
> this patchset) are optional. This means that use of the term
> "enable_libs" is misleading - at least to me - in that it's only a very
> small proportion of the libs which would be affected by that flag
> (compared to 99% of the drivers)
The options are described like this:
option('disable_libs', type: 'string', value: '', description:
'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
+option('enable_libs', type: 'string', value: '', description:
+ 'Comma-separated list of libraries to explicitly enable.')
I feel we should mention it is enabling optional libraries,
and the default is to enable all.
> * Also, while the number of mandatory drivers is unlikely to change much
> (since there are only 2), it should be fairly safe to do builds using
> "--enable-drivers". On the other hand, the list of libraries affected by
> "--enable-libs" is likely to change, so short of each user naming each
> and every lib they use (and each library those depend on), to the list,
> it's quite possible that any --enable-libs use could lead to a broken
> build in future if a library changes from mandatory to optional.
In order to be safe, the user can list all required libs,
including non-optional ones.
> Overall, I'm just concerned that this flag is premature, and would prefer
> to keep it just to the disable option until we are confident that out
> "optional library" list is relatively settled.
I see it the opposite way:
Someone who does not wish to deliver extra libs could use this option
to list the required libs, so not-required libs will disappear from
the build once they are declared optional in future releases.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/5] ci: test build with minimum configuration
2021-11-16 17:06 ` Thomas Monjalon
@ 2021-11-16 20:39 ` David Marchand
2021-11-16 21:47 ` Thomas Monjalon
0 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-16 20:39 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Bruce Richardson, Luca Boccassi, Timothy Redaelli,
Ilya Maximets, Jim Harris, mohammed, Aaron Conole,
Michael Santana
On Tue, Nov 16, 2021 at 6:06 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 10/11/2021 17:48, David Marchand:
> > Disabling optional libraries was not tested.
> > Add a new target in test-meson-builds.sh and GHA.
> >
> > The Bluefield target is removed from test-meson-builds.sh to save space
> > and compilation time in exchange of the new target.
>
> OK to remove build-arm64-bluefield.
> We should also remove build-arm64-host-clang which has no benefit.
To be fair, I originally had no such change and added this following
an offline discussion you and I had :-).
But this is going farther than what this patch is about: testing
disabling components through existing options.
> And instead of adding a new target, can we reuse an existing one,
> like build-clang-static or build-arm64-dpaa?
Reusing means we lose some coverage of the existing target.
I prefer adding a dedicated target.
So I think I'll go back to my original idea and drop the bluefield change.
To make this new target less space/cpu consuming (which impacts us,
maintainers, when merging patches), I can make it a really
minimalistic configuration: I would disable all drivers (but the
net/null one used in test-null.sh).
I don't mind looking into other target usefulness in
test-meson-builds.sh, but as a followup series.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/5] ci: test build with minimum configuration
2021-11-16 20:39 ` David Marchand
@ 2021-11-16 21:47 ` Thomas Monjalon
0 siblings, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-16 21:47 UTC (permalink / raw)
To: David Marchand
Cc: dev, Bruce Richardson, Luca Boccassi, Timothy Redaelli,
Ilya Maximets, Jim Harris, mohammed, Aaron Conole,
Michael Santana
16/11/2021 21:39, David Marchand:
> On Tue, Nov 16, 2021 at 6:06 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 10/11/2021 17:48, David Marchand:
> > > Disabling optional libraries was not tested.
> > > Add a new target in test-meson-builds.sh and GHA.
> > >
> > > The Bluefield target is removed from test-meson-builds.sh to save space
> > > and compilation time in exchange of the new target.
> >
> > OK to remove build-arm64-bluefield.
> > We should also remove build-arm64-host-clang which has no benefit.
>
> To be fair, I originally had no such change and added this following
> an offline discussion you and I had :-).
> But this is going farther than what this patch is about: testing
> disabling components through existing options.
>
>
> > And instead of adding a new target, can we reuse an existing one,
> > like build-clang-static or build-arm64-dpaa?
>
> Reusing means we lose some coverage of the existing target.
> I prefer adding a dedicated target.
>
> So I think I'll go back to my original idea and drop the bluefield change.
> To make this new target less space/cpu consuming (which impacts us,
> maintainers, when merging patches), I can make it a really
> minimalistic configuration: I would disable all drivers (but the
> net/null one used in test-null.sh).
>
>
> I don't mind looking into other target usefulness in
> test-meson-builds.sh, but as a followup series.
I fully agree with this plan.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 5/5] build: select optional libraries
2021-11-16 17:25 ` Thomas Monjalon
@ 2021-11-17 10:47 ` Bruce Richardson
2021-11-17 11:27 ` David Marchand
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2021-11-17 10:47 UTC (permalink / raw)
To: Thomas Monjalon
Cc: David Marchand, dev, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
On Tue, Nov 16, 2021 at 06:25:28PM +0100, Thomas Monjalon wrote:
> 10/11/2021 18:34, Bruce Richardson:
> > On Wed, Nov 10, 2021 at 05:48:14PM +0100, David Marchand wrote:
> > > There is currently no way to know which libraries are optional.
> > > Introduce a enable_libs option (close to what we have for drivers) so
> > > that packagers or projects consuming DPDK can more easily select the
> > > optional libraries that matter to them and disable other optional
> > > libraries.
> > >
> > > Note: the enabled_libs variable is renamed for sake of consistency.
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > This is the only patch of this set I would have some concerns about. I'm
> > just not sure that it makes sense to have this option for libraries
> > compared to drivers.
> >
> > Specifically:
> > * We have over 200 drivers in DPDK (rough count using find), of which 2 are
> > mandatory, and therefore specifying just 1 or 2 that you want can make
> > sense.
> > * On the other hand, we have 53 libraries, of which only 7 or so (after
> > this patchset) are optional. This means that use of the term
> > "enable_libs" is misleading - at least to me - in that it's only a very
> > small proportion of the libs which would be affected by that flag
> > (compared to 99% of the drivers)
>
> The options are described like this:
>
> option('disable_libs', type: 'string', value: '', description:
> 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
> +option('enable_libs', type: 'string', value: '', description:
> + 'Comma-separated list of libraries to explicitly enable.')
>
> I feel we should mention it is enabling optional libraries,
> and the default is to enable all.
>
> > * Also, while the number of mandatory drivers is unlikely to change much
> > (since there are only 2), it should be fairly safe to do builds using
> > "--enable-drivers". On the other hand, the list of libraries affected by
> > "--enable-libs" is likely to change, so short of each user naming each
> > and every lib they use (and each library those depend on), to the list,
> > it's quite possible that any --enable-libs use could lead to a broken
> > build in future if a library changes from mandatory to optional.
>
> In order to be safe, the user can list all required libs,
> including non-optional ones.
>
Yes, they can, but that is the part I'm concerned about. It should not be
expected for users to know what all libraries should be needed and
dependencies between them. The list of mandatory libraries is quite long.
> > Overall, I'm just concerned that this flag is premature, and would prefer
> > to keep it just to the disable option until we are confident that out
> > "optional library" list is relatively settled.
>
> I see it the opposite way:
> Someone who does not wish to deliver extra libs could use this option
> to list the required libs, so not-required libs will disappear from
> the build once they are declared optional in future releases.
>
Yes, though as-above, I'm concerned about the difficulty of building up
such a list. However, if this option is only for "expert" and
distro-packaging use, then I suppose it's reasonable. Perhaps we should add
a warning to the doc line too, noting that it's only recommended for
advanced users.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 5/5] build: select optional libraries
2021-11-17 10:47 ` Bruce Richardson
@ 2021-11-17 11:27 ` David Marchand
2022-01-07 16:13 ` Morten Brørup
0 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:27 UTC (permalink / raw)
To: Bruce Richardson
Cc: Thomas Monjalon, dev, Luca Boccassi, Timothy Redaelli,
Ilya Maximets, Jim Harris, mohammed
On Wed, Nov 17, 2021 at 11:47 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Nov 16, 2021 at 06:25:28PM +0100, Thomas Monjalon wrote:
> > 10/11/2021 18:34, Bruce Richardson:
> > > On Wed, Nov 10, 2021 at 05:48:14PM +0100, David Marchand wrote:
> > > > There is currently no way to know which libraries are optional.
> > > > Introduce a enable_libs option (close to what we have for drivers) so
> > > > that packagers or projects consuming DPDK can more easily select the
> > > > optional libraries that matter to them and disable other optional
> > > > libraries.
> > > >
> > > > Note: the enabled_libs variable is renamed for sake of consistency.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > This is the only patch of this set I would have some concerns about. I'm
> > > just not sure that it makes sense to have this option for libraries
> > > compared to drivers.
> > >
> > > Specifically:
> > > * We have over 200 drivers in DPDK (rough count using find), of which 2 are
> > > mandatory, and therefore specifying just 1 or 2 that you want can make
> > > sense.
> > > * On the other hand, we have 53 libraries, of which only 7 or so (after
> > > this patchset) are optional. This means that use of the term
> > > "enable_libs" is misleading - at least to me - in that it's only a very
> > > small proportion of the libs which would be affected by that flag
> > > (compared to 99% of the drivers)
> >
> > The options are described like this:
> >
> > option('disable_libs', type: 'string', value: '', description:
> > 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
> > +option('enable_libs', type: 'string', value: '', description:
> > + 'Comma-separated list of libraries to explicitly enable.')
> >
> > I feel we should mention it is enabling optional libraries,
> > and the default is to enable all.
> >
> > > * Also, while the number of mandatory drivers is unlikely to change much
> > > (since there are only 2), it should be fairly safe to do builds using
> > > "--enable-drivers". On the other hand, the list of libraries affected by
> > > "--enable-libs" is likely to change, so short of each user naming each
> > > and every lib they use (and each library those depend on), to the list,
> > > it's quite possible that any --enable-libs use could lead to a broken
> > > build in future if a library changes from mandatory to optional.
> >
> > In order to be safe, the user can list all required libs,
> > including non-optional ones.
> >
>
> Yes, they can, but that is the part I'm concerned about. It should not be
> expected for users to know what all libraries should be needed and
> dependencies between them. The list of mandatory libraries is quite long.
>
> > > Overall, I'm just concerned that this flag is premature, and would prefer
> > > to keep it just to the disable option until we are confident that out
> > > "optional library" list is relatively settled.
> >
> > I see it the opposite way:
> > Someone who does not wish to deliver extra libs could use this option
> > to list the required libs, so not-required libs will disappear from
> > the build once they are declared optional in future releases.
> >
>
> Yes, though as-above, I'm concerned about the difficulty of building up
> such a list. However, if this option is only for "expert" and
> distro-packaging use, then I suppose it's reasonable. Perhaps we should add
> a warning to the doc line too, noting that it's only recommended for
> advanced users.
>
Ok, we still need some discussion.
I'll just respin for Thomas to merge patches that are ready.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 0/5] Extend optional libraries list
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
` (5 preceding siblings ...)
2021-11-10 17:34 ` [PATCH 0/5] Extend optional libraries list Bruce Richardson
@ 2021-11-17 11:28 ` David Marchand
2021-11-17 11:28 ` [PATCH v2 1/5] ci: test minimum configuration David Marchand
` (5 more replies)
6 siblings, 6 replies; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:28 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
This series extends the optional libraries list adding to them testpmd
non essential dependencies.
We were not testing the disable_libs option, so let's add a minimum target
in the the public CI script (mainly for GHA) and in test-meson-builds.sh
script.
The last patch is an idea for enhancing the optional libraries selection.
--
David Marchand
Changes since v1:
- only touched patch1,
David Marchand (5):
ci: test minimum configuration
build: make GRO/GSO optional
build: make metrics libraries optional
build: make pdump optional
build: select optional libraries
.ci/linux-build.sh | 4 ++++
.github/workflows/build.yml | 5 +++++
app/meson.build | 18 ++++++++++-----
app/proc-info/main.c | 16 +++++++++++++
app/proc-info/meson.build | 5 ++++-
app/test-pmd/cmdline.c | 14 ++++++++++++
app/test-pmd/config.c | 6 +++++
app/test-pmd/csumonly.c | 35 ++++++++++++++++++++++++-----
app/test-pmd/meson.build | 11 ++++++++-
app/test-pmd/testpmd.c | 18 +++++++++++++++
app/test-pmd/testpmd.h | 18 +++++++++++++++
app/test/meson.build | 34 ++++++++++++++++------------
buildtools/chkincs/meson.build | 2 +-
devtools/test-meson-builds.sh | 3 +++
lib/meson.build | 41 +++++++++++++++++++++-------------
meson.build | 3 ++-
meson_options.txt | 2 ++
17 files changed, 190 insertions(+), 45 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 1/5] ci: test minimum configuration
2021-11-17 11:28 ` [PATCH v2 " David Marchand
@ 2021-11-17 11:28 ` David Marchand
2021-11-17 11:50 ` Thomas Monjalon
2021-11-17 13:38 ` Aaron Conole
2021-11-17 11:28 ` [PATCH v2 2/5] build: make GRO/GSO optional David Marchand
` (4 subsequent siblings)
5 siblings, 2 replies; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:28 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
Disabling drivers and optional libraries was not tested.
Add a new target in test-meson-builds.sh and GHA with just the minimum
to run test-null.sh and any other optional component disabled.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- dropped target cleanup in test-meson-builds.sh, this will be handled
in a separate series in the future,
- disabled all drivers but mempool/ring and net/null,
required to pass test-null.sh. bus/vdev is explicitly enabled for
---
.ci/linux-build.sh | 4 ++++
.github/workflows/build.yml | 5 +++++
devtools/test-meson-builds.sh | 3 +++
3 files changed, 12 insertions(+)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index ef0bd099be..6e8bd7baa9 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -87,6 +87,10 @@ OPTS="$OPTS -Dplatform=generic"
OPTS="$OPTS --default-library=$DEF_LIB"
OPTS="$OPTS --buildtype=debugoptimized"
OPTS="$OPTS -Dcheck_includes=true"
+if [ "$MINI" = "true" ]; then
+ OPTS="$OPTS -Denable_drivers=bus/vdev,mempool/ring,net/null"
+ OPTS="$OPTS -Ddisable_libs=*"
+fi
meson build --werror $OPTS
ninja -C build
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 4151cafee7..2e9c4be6d0 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -21,6 +21,7 @@ jobs:
CC: ccache ${{ matrix.config.compiler }}
DEF_LIB: ${{ matrix.config.library }}
LIBABIGAIL_VERSION: libabigail-1.8
+ MINI: ${{ matrix.config.mini != '' }}
PPC64LE: ${{ matrix.config.cross == 'ppc64le' }}
REF_GIT_TAG: none
RUN_TESTS: ${{ contains(matrix.config.checks, 'tests') }}
@@ -32,6 +33,10 @@ jobs:
- os: ubuntu-18.04
compiler: gcc
library: static
+ - os: ubuntu-18.04
+ compiler: gcc
+ library: shared
+ mini: mini
- os: ubuntu-18.04
compiler: gcc
library: shared
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 9ec8e2bc7e..4ed61328b9 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -220,6 +220,9 @@ for c in gcc clang ; do
done
done
+build build-mini cc skipABI $use_shared -Ddisable_libs=* \
+ -Denable_drivers=bus/vdev,mempool/ring,net/null
+
# test compilation with minimal x86 instruction set
# Set the install path for libraries to "lib" explicitly to prevent problems
# with pkg-config prefixes if installed in "lib/x86_64-linux-gnu" later.
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 2/5] build: make GRO/GSO optional
2021-11-17 11:28 ` [PATCH v2 " David Marchand
2021-11-17 11:28 ` [PATCH v2 1/5] ci: test minimum configuration David Marchand
@ 2021-11-17 11:28 ` David Marchand
2021-11-17 11:28 ` [PATCH v2 3/5] build: make metrics libraries optional David Marchand
` (3 subsequent siblings)
5 siblings, 0 replies; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:28 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Xiaoyun Li
GRO and GSO integration in testpmd is relatively self contained and easy
to extract.
Those libraries can be made optional as they provide standalone
features.
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
app/test-pmd/cmdline.c | 14 ++++++++++++++
app/test-pmd/config.c | 6 ++++++
app/test-pmd/csumonly.c | 35 +++++++++++++++++++++++++++++------
app/test-pmd/meson.build | 8 +++++++-
app/test-pmd/testpmd.c | 14 ++++++++++++++
app/test-pmd/testpmd.h | 18 ++++++++++++++++++
lib/meson.build | 2 ++
7 files changed, 90 insertions(+), 7 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 85d9b57a9b..3faa37db6d 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -34,7 +34,9 @@
#include <rte_string_fns.h>
#include <rte_devargs.h>
#include <rte_flow.h>
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
#include <rte_mbuf_dyn.h>
#include <cmdline_rdline.h>
@@ -458,6 +460,7 @@ static void cmd_help_long_parsed(void *parsed_result,
"tso show (portid)"
" Display the status of TCP Segmentation Offload.\n\n"
+#ifdef RTE_LIB_GRO
"set port (port_id) gro on|off\n"
" Enable or disable Generic Receive Offload in"
" csum forwarding engine.\n\n"
@@ -468,7 +471,9 @@ static void cmd_help_long_parsed(void *parsed_result,
"set gro flush (cycles)\n"
" Set the cycle to flush GROed packets from"
" reassembly tables.\n\n"
+#endif
+#ifdef RTE_LIB_GSO
"set port (port_id) gso (on|off)"
" Enable or disable Generic Segmentation Offload in"
" csum forwarding engine.\n\n"
@@ -479,6 +484,7 @@ static void cmd_help_long_parsed(void *parsed_result,
"show port (port_id) gso\n"
" Show GSO configuration.\n\n"
+#endif
"set fwd (%s)\n"
" Set packet forwarding mode.\n\n"
@@ -5149,6 +5155,7 @@ cmdline_parse_inst_t cmd_tunnel_tso_show = {
},
};
+#ifdef RTE_LIB_GRO
/* *** SET GRO FOR A PORT *** */
struct cmd_gro_enable_result {
cmdline_fixed_string_t cmd_set;
@@ -5292,7 +5299,9 @@ cmdline_parse_inst_t cmd_gro_flush = {
NULL,
},
};
+#endif /* RTE_LIB_GRO */
+#ifdef RTE_LIB_GSO
/* *** ENABLE/DISABLE GSO *** */
struct cmd_gso_enable_result {
cmdline_fixed_string_t cmd_set;
@@ -5459,6 +5468,7 @@ cmdline_parse_inst_t cmd_gso_show = {
NULL,
},
};
+#endif /* RTE_LIB_GSO */
/* *** ENABLE/DISABLE FLUSH ON RX STREAMS *** */
struct cmd_set_flush_rx {
@@ -17660,12 +17670,16 @@ cmdline_parse_ctx_t main_ctx[] = {
(cmdline_parse_inst_t *)&cmd_tso_show,
(cmdline_parse_inst_t *)&cmd_tunnel_tso_set,
(cmdline_parse_inst_t *)&cmd_tunnel_tso_show,
+#ifdef RTE_LIB_GRO
(cmdline_parse_inst_t *)&cmd_gro_enable,
(cmdline_parse_inst_t *)&cmd_gro_flush,
(cmdline_parse_inst_t *)&cmd_gro_show,
+#endif
+#ifdef RTE_LIB_GSO
(cmdline_parse_inst_t *)&cmd_gso_enable,
(cmdline_parse_inst_t *)&cmd_gso_size,
(cmdline_parse_inst_t *)&cmd_gso_show,
+#endif
(cmdline_parse_inst_t *)&cmd_link_flow_control_set,
(cmdline_parse_inst_t *)&cmd_link_flow_control_set_rx,
(cmdline_parse_inst_t *)&cmd_link_flow_control_set_tx,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 2c2ab449b5..6fca09527b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -48,7 +48,9 @@
#ifdef RTE_NET_BNXT
#include <rte_pmd_bnxt.h>
#endif
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
#include <rte_hexdump.h>
#include "testpmd.h"
@@ -4191,6 +4193,7 @@ set_tx_pkt_times(unsigned int *tx_times)
tx_pkt_times_intra = tx_times[1];
}
+#ifdef RTE_LIB_GRO
void
setup_gro(const char *onoff, portid_t port_id)
{
@@ -4272,7 +4275,9 @@ show_gro(portid_t port_id)
} else
printf("Port %u doesn't enable GRO.\n", port_id);
}
+#endif /* RTE_LIB_GRO */
+#ifdef RTE_LIB_GSO
void
setup_gso(const char *mode, portid_t port_id)
{
@@ -4296,6 +4301,7 @@ setup_gso(const char *mode, portid_t port_id)
gso_ports[port_id].enable = 0;
}
}
+#endif /* RTE_LIB_GSO */
char*
list_pkt_forwarding_modes(void)
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index e0b00abe8c..2aeea243b6 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -40,8 +40,12 @@
#include <rte_prefetch.h>
#include <rte_string_fns.h>
#include <rte_flow.h>
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
+#ifdef RTE_LIB_GSO
#include <rte_gso.h>
+#endif
#include <rte_geneve.h>
#include "testpmd.h"
@@ -68,7 +72,9 @@ uint16_t geneve_udp_port = RTE_GENEVE_DEFAULT_PORT;
/* structure that caches offload info for the current packet */
struct testpmd_offload_info {
uint16_t ethertype;
+#ifdef RTE_LIB_GSO
uint8_t gso_enable;
+#endif
uint16_t l2_len;
uint16_t l3_len;
uint16_t l4_len;
@@ -510,8 +516,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
info->ethertype);
}
}
+#ifdef RTE_LIB_GSO
if (info->gso_enable)
ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
+#endif
} else if (info->l4_proto == IPPROTO_TCP) {
tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
if (tso_segsz)
@@ -524,8 +532,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
get_udptcp_checksum(l3_hdr, tcp_hdr,
info->ethertype);
}
+#ifdef RTE_LIB_GSO
if (info->gso_enable)
ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
+#endif
} else if (info->l4_proto == IPPROTO_SCTP) {
sctp_hdr = (struct rte_sctp_hdr *)
((char *)l3_hdr + info->l3_len);
@@ -794,16 +804,20 @@ static void
pkt_burst_checksum_forward(struct fwd_stream *fs)
{
struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
+#ifdef RTE_LIB_GSO
struct rte_mbuf *gso_segments[GSO_MAX_PKT_BURST];
struct rte_gso_ctx *gso_ctx;
+#endif
struct rte_mbuf **tx_pkts_burst;
struct rte_port *txp;
struct rte_mbuf *m, *p;
struct rte_ether_hdr *eth_hdr;
void *l3_hdr = NULL, *outer_l3_hdr = NULL; /* can be IPv4 or IPv6 */
+#ifdef RTE_LIB_GRO
void **gro_ctx;
uint16_t gro_pkts_num;
uint8_t gro_enable;
+#endif
uint16_t nb_rx;
uint16_t nb_tx;
uint16_t nb_prep;
@@ -816,8 +830,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
uint32_t rx_bad_outer_l4_csum;
uint32_t rx_bad_outer_ip_csum;
struct testpmd_offload_info info;
- uint16_t nb_segments = 0;
- int ret;
uint64_t start_tsc = 0;
@@ -835,15 +847,19 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
rx_bad_l4_csum = 0;
rx_bad_outer_l4_csum = 0;
rx_bad_outer_ip_csum = 0;
+#ifdef RTE_LIB_GRO
gro_enable = gro_ports[fs->rx_port].enable;
+#endif
txp = &ports[fs->tx_port];
tx_offloads = txp->dev_conf.txmode.offloads;
memset(&info, 0, sizeof(info));
info.tso_segsz = txp->tso_segsz;
info.tunnel_tso_segsz = txp->tunnel_tso_segsz;
+#ifdef RTE_LIB_GSO
if (gso_ports[fs->tx_port].enable)
info.gso_enable = 1;
+#endif
for (i = 0; i < nb_rx; i++) {
if (likely(i < nb_rx - 1))
@@ -1052,6 +1068,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
}
}
+#ifdef RTE_LIB_GRO
if (unlikely(gro_enable)) {
if (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) {
nb_rx = rte_gro_reassemble_burst(pkts_burst, nb_rx,
@@ -1073,13 +1090,17 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
}
}
}
+#endif
+
+#ifdef RTE_LIB_GSO
+ if (gso_ports[fs->tx_port].enable != 0) {
+ uint16_t nb_segments = 0;
- if (gso_ports[fs->tx_port].enable == 0)
- tx_pkts_burst = pkts_burst;
- else {
gso_ctx = &(current_fwd_lcore()->gso_ctx);
gso_ctx->gso_size = gso_max_segment_size;
for (i = 0; i < nb_rx; i++) {
+ int ret;
+
ret = rte_gso_segment(pkts_burst[i], gso_ctx,
&gso_segments[nb_segments],
GSO_MAX_PKT_BURST - nb_segments);
@@ -1101,7 +1122,9 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
tx_pkts_burst = gso_segments;
nb_rx = nb_segments;
- }
+ } else
+#endif
+ tx_pkts_burst = pkts_burst;
nb_prep = rte_eth_tx_prepare(fs->tx_port, fs->tx_queue,
tx_pkts_burst, nb_rx);
diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
index d5df52c470..eba03b572c 100644
--- a/app/test-pmd/meson.build
+++ b/app/test-pmd/meson.build
@@ -32,7 +32,7 @@ if dpdk_conf.has('RTE_HAS_JANSSON')
ext_deps += jansson_dep
endif
-deps += ['ethdev', 'gro', 'gso', 'cmdline', 'metrics', 'bus_pci']
+deps += ['ethdev', 'cmdline', 'metrics', 'bus_pci']
if dpdk_conf.has('RTE_CRYPTO_SCHEDULER')
deps += 'crypto_scheduler'
endif
@@ -43,6 +43,12 @@ if dpdk_conf.has('RTE_LIB_BPF')
sources += files('bpf_cmd.c')
deps += 'bpf'
endif
+if dpdk_conf.has('RTE_LIB_GRO')
+ deps += 'gro'
+endif
+if dpdk_conf.has('RTE_LIB_GSO')
+ deps += 'gso'
+endif
if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
deps += 'latencystats'
endif
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 59cc4580b2..f74ba61be6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -517,8 +517,10 @@ lcoreid_t bitrate_lcore_id;
uint8_t bitrate_enabled;
#endif
+#ifdef RTE_LIB_GRO
struct gro_status gro_ports[RTE_MAX_ETHPORTS];
uint8_t gro_flush_cycles = GRO_DEFAULT_FLUSH_CYCLES;
+#endif
/*
* hexadecimal bitmask of RX mq mode can be enabled.
@@ -657,8 +659,10 @@ static void fill_xstats_display_info(void);
*/
static int all_ports_started(void);
+#ifdef RTE_LIB_GSO
struct gso_status gso_ports[RTE_MAX_ETHPORTS];
uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN;
+#endif
/* Holds the registered mbuf dynamic flags names. */
char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
@@ -1632,8 +1636,12 @@ init_config(void)
struct rte_mempool *mbp;
unsigned int nb_mbuf_per_pool;
lcoreid_t lc_id;
+#ifdef RTE_LIB_GRO
struct rte_gro_param gro_param;
+#endif
+#ifdef RTE_LIB_GSO
uint32_t gso_types;
+#endif
/* Configuration of logical cores. */
fwd_lcores = rte_zmalloc("testpmd: fwd_lcores",
@@ -1716,8 +1724,10 @@ init_config(void)
init_port_config();
+#ifdef RTE_LIB_GSO
gso_types = RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO;
+#endif
/*
* Records which Mbuf pool to use by each logical core, if needed.
*/
@@ -1728,6 +1738,7 @@ init_config(void)
if (mbp == NULL)
mbp = mbuf_pool_find(0, 0);
fwd_lcores[lc_id]->mbp = mbp;
+#ifdef RTE_LIB_GSO
/* initialize GSO context */
fwd_lcores[lc_id]->gso_ctx.direct_pool = mbp;
fwd_lcores[lc_id]->gso_ctx.indirect_pool = mbp;
@@ -1735,10 +1746,12 @@ init_config(void)
fwd_lcores[lc_id]->gso_ctx.gso_size = RTE_ETHER_MAX_LEN -
RTE_ETHER_CRC_LEN;
fwd_lcores[lc_id]->gso_ctx.flag = 0;
+#endif
}
fwd_config_setup();
+#ifdef RTE_LIB_GRO
/* create a gro context for each lcore */
gro_param.gro_types = RTE_GRO_TCP_IPV4;
gro_param.max_flow_num = GRO_MAX_FLUSH_CYCLES;
@@ -1752,6 +1765,7 @@ init_config(void)
"rte_gro_ctx_create() failed\n");
}
}
+#endif
}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 669ce1e87d..b1dfd097c7 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -9,8 +9,12 @@
#include <rte_pci.h>
#include <rte_bus_pci.h>
+#ifdef RTE_LIB_GRO
#include <rte_gro.h>
+#endif
+#ifdef RTE_LIB_GSO
#include <rte_gso.h>
+#endif
#include <rte_os_shim.h>
#include <cmdline.h>
#include <sys/queue.h>
@@ -143,7 +147,9 @@ struct fwd_stream {
/**< received packets has bad outer l4 checksum */
uint64_t rx_bad_outer_ip_csum;
/**< received packets having bad outer ip checksum */
+#ifdef RTE_LIB_GRO
unsigned int gro_times; /**< GRO operation times */
+#endif
uint64_t core_cycles; /**< used for RX and TX processing */
struct pkt_burst_stats rx_burst_stats;
struct pkt_burst_stats tx_burst_stats;
@@ -264,9 +270,13 @@ struct rte_port {
* CPU id. configuration table.
*/
struct fwd_lcore {
+#ifdef RTE_LIB_GSO
struct rte_gso_ctx gso_ctx; /**< GSO context */
+#endif
struct rte_mempool *mbp; /**< The mbuf pool to use by this core */
+#ifdef RTE_LIB_GRO
void *gro_ctx; /**< GRO context */
+#endif
streamid_t stream_idx; /**< index of 1st stream in "fwd_streams" */
streamid_t stream_nb; /**< number of streams in "fwd_streams" */
lcoreid_t cpuid_idx; /**< index of logical core in CPU id table */
@@ -560,6 +570,7 @@ extern struct rte_ether_addr peer_eth_addrs[RTE_MAX_ETHPORTS];
extern uint32_t burst_tx_delay_time; /**< Burst tx delay time(us) for mac-retry. */
extern uint32_t burst_tx_retry_num; /**< Burst tx retry number for mac-retry. */
+#ifdef RTE_LIB_GRO
#define GRO_DEFAULT_ITEM_NUM_PER_FLOW 32
#define GRO_DEFAULT_FLOW_NUM (RTE_GRO_MAX_BURST_ITEM_NUM / \
GRO_DEFAULT_ITEM_NUM_PER_FLOW)
@@ -573,13 +584,16 @@ struct gro_status {
};
extern struct gro_status gro_ports[RTE_MAX_ETHPORTS];
extern uint8_t gro_flush_cycles;
+#endif /* RTE_LIB_GRO */
+#ifdef RTE_LIB_GSO
#define GSO_MAX_PKT_BURST 2048
struct gso_status {
uint8_t enable;
};
extern struct gso_status gso_ports[RTE_MAX_ETHPORTS];
extern uint16_t gso_max_segment_size;
+#endif /* RTE_LIB_GSO */
/* VXLAN encap/decap parameters. */
struct vxlan_encap_conf {
@@ -1006,10 +1020,14 @@ void port_rss_hash_key_update(portid_t port_id, char rss_type[],
uint8_t *hash_key, uint8_t hash_key_len);
int rx_queue_id_is_invalid(queueid_t rxq_id);
int tx_queue_id_is_invalid(queueid_t txq_id);
+#ifdef RTE_LIB_GRO
void setup_gro(const char *onoff, portid_t port_id);
void setup_gro_flush_cycles(uint8_t cycles);
void show_gro(portid_t port_id);
+#endif
+#ifdef RTE_LIB_GSO
void setup_gso(const char *mode, portid_t port_id);
+#endif
int eth_dev_info_get_print_err(uint16_t port_id,
struct rte_eth_dev_info *dev_info);
int eth_dev_conf_get_print_err(uint16_t port_id,
diff --git a/lib/meson.build b/lib/meson.build
index 8537a5ab80..2766c02bd2 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -66,6 +66,8 @@ libraries = [
]
optional_libs = [
+ 'gro',
+ 'gso',
'kni',
'power',
'vhost',
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 3/5] build: make metrics libraries optional
2021-11-17 11:28 ` [PATCH v2 " David Marchand
2021-11-17 11:28 ` [PATCH v2 1/5] ci: test minimum configuration David Marchand
2021-11-17 11:28 ` [PATCH v2 2/5] build: make GRO/GSO optional David Marchand
@ 2021-11-17 11:28 ` David Marchand
2021-11-17 11:28 ` [PATCH v2 4/5] build: make pdump optional David Marchand
` (2 subsequent siblings)
5 siblings, 0 replies; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:28 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Maryam Tahhan, Reshma Pattan,
Xiaoyun Li
metrics, bitratestats, jobstats and latencystats libraries can be made
optional as they provide standalone features.
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
app/proc-info/main.c | 16 ++++++++++++++++
app/proc-info/meson.build | 5 ++++-
app/test-pmd/meson.build | 5 ++++-
app/test-pmd/testpmd.c | 4 ++++
app/test/meson.build | 24 +++++++++++++++---------
lib/meson.build | 4 ++++
6 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index ebe2d77264..ce140aaf84 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -29,7 +29,9 @@
#include <rte_log.h>
#include <rte_branch_prediction.h>
#include <rte_string_fns.h>
+#ifdef RTE_LIB_METRICS
#include <rte_metrics.h>
+#endif
#include <rte_cycles.h>
#ifdef RTE_LIB_SECURITY
#include <rte_security.h>
@@ -58,8 +60,10 @@ static uint32_t enable_collectd_format;
static int stdout_fd;
/**< Host id process is running on */
static char host_id[MAX_LONG_OPT_SZ];
+#ifdef RTE_LIB_METRICS
/**< Enable metrics. */
static uint32_t enable_metrics;
+#endif
/**< Enable stats reset. */
static uint32_t reset_stats;
/**< Enable xstats reset. */
@@ -107,8 +111,10 @@ proc_info_usage(const char *prgname)
" --stats: to display port statistics, enabled by default\n"
" --xstats: to display extended port statistics, disabled by "
"default\n"
+#ifdef RTE_LIB_METRICS
" --metrics: to display derived metrics of the ports, disabled by "
"default\n"
+#endif
" --xstats-name NAME: to display single xstat id by NAME\n"
" --xstats-ids IDLIST: to display xstat values by id. "
"The argument is comma-separated list of xstat ids to print out.\n"
@@ -217,7 +223,9 @@ proc_info_parse_args(int argc, char **argv)
{"stats", 0, NULL, 0},
{"stats-reset", 0, NULL, 0},
{"xstats", 0, NULL, 0},
+#ifdef RTE_LIB_METRICS
{"metrics", 0, NULL, 0},
+#endif
{"xstats-reset", 0, NULL, 0},
{"xstats-name", required_argument, NULL, 1},
{"collectd-format", 0, NULL, 0},
@@ -259,10 +267,12 @@ proc_info_parse_args(int argc, char **argv)
else if (!strncmp(long_option[option_index].name, "xstats",
MAX_LONG_OPT_SZ))
enable_xstats = 1;
+#ifdef RTE_LIB_METRICS
else if (!strncmp(long_option[option_index].name,
"metrics",
MAX_LONG_OPT_SZ))
enable_metrics = 1;
+#endif
/* Reset stats */
if (!strncmp(long_option[option_index].name, "stats-reset",
MAX_LONG_OPT_SZ))
@@ -592,6 +602,7 @@ nic_xstats_clear(uint16_t port_id)
printf("\n NIC extended statistics for port %d cleared\n", port_id);
}
+#ifdef RTE_LIB_METRICS
static void
metrics_display(int port_id)
{
@@ -652,6 +663,7 @@ metrics_display(int port_id)
rte_free(metrics);
rte_free(names);
}
+#endif
static void
show_security_context(uint16_t portid, bool inline_offload)
@@ -1521,14 +1533,18 @@ main(int argc, char **argv)
else if (nb_xstats_ids > 0)
nic_xstats_by_ids_display(i, xstats_ids,
nb_xstats_ids);
+#ifdef RTE_LIB_METRICS
else if (enable_metrics)
metrics_display(i);
+#endif
}
+#ifdef RTE_LIB_METRICS
/* print port independent stats */
if (enable_metrics)
metrics_display(RTE_METRICS_GLOBAL);
+#endif
/* show information for PMD */
if (enable_shw_port)
diff --git a/app/proc-info/meson.build b/app/proc-info/meson.build
index 1062e0ef86..1563ce656a 100644
--- a/app/proc-info/meson.build
+++ b/app/proc-info/meson.build
@@ -8,4 +8,7 @@ if is_windows
endif
sources = files('main.c')
-deps += ['ethdev', 'metrics', 'security']
+deps += ['ethdev', 'security']
+if dpdk_conf.has('RTE_LIB_METRICS')
+ deps += 'metrics'
+endif
diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
index eba03b572c..43130c8856 100644
--- a/app/test-pmd/meson.build
+++ b/app/test-pmd/meson.build
@@ -32,7 +32,7 @@ if dpdk_conf.has('RTE_HAS_JANSSON')
ext_deps += jansson_dep
endif
-deps += ['ethdev', 'cmdline', 'metrics', 'bus_pci']
+deps += ['ethdev', 'cmdline', 'bus_pci']
if dpdk_conf.has('RTE_CRYPTO_SCHEDULER')
deps += 'crypto_scheduler'
endif
@@ -52,6 +52,9 @@ endif
if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
deps += 'latencystats'
endif
+if dpdk_conf.has('RTE_LIB_METRICS')
+ deps += 'metrics'
+endif
if dpdk_conf.has('RTE_LIB_PDUMP')
deps += 'pdump'
endif
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f74ba61be6..ba65342b6d 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -54,7 +54,9 @@
#include <rte_pdump.h>
#endif
#include <rte_flow.h>
+#ifdef RTE_LIB_METRICS
#include <rte_metrics.h>
+#endif
#ifdef RTE_LIB_BITRATESTATS
#include <rte_bitrate.h>
#endif
@@ -4242,8 +4244,10 @@ main(int argc, char** argv)
port_id, rte_strerror(-ret));
}
+#ifdef RTE_LIB_METRICS
/* Init metrics library */
rte_metrics_init(rte_socket_id());
+#endif
#ifdef RTE_LIB_LATENCYSTATS
if (latencystats_enabled != 0) {
diff --git a/app/test/meson.build b/app/test/meson.build
index 2191a5e146..0d261b1138 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -98,7 +98,6 @@ test_sources = files(
'test_mempool_perf.c',
'test_memzone.c',
'test_meter.c',
- 'test_metrics.c',
'test_mcslock.c',
'test_mp_secondary.c',
'test_per_lcore.c',
@@ -162,7 +161,6 @@ test_deps = [
'acl',
'bus_pci',
'bus_vdev',
- 'bitratestats',
'bpf',
'cfgfile',
'cmdline',
@@ -177,10 +175,8 @@ test_deps = [
'graph',
'hash',
'ipsec',
- 'latencystats',
'lpm',
'member',
- 'metrics',
'node',
'pipeline',
'port',
@@ -280,7 +276,6 @@ fast_tests = [
['kni_autotest', false],
['kvargs_autotest', true],
['member_autotest', true],
- ['metrics_autotest', true],
['power_cpufreq_autotest', false],
['power_autotest', true],
['power_kvm_vm_autotest', false],
@@ -385,6 +380,11 @@ endif
if dpdk_conf.has('RTE_EVENT_SKELETON')
test_deps += 'event_skeleton'
endif
+if dpdk_conf.has('RTE_LIB_METRICS')
+ test_deps += 'metrics'
+ test_sources += ['test_metrics.c']
+ fast_tests += [['metrics_autotest', true]]
+endif
if dpdk_conf.has('RTE_LIB_TELEMETRY')
test_sources += ['test_telemetry_json.c', 'test_telemetry_data.c']
fast_tests += [['telemetry_json_autotest', true], ['telemetry_data_autotest', true]]
@@ -406,16 +406,22 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_pmd_ring_perf.c'
test_sources += 'test_pmd_ring.c'
test_sources += 'test_event_eth_tx_adapter.c'
- test_sources += 'test_bitratestats.c'
- test_sources += 'test_latencystats.c'
test_sources += 'sample_packet_forward.c'
test_sources += 'test_pdump.c'
fast_tests += [['ring_pmd_autotest', true]]
perf_test_names += 'ring_pmd_perf_autotest'
fast_tests += [['event_eth_tx_adapter_autotest', false]]
- fast_tests += [['bitratestats_autotest', true]]
- fast_tests += [['latencystats_autotest', true]]
fast_tests += [['pdump_autotest', true]]
+ if dpdk_conf.has('RTE_LIB_BITRATESTATS')
+ test_deps += 'bitratestats'
+ test_sources += 'test_bitratestats.c'
+ fast_tests += [['bitratestats_autotest', true]]
+ endif
+ if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
+ test_deps += 'latencystats'
+ test_sources += 'test_latencystats.c'
+ fast_tests += [['latencystats_autotest', true]]
+ endif
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
diff --git a/lib/meson.build b/lib/meson.build
index 2766c02bd2..961b95f4ad 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -66,9 +66,13 @@ libraries = [
]
optional_libs = [
+ 'bitratestats',
'gro',
'gso',
'kni',
+ 'jobstats',
+ 'latencystats',
+ 'metrics',
'power',
'vhost',
]
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 4/5] build: make pdump optional
2021-11-17 11:28 ` [PATCH v2 " David Marchand
` (2 preceding siblings ...)
2021-11-17 11:28 ` [PATCH v2 3/5] build: make metrics libraries optional David Marchand
@ 2021-11-17 11:28 ` David Marchand
2021-11-17 11:28 ` [PATCH v2 5/5] build: select optional libraries David Marchand
2021-11-17 11:52 ` [PATCH v2 0/5] Extend optional libraries list Thomas Monjalon
5 siblings, 0 replies; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:28 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
This library can be made optional.
dumpcap and pdump applications depend on this library, check for
dependencies like what we have for examples.
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
app/meson.build | 18 +++++++++++++-----
app/test/meson.build | 10 +++++-----
lib/meson.build | 1 +
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/app/meson.build b/app/meson.build
index 310e83076f..93d8c15032 100644
--- a/app/meson.build
+++ b/app/meson.build
@@ -43,15 +43,23 @@ foreach app:apps
subdir(name)
+ if build
+ dep_objs = []
+ foreach d:deps
+ var_name = get_option('default_library') + '_rte_' + d
+ if not is_variable(var_name)
+ build = false
+ message('Missing dependency "@0@" for app "@1@"'.format(d, name))
+ break
+ endif
+ dep_objs += [get_variable(var_name)]
+ endforeach
+ endif
+
if not build
continue
endif
- dep_objs = []
- foreach d:deps
- dep_objs += get_variable(get_option('default_library') + '_rte_' + d)
- endforeach
-
link_libs = []
if get_option('default_library') == 'static'
link_libs = dpdk_static_libraries + dpdk_drivers
diff --git a/app/test/meson.build b/app/test/meson.build
index 0d261b1138..961bebc5cb 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -407,11 +407,9 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_pmd_ring.c'
test_sources += 'test_event_eth_tx_adapter.c'
test_sources += 'sample_packet_forward.c'
- test_sources += 'test_pdump.c'
fast_tests += [['ring_pmd_autotest', true]]
perf_test_names += 'ring_pmd_perf_autotest'
fast_tests += [['event_eth_tx_adapter_autotest', false]]
- fast_tests += [['pdump_autotest', true]]
if dpdk_conf.has('RTE_LIB_BITRATESTATS')
test_deps += 'bitratestats'
test_sources += 'test_bitratestats.c'
@@ -422,6 +420,11 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_latencystats.c'
fast_tests += [['latencystats_autotest', true]]
endif
+ if dpdk_conf.has('RTE_LIB_PDUMP')
+ test_deps += 'pdump'
+ test_sources += 'test_pdump.c'
+ fast_tests += [['pdump_autotest', true]]
+ endif
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
@@ -438,9 +441,6 @@ endif
if dpdk_conf.has('RTE_LIB_KNI')
test_deps += 'kni'
endif
-if dpdk_conf.has('RTE_LIB_PDUMP')
- test_deps += 'pdump'
-endif
if cc.has_argument('-Wno-format-truncation')
cflags += '-Wno-format-truncation'
diff --git a/lib/meson.build b/lib/meson.build
index 961b95f4ad..dad9fce14d 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -73,6 +73,7 @@ optional_libs = [
'jobstats',
'latencystats',
'metrics',
+ 'pdump',
'power',
'vhost',
]
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 5/5] build: select optional libraries
2021-11-17 11:28 ` [PATCH v2 " David Marchand
` (3 preceding siblings ...)
2021-11-17 11:28 ` [PATCH v2 4/5] build: make pdump optional David Marchand
@ 2021-11-17 11:28 ` David Marchand
2023-06-16 7:14 ` [PATCH v3] " David Marchand
` (2 more replies)
2021-11-17 11:52 ` [PATCH v2 0/5] Extend optional libraries list Thomas Monjalon
5 siblings, 3 replies; 70+ messages in thread
From: David Marchand @ 2021-11-17 11:28 UTC (permalink / raw)
To: dev
Cc: bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
There is currently no way to know which libraries are optional.
Introduce a enable_libs option (close to what we have for drivers) so
that packagers or projects consuming DPDK can more easily select the
optional libraries that matter to them and disable other optional
libraries.
Note: the enabled_libs variable is renamed for sake of consistency.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
buildtools/chkincs/meson.build | 2 +-
lib/meson.build | 34 ++++++++++++++++++----------------
meson.build | 3 ++-
meson_options.txt | 2 ++
4 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 5ffca89761..6b93d5f46c 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -18,7 +18,7 @@ sources = files('main.c')
sources += gen_c_files.process(dpdk_chkinc_headers)
deps = []
-foreach l:enabled_libs
+foreach l:dpdk_libs_enabled
deps += get_variable('static_rte_' + l)
endforeach
diff --git a/lib/meson.build b/lib/meson.build
index dad9fce14d..47c857c0fc 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -78,17 +78,10 @@ optional_libs = [
'vhost',
]
-disabled_libs = []
-opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
+enable_libs = run_command(list_dir_globs, get_option('enable_libs'),
+ check: true).stdout().split()
+disable_libs = run_command(list_dir_globs, get_option('disable_libs'),
check: true).stdout().split()
-foreach l:opt_disabled_libs
- if not optional_libs.contains(l)
- warning('Cannot disable mandatory library "@0@"'.format(l))
- continue
- endif
- disabled_libs += l
-endforeach
-
default_cflags = machine_args
default_cflags += ['-DALLOW_EXPERIMENTAL_API']
@@ -98,8 +91,6 @@ if cc.has_argument('-Wno-format-truncation')
default_cflags += '-Wno-format-truncation'
endif
-enabled_libs = [] # used to print summary at the end
-
foreach l:libraries
build = true
reason = '<unknown reason>' # set if build == false to explain why
@@ -123,10 +114,21 @@ foreach l:libraries
deps += ['eal']
endif
- if disabled_libs.contains(l)
- build = false
- reason = 'explicitly disabled via build config'
+ if optional_libs.contains(l)
+ # check for enabled libraries only if one is set in config
+ if enable_libs.length() != 0 and not enable_libs.contains(l)
+ build = false
+ reason = 'not in enabled libraries build config'
+ elif disable_libs.contains(l)
+ build = false
+ reason = 'explicitly disabled via build config'
+ endif
else
+ if disable_libs.contains(l)
+ warning('Cannot disable mandatory library "@0@"'.format(l))
+ endif
+ endif
+ if build
subdir(l)
endif
if name != l
@@ -150,7 +152,7 @@ foreach l:libraries
static_deps += [get_variable('static_rte_' + d)]
endforeach
- enabled_libs += name
+ dpdk_libs_enabled += name
dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
install_headers(headers)
install_headers(indirect_headers)
diff --git a/meson.build b/meson.build
index 12cb6e0e83..65855ceee8 100644
--- a/meson.build
+++ b/meson.build
@@ -35,6 +35,7 @@ dpdk_driver_classes = []
dpdk_drivers = []
dpdk_extra_ldflags = []
dpdk_libs_disabled = []
+dpdk_libs_enabled = []
dpdk_drvs_disabled = []
abi_version_file = files('ABI_VERSION')
@@ -102,7 +103,7 @@ subdir('buildtools/pkg-config')
output_message = '\n=================\nLibraries Enabled\n=================\n'
output_message += '\nlibs:\n\t'
output_count = 0
-foreach lib:enabled_libs
+foreach lib:dpdk_libs_enabled
output_message += lib + ', '
output_count += 1
if output_count == 8
diff --git a/meson_options.txt b/meson_options.txt
index 7c220ad68d..ccb92cff7a 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -16,6 +16,8 @@ option('enable_docs', type: 'boolean', value: false, description:
'build documentation')
option('enable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to build. If unspecified, build all drivers.')
+option('enable_libs', type: 'string', value: '', description:
+ 'Comma-separated list of libraries to explicitly enable.')
option('enable_driver_sdk', type: 'boolean', value: false, description:
'Install headers to build drivers.')
option('enable_kmods', type: 'boolean', value: false, description:
--
2.23.0
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/5] ci: test minimum configuration
2021-11-17 11:28 ` [PATCH v2 1/5] ci: test minimum configuration David Marchand
@ 2021-11-17 11:50 ` Thomas Monjalon
2021-11-17 13:38 ` Aaron Conole
1 sibling, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-17 11:50 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
17/11/2021 12:28, David Marchand:
> Disabling drivers and optional libraries was not tested.
> Add a new target in test-meson-builds.sh and GHA with just the minimum
> to run test-null.sh and any other optional component disabled.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - dropped target cleanup in test-meson-builds.sh, this will be handled
> in a separate series in the future,
> - disabled all drivers but mempool/ring and net/null,
> required to pass test-null.sh. bus/vdev is explicitly enabled for
Acked-by: Thomas Monjalon <thomas@monjalon.net>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/5] Extend optional libraries list
2021-11-17 11:28 ` [PATCH v2 " David Marchand
` (4 preceding siblings ...)
2021-11-17 11:28 ` [PATCH v2 5/5] build: select optional libraries David Marchand
@ 2021-11-17 11:52 ` Thomas Monjalon
5 siblings, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2021-11-17 11:52 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
17/11/2021 12:28, David Marchand:
> This series extends the optional libraries list adding to them testpmd
> non essential dependencies.
>
> We were not testing the disable_libs option, so let's add a minimum target
> in the the public CI script (mainly for GHA) and in test-meson-builds.sh
> script.
>
> The last patch is an idea for enhancing the optional libraries selection.
Applied all but last patch, thanks.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/5] ci: test minimum configuration
2021-11-17 11:28 ` [PATCH v2 1/5] ci: test minimum configuration David Marchand
2021-11-17 11:50 ` Thomas Monjalon
@ 2021-11-17 13:38 ` Aaron Conole
1 sibling, 0 replies; 70+ messages in thread
From: Aaron Conole @ 2021-11-17 13:38 UTC (permalink / raw)
To: David Marchand
Cc: dev, bruce.richardson, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Michael Santana
David Marchand <david.marchand@redhat.com> writes:
> Disabling drivers and optional libraries was not tested.
> Add a new target in test-meson-builds.sh and GHA with just the minimum
> to run test-null.sh and any other optional component disabled.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - dropped target cleanup in test-meson-builds.sh, this will be handled
> in a separate series in the future,
> - disabled all drivers but mempool/ring and net/null,
> required to pass test-null.sh. bus/vdev is explicitly enabled for
>
Acked-by: Aaron Conole <aconole@redhat.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 5/5] build: select optional libraries
2021-11-17 11:27 ` David Marchand
@ 2022-01-07 16:13 ` Morten Brørup
2022-01-07 16:47 ` Stephen Hemminger
0 siblings, 1 reply; 70+ messages in thread
From: Morten Brørup @ 2022-01-07 16:13 UTC (permalink / raw)
To: David Marchand, Bruce Richardson
Cc: Thomas Monjalon, dev, Luca Boccassi, Timothy Redaelli,
Ilya Maximets, Jim Harris, mohammed
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 17 November 2021 12.27
>
> On Wed, Nov 17, 2021 at 11:47 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Nov 16, 2021 at 06:25:28PM +0100, Thomas Monjalon wrote:
> > > 10/11/2021 18:34, Bruce Richardson:
> > > > On Wed, Nov 10, 2021 at 05:48:14PM +0100, David Marchand wrote:
> > > > > There is currently no way to know which libraries are optional.
> > > > > Introduce a enable_libs option (close to what we have for
> drivers) so
> > > > > that packagers or projects consuming DPDK can more easily
> select the
> > > > > optional libraries that matter to them and disable other
> optional
> > > > > libraries.
> > > > >
> > > > > Note: the enabled_libs variable is renamed for sake of
> consistency.
> > > > >
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > > ---
> > > > This is the only patch of this set I would have some concerns
> about. I'm
> > > > just not sure that it makes sense to have this option for
> libraries
> > > > compared to drivers.
> > > >
> > > > Specifically:
> > > > * We have over 200 drivers in DPDK (rough count using find), of
> which 2 are
> > > > mandatory, and therefore specifying just 1 or 2 that you want
> can make
> > > > sense.
> > > > * On the other hand, we have 53 libraries, of which only 7 or so
> (after
> > > > this patchset) are optional. This means that use of the term
> > > > "enable_libs" is misleading - at least to me - in that it's
> only a very
> > > > small proportion of the libs which would be affected by that
> flag
> > > > (compared to 99% of the drivers)
> > >
> > > The options are described like this:
> > >
> > > option('disable_libs', type: 'string', value: '', description:
> > > 'Comma-separated list of libraries to explicitly disable.
> [NOTE: not all libs can be disabled]')
> > > +option('enable_libs', type: 'string', value: '', description:
> > > + 'Comma-separated list of libraries to explicitly enable.')
> > >
> > > I feel we should mention it is enabling optional libraries,
> > > and the default is to enable all.
> > >
> > > > * Also, while the number of mandatory drivers is unlikely to
> change much
> > > > (since there are only 2), it should be fairly safe to do builds
> using
> > > > "--enable-drivers". On the other hand, the list of libraries
> affected by
> > > > "--enable-libs" is likely to change, so short of each user
> naming each
> > > > and every lib they use (and each library those depend on), to
> the list,
> > > > it's quite possible that any --enable-libs use could lead to a
> broken
> > > > build in future if a library changes from mandatory to
> optional.
> > >
> > > In order to be safe, the user can list all required libs,
> > > including non-optional ones.
> > >
> >
> > Yes, they can, but that is the part I'm concerned about. It should
> not be
> > expected for users to know what all libraries should be needed and
> > dependencies between them. The list of mandatory libraries is quite
> long.
> >
> > > > Overall, I'm just concerned that this flag is premature, and
> would prefer
> > > > to keep it just to the disable option until we are confident that
> out
> > > > "optional library" list is relatively settled.
> > >
> > > I see it the opposite way:
> > > Someone who does not wish to deliver extra libs could use this
> option
> > > to list the required libs, so not-required libs will disappear from
> > > the build once they are declared optional in future releases.
> > >
> >
> > Yes, though as-above, I'm concerned about the difficulty of building
> up
> > such a list. However, if this option is only for "expert" and
> > distro-packaging use, then I suppose it's reasonable. Perhaps we
> should add
> > a warning to the doc line too, noting that it's only recommended for
> > advanced users.
> >
>
> Ok, we still need some discussion.
> I'll just respin for Thomas to merge patches that are ready.
This patch is a step in the right direction. Thank you.
Adding my opinion to the discussion will be a long rant from a grumpy old minimalist (working on dedicated network appliances, not Linux distros), so here goes:
The goal should be making all non-core libraries optional. And off the top of my head, these are the only core libraries:
EAL, mbuf, mempool, ring.
And yes, that also means that not even the simplest example applications can compile with only the core libraries. However, the ability to build a minimal dpdk.so should not depend on what is required to be able to build any example applications. In other words: If an example application cannot be built due to an omitted library, then do not build that application.
E.g. an Ethernet bridge application does not need any IP or routing libraries, so they are not core libraries.
I consider the mbuf library and the libraries it uses (i.e. mempool and ring) core libraries, because the core purpose of DPDK is to provide a data plane library, and all DPDK data plane application use mbufs. :-)
BTW, I also consider the ethdev library extremely bloated, with Flow, Metering, Traffic Management and ever more being added into one big monolithic library instead of separate libraries or modules.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 5/5] build: select optional libraries
2022-01-07 16:13 ` Morten Brørup
@ 2022-01-07 16:47 ` Stephen Hemminger
0 siblings, 0 replies; 70+ messages in thread
From: Stephen Hemminger @ 2022-01-07 16:47 UTC (permalink / raw)
To: Morten Brørup
Cc: David Marchand, Bruce Richardson, Thomas Monjalon, dev,
Luca Boccassi, Timothy Redaelli, Ilya Maximets, Jim Harris,
mohammed
On Fri, 7 Jan 2022 17:13:04 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Wednesday, 17 November 2021 12.27
> >
> > On Wed, Nov 17, 2021 at 11:47 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Tue, Nov 16, 2021 at 06:25:28PM +0100, Thomas Monjalon wrote:
> > > > 10/11/2021 18:34, Bruce Richardson:
> > > > > On Wed, Nov 10, 2021 at 05:48:14PM +0100, David Marchand wrote:
> > > > > > There is currently no way to know which libraries are optional.
> > > > > > Introduce a enable_libs option (close to what we have for
> > drivers) so
> > > > > > that packagers or projects consuming DPDK can more easily
> > select the
> > > > > > optional libraries that matter to them and disable other
> > optional
> > > > > > libraries.
> > > > > >
> > > > > > Note: the enabled_libs variable is renamed for sake of
> > consistency.
> > > > > >
> > > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > > > ---
> > > > > This is the only patch of this set I would have some concerns
> > about. I'm
> > > > > just not sure that it makes sense to have this option for
> > libraries
> > > > > compared to drivers.
> > > > >
> > > > > Specifically:
> > > > > * We have over 200 drivers in DPDK (rough count using find), of
> > which 2 are
> > > > > mandatory, and therefore specifying just 1 or 2 that you want
> > can make
> > > > > sense.
> > > > > * On the other hand, we have 53 libraries, of which only 7 or so
> > (after
> > > > > this patchset) are optional. This means that use of the term
> > > > > "enable_libs" is misleading - at least to me - in that it's
> > only a very
> > > > > small proportion of the libs which would be affected by that
> > flag
> > > > > (compared to 99% of the drivers)
> > > >
> > > > The options are described like this:
> > > >
> > > > option('disable_libs', type: 'string', value: '', description:
> > > > 'Comma-separated list of libraries to explicitly disable.
> > [NOTE: not all libs can be disabled]')
> > > > +option('enable_libs', type: 'string', value: '', description:
> > > > + 'Comma-separated list of libraries to explicitly enable.')
> > > >
> > > > I feel we should mention it is enabling optional libraries,
> > > > and the default is to enable all.
> > > >
> > > > > * Also, while the number of mandatory drivers is unlikely to
> > change much
> > > > > (since there are only 2), it should be fairly safe to do builds
> > using
> > > > > "--enable-drivers". On the other hand, the list of libraries
> > affected by
> > > > > "--enable-libs" is likely to change, so short of each user
> > naming each
> > > > > and every lib they use (and each library those depend on), to
> > the list,
> > > > > it's quite possible that any --enable-libs use could lead to a
> > broken
> > > > > build in future if a library changes from mandatory to
> > optional.
> > > >
> > > > In order to be safe, the user can list all required libs,
> > > > including non-optional ones.
> > > >
> > >
> > > Yes, they can, but that is the part I'm concerned about. It should
> > not be
> > > expected for users to know what all libraries should be needed and
> > > dependencies between them. The list of mandatory libraries is quite
> > long.
> > >
> > > > > Overall, I'm just concerned that this flag is premature, and
> > would prefer
> > > > > to keep it just to the disable option until we are confident that
> > out
> > > > > "optional library" list is relatively settled.
> > > >
> > > > I see it the opposite way:
> > > > Someone who does not wish to deliver extra libs could use this
> > option
> > > > to list the required libs, so not-required libs will disappear from
> > > > the build once they are declared optional in future releases.
> > > >
> > >
> > > Yes, though as-above, I'm concerned about the difficulty of building
> > up
> > > such a list. However, if this option is only for "expert" and
> > > distro-packaging use, then I suppose it's reasonable. Perhaps we
> > should add
> > > a warning to the doc line too, noting that it's only recommended for
> > > advanced users.
> > >
> >
> > Ok, we still need some discussion.
> > I'll just respin for Thomas to merge patches that are ready.
>
> This patch is a step in the right direction. Thank you.
>
> Adding my opinion to the discussion will be a long rant from a grumpy old minimalist (working on dedicated network appliances, not Linux distros), so here goes:
>
> The goal should be making all non-core libraries optional. And off the top of my head, these are the only core libraries:
> EAL, mbuf, mempool, ring.
>
> And yes, that also means that not even the simplest example applications can compile with only the core libraries. However, the ability to build a minimal dpdk.so should not depend on what is required to be able to build any example applications. In other words: If an example application cannot be built due to an omitted library, then do not build that application.
>
> E.g. an Ethernet bridge application does not need any IP or routing libraries, so they are not core libraries.
>
> I consider the mbuf library and the libraries it uses (i.e. mempool and ring) core libraries, because the core purpose of DPDK is to provide a data plane library, and all DPDK data plane application use mbufs. :-)
>
> BTW, I also consider the ethdev library extremely bloated, with Flow, Metering, Traffic Management and ever more being added into one big monolithic library instead of separate libraries or modules.
>
Agreed, DPDK has become fat and now requires lots of things like meter and telemetry
to build.
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v3] build: select optional libraries
2021-11-17 11:28 ` [PATCH v2 5/5] build: select optional libraries David Marchand
@ 2023-06-16 7:14 ` David Marchand
2023-06-16 9:42 ` Bruce Richardson
2023-06-19 14:11 ` David Marchand
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
2023-06-28 13:20 ` [PATCH v5 0/2] " David Marchand
2 siblings, 2 replies; 70+ messages in thread
From: David Marchand @ 2023-06-16 7:14 UTC (permalink / raw)
To: dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Aaron Conole, Michael Santana, Bruce Richardson
There is currently no way to know which libraries are optional.
Introduce a enable_libs option (close to what we have for drivers) so
that packagers or projects consuming DPDK can more easily select the
optional libraries that matter to them and disable other optional
libraries.
Deprecated libraries are handled via some logic in lib/meson.build
rather than a default value in meson_options.txt.
Enabling deprecated libraries is achieved by requesting all
libraries to be built in the CI.
kni dependency to IOVA configuration is moved to its own meson.build.
Note: the enabled_libs variable is renamed for sake of consistency wrt to
drivers and applications similar variables.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
The topic was raised again while Stephen was reviewing stale patches in
patchwork. The idea of this patch is the same as before.
I simply rebased the change (and dealt with the deprecated libraries that
added some little complication).
Changes since v2:
- moved the IOVA check for kni build in lib/kni/meson.build,
- reworked deprecated libraries handling by only considering them when
no enable_libs option is set. With this, no need for a default value
in meson_options.txt, yet configurations in the CI must be adjusted,
- moved mandatory libraries check after enable/disable_libs evaluation,
---
.ci/linux-build.sh | 2 +-
app/test/meson.build | 2 +-
buildtools/chkincs/meson.build | 2 +-
devtools/test-meson-builds.sh | 4 ++--
lib/kni/meson.build | 5 ++++
lib/meson.build | 44 ++++++++++++++++++++--------------
meson.build | 3 ++-
meson_options.txt | 4 +++-
8 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 9631e342b5..a4e474a900 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -97,7 +97,7 @@ if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
else
- OPTS="$OPTS -Ddisable_libs="
+ OPTS="$OPTS -Denable_libs=*"
fi
if [ "$ASAN" = "true" ]; then
diff --git a/app/test/meson.build b/app/test/meson.build
index d0fabcbb8b..f3217ae577 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -152,7 +152,7 @@ test_sources = files(
'virtual_pmd.c',
)
-test_deps = enabled_libs
+test_deps = dpdk_libs_enabled
# as well as libs, the pci and vdev bus drivers are needed for a lot of tests
test_deps += ['bus_pci', 'bus_vdev']
diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 378c2f19ef..f2dadcae18 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -22,7 +22,7 @@ sources += gen_c_files.process(dpdk_chkinc_headers)
# so we always include them in deps list
deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
# add the rest of the libs to the dependencies
-foreach l:enabled_libs
+foreach l:dpdk_libs_enabled
deps += get_variable('shared_rte_' + l)
endforeach
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 9131088c9d..4f93702e1f 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -122,8 +122,8 @@ config () # <dir> <builddir> <meson options>
options=
# deprecated libs may be disabled by default, so for complete builds ensure
# no libs are disabled
- if ! echo $* | grep -q -- 'disable_libs' ; then
- options="$options -Ddisable_libs="
+ if ! echo $* | grep -q -- 'enable_libs' ; then
+ options="$options -Denable_libs=*"
fi
if echo $* | grep -qw -- '--default-library=shared' ; then
options="$options -Dexamples=all"
diff --git a/lib/kni/meson.build b/lib/kni/meson.build
index 8a71d8ba6f..5ce410f7f2 100644
--- a/lib/kni/meson.build
+++ b/lib/kni/meson.build
@@ -7,6 +7,11 @@ if is_windows
subdir_done()
endif
+if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
+ build = false
+ reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'
+endif
+
if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
build = false
reason = 'only supported on 64-bit Linux'
diff --git a/lib/meson.build b/lib/meson.build
index 9677239236..6018f5230d 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -92,20 +92,18 @@ dpdk_libs_deprecated += [
'kni',
]
-disabled_libs = []
-opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
+disable_libs = run_command(list_dir_globs, get_option('disable_libs'),
check: true).stdout().split()
-if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
- opt_disabled_libs += ['kni']
+enable_libs = run_command(list_dir_globs, get_option('enable_libs'),
+ check: true).stdout().split()
+if enable_libs.length() == 0
+ enable_libs = []
+ foreach l:run_command(list_dir_globs, '*', check: true).stdout().split()
+ if l not in dpdk_libs_deprecated
+ enable_libs += [l]
+ endif
+ endforeach
endif
-foreach l:opt_disabled_libs
- if not optional_libs.contains(l)
- warning('Cannot disable mandatory library "@0@"'.format(l))
- continue
- endif
- disabled_libs += l
-endforeach
-
default_cflags = machine_args
default_cflags += ['-DALLOW_EXPERIMENTAL_API']
@@ -115,8 +113,6 @@ if cc.has_argument('-Wno-format-truncation')
default_cflags += '-Wno-format-truncation'
endif
-enabled_libs = [] # used to print summary at the end
-
foreach l:libraries
build = true
reason = '<unknown reason>' # set if build == false to explain why
@@ -141,13 +137,25 @@ foreach l:libraries
deps += ['eal']
endif
- if disabled_libs.contains(l)
+ if not enable_libs.contains(l)
+ build = false
+ reason = 'not in enabled libraries build config'
+ elif disable_libs.contains(l)
build = false
reason = 'explicitly disabled via build config'
- if dpdk_libs_deprecated.contains(l)
+ endif
+
+ if not build
+ if not optional_libs.contains(l)
+ warning('Cannot disable mandatory library "@0@"'.format(l))
+ build = true
+ reason = '<unknown reason>'
+ elif dpdk_libs_deprecated.contains(l)
reason += ' (deprecated lib)'
endif
- else
+ endif
+
+ if build
if dpdk_libs_deprecated.contains(l)
warning('Enabling deprecated library, "@0@"'.format(l))
endif
@@ -183,7 +191,7 @@ foreach l:libraries
continue
endif
- enabled_libs += name
+ dpdk_libs_enabled += name
dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
install_headers(headers)
install_headers(indirect_headers)
diff --git a/meson.build b/meson.build
index 992ca91e88..39cb73846d 100644
--- a/meson.build
+++ b/meson.build
@@ -44,6 +44,7 @@ dpdk_extra_ldflags = []
dpdk_libs_deprecated = []
dpdk_apps_disabled = []
dpdk_libs_disabled = []
+dpdk_libs_enabled = []
dpdk_drvs_disabled = []
testpmd_drivers_sources = []
testpmd_drivers_deps = []
@@ -134,7 +135,7 @@ message(output_message + '\n')
output_message = '\n=================\nLibraries Enabled\n=================\n'
output_message += '\nlibs:\n\t'
output_count = 0
-foreach lib:enabled_libs
+foreach lib:dpdk_libs_enabled
output_message += lib + ', '
output_count += 1
if output_count == 8
diff --git a/meson_options.txt b/meson_options.txt
index 82c8297065..af54f8b8d4 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', description:
'Comma-separated list of apps to explicitly disable.')
option('disable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to explicitly disable.')
-option('disable_libs', type: 'string', value: 'flow_classify,kni', description:
+option('disable_libs', type: 'string', value: '', description:
'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
@@ -20,6 +20,8 @@ option('enable_apps', type: 'string', value: '', description:
'Comma-separated list of apps to build. If unspecified, build all apps.')
option('enable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to build. If unspecified, build all drivers.')
+option('enable_libs', type: 'string', value: '', description:
+ 'Comma-separated list of libraries to explicitly enable.')
option('enable_driver_sdk', type: 'boolean', value: false, description:
'Install headers to build drivers.')
option('enable_kmods', type: 'boolean', value: false, description:
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-16 7:14 ` [PATCH v3] " David Marchand
@ 2023-06-16 9:42 ` Bruce Richardson
2023-06-19 8:00 ` David Marchand
2023-06-19 14:11 ` David Marchand
1 sibling, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-16 9:42 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Fri, Jun 16, 2023 at 09:14:50AM +0200, David Marchand wrote:
> There is currently no way to know which libraries are optional.
> Introduce a enable_libs option (close to what we have for drivers) so
> that packagers or projects consuming DPDK can more easily select the
> optional libraries that matter to them and disable other optional
> libraries.
>
> Deprecated libraries are handled via some logic in lib/meson.build
> rather than a default value in meson_options.txt.
> Enabling deprecated libraries is achieved by requesting all
> libraries to be built in the CI.
>
> kni dependency to IOVA configuration is moved to its own meson.build.
>
> Note: the enabled_libs variable is renamed for sake of consistency wrt to
> drivers and applications similar variables.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Thanks David.
Some comments inline below just on the usability aspects of this.
/Bruce
> The topic was raised again while Stephen was reviewing stale patches in
> patchwork. The idea of this patch is the same as before.
> I simply rebased the change (and dealt with the deprecated libraries that
> added some little complication).
>
> Changes since v2:
> - moved the IOVA check for kni build in lib/kni/meson.build,
> - reworked deprecated libraries handling by only considering them when
> no enable_libs option is set. With this, no need for a default value
> in meson_options.txt, yet configurations in the CI must be adjusted,
> - moved mandatory libraries check after enable/disable_libs evaluation,
>
> ---
> .ci/linux-build.sh | 2 +-
> app/test/meson.build | 2 +-
> buildtools/chkincs/meson.build | 2 +-
> devtools/test-meson-builds.sh | 4 ++--
> lib/kni/meson.build | 5 ++++
> lib/meson.build | 44 ++++++++++++++++++++--------------
> meson.build | 3 ++-
> meson_options.txt | 4 +++-
> 8 files changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 9631e342b5..a4e474a900 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -97,7 +97,7 @@ if [ "$MINI" = "true" ]; then
> OPTS="$OPTS -Denable_drivers=net/null"
> OPTS="$OPTS -Ddisable_libs=*"
> else
> - OPTS="$OPTS -Ddisable_libs="
> + OPTS="$OPTS -Denable_libs=*"
> fi
>
> if [ "$ASAN" = "true" ]; then
> diff --git a/app/test/meson.build b/app/test/meson.build
> index d0fabcbb8b..f3217ae577 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -152,7 +152,7 @@ test_sources = files(
> 'virtual_pmd.c',
> )
>
> -test_deps = enabled_libs
> +test_deps = dpdk_libs_enabled
> # as well as libs, the pci and vdev bus drivers are needed for a lot of tests
> test_deps += ['bus_pci', 'bus_vdev']
>
> diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
> index 378c2f19ef..f2dadcae18 100644
> --- a/buildtools/chkincs/meson.build
> +++ b/buildtools/chkincs/meson.build
> @@ -22,7 +22,7 @@ sources += gen_c_files.process(dpdk_chkinc_headers)
> # so we always include them in deps list
> deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
> # add the rest of the libs to the dependencies
> -foreach l:enabled_libs
> +foreach l:dpdk_libs_enabled
> deps += get_variable('shared_rte_' + l)
> endforeach
>
> diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
> index 9131088c9d..4f93702e1f 100755
> --- a/devtools/test-meson-builds.sh
> +++ b/devtools/test-meson-builds.sh
> @@ -122,8 +122,8 @@ config () # <dir> <builddir> <meson options>
> options=
> # deprecated libs may be disabled by default, so for complete builds ensure
> # no libs are disabled
> - if ! echo $* | grep -q -- 'disable_libs' ; then
> - options="$options -Ddisable_libs="
> + if ! echo $* | grep -q -- 'enable_libs' ; then
> + options="$options -Denable_libs=*"
> fi
> if echo $* | grep -qw -- '--default-library=shared' ; then
> options="$options -Dexamples=all"
> diff --git a/lib/kni/meson.build b/lib/kni/meson.build
> index 8a71d8ba6f..5ce410f7f2 100644
> --- a/lib/kni/meson.build
> +++ b/lib/kni/meson.build
> @@ -7,6 +7,11 @@ if is_windows
> subdir_done()
> endif
>
> +if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
> + build = false
> + reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'
> +endif
> +
> if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
> build = false
> reason = 'only supported on 64-bit Linux'
> diff --git a/lib/meson.build b/lib/meson.build
> index 9677239236..6018f5230d 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -92,20 +92,18 @@ dpdk_libs_deprecated += [
> 'kni',
> ]
>
> -disabled_libs = []
> -opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
> +disable_libs = run_command(list_dir_globs, get_option('disable_libs'),
> check: true).stdout().split()
> -if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
> - opt_disabled_libs += ['kni']
> +enable_libs = run_command(list_dir_globs, get_option('enable_libs'),
> + check: true).stdout().split()
> +if enable_libs.length() == 0
> + enable_libs = []
> + foreach l:run_command(list_dir_globs, '*', check: true).stdout().split()
> + if l not in dpdk_libs_deprecated
> + enable_libs += [l]
> + endif
> + endforeach
The enable libraries option is really only used for enabling optional
libraries - mandatory libs should not need to be specified. I'm wondering
about if we should append the mandatory libs to the glob search
automatically. This specifically concerns the error messsage about
mandatory libs being disabled below....
> endif
> -foreach l:opt_disabled_libs
> - if not optional_libs.contains(l)
> - warning('Cannot disable mandatory library "@0@"'.format(l))
> - continue
> - endif
> - disabled_libs += l
> -endforeach
> -
>
> default_cflags = machine_args
> default_cflags += ['-DALLOW_EXPERIMENTAL_API']
> @@ -115,8 +113,6 @@ if cc.has_argument('-Wno-format-truncation')
> default_cflags += '-Wno-format-truncation'
> endif
>
> -enabled_libs = [] # used to print summary at the end
> -
> foreach l:libraries
> build = true
> reason = '<unknown reason>' # set if build == false to explain why
> @@ -141,13 +137,25 @@ foreach l:libraries
> deps += ['eal']
> endif
>
> - if disabled_libs.contains(l)
> + if not enable_libs.contains(l)
> + build = false
> + reason = 'not in enabled libraries build config'
> + elif disable_libs.contains(l)
> build = false
> reason = 'explicitly disabled via build config'
> - if dpdk_libs_deprecated.contains(l)
> + endif
> +
> + if not build
> + if not optional_libs.contains(l)
> + warning('Cannot disable mandatory library "@0@"'.format(l))
> + build = true
> + reason = '<unknown reason>'
> + elif dpdk_libs_deprecated.contains(l)
This error message seems weird/wrong in the case that the user uses
"enable_libs" option rather than "disable_libs". If the user specified he
wanted to enable the vhost lib, for example, it would be strange getting an
error about having disabled eal, mbuf etc. etc.
> reason += ' (deprecated lib)'
> endif
> - else
> + endif
> +
> + if build
> if dpdk_libs_deprecated.contains(l)
> warning('Enabling deprecated library, "@0@"'.format(l))
> endif
> @@ -183,7 +191,7 @@ foreach l:libraries
> continue
> endif
>
> - enabled_libs += name
> + dpdk_libs_enabled += name
> dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
> install_headers(headers)
> install_headers(indirect_headers)
> diff --git a/meson.build b/meson.build
> index 992ca91e88..39cb73846d 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -44,6 +44,7 @@ dpdk_extra_ldflags = []
> dpdk_libs_deprecated = []
> dpdk_apps_disabled = []
> dpdk_libs_disabled = []
> +dpdk_libs_enabled = []
> dpdk_drvs_disabled = []
> testpmd_drivers_sources = []
> testpmd_drivers_deps = []
> @@ -134,7 +135,7 @@ message(output_message + '\n')
> output_message = '\n=================\nLibraries Enabled\n=================\n'
> output_message += '\nlibs:\n\t'
> output_count = 0
> -foreach lib:enabled_libs
> +foreach lib:dpdk_libs_enabled
> output_message += lib + ', '
> output_count += 1
> if output_count == 8
> diff --git a/meson_options.txt b/meson_options.txt
> index 82c8297065..af54f8b8d4 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', description:
> 'Comma-separated list of apps to explicitly disable.')
> option('disable_drivers', type: 'string', value: '', description:
> 'Comma-separated list of drivers to explicitly disable.')
> -option('disable_libs', type: 'string', value: 'flow_classify,kni', description:
> +option('disable_libs', type: 'string', value: '', description:
> 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
> option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
> 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
> @@ -20,6 +20,8 @@ option('enable_apps', type: 'string', value: '', description:
> 'Comma-separated list of apps to build. If unspecified, build all apps.')
> option('enable_drivers', type: 'string', value: '', description:
> 'Comma-separated list of drivers to build. If unspecified, build all drivers.')
> +option('enable_libs', type: 'string', value: '', description:
> + 'Comma-separated list of libraries to explicitly enable.')
s/libraries/optional libraries/ ??
Maybe like with the disable option, add a note that mandatory libs are
always enabled?
> option('enable_driver_sdk', type: 'boolean', value: false, description:
> 'Install headers to build drivers.')
> option('enable_kmods', type: 'boolean', value: false, description:
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-16 9:42 ` Bruce Richardson
@ 2023-06-19 8:00 ` David Marchand
0 siblings, 0 replies; 70+ messages in thread
From: David Marchand @ 2023-06-19 8:00 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Fri, Jun 16, 2023 at 11:42 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Fri, Jun 16, 2023 at 09:14:50AM +0200, David Marchand wrote:
> > There is currently no way to know which libraries are optional.
> > Introduce a enable_libs option (close to what we have for drivers) so
> > that packagers or projects consuming DPDK can more easily select the
> > optional libraries that matter to them and disable other optional
> > libraries.
> >
> > Deprecated libraries are handled via some logic in lib/meson.build
> > rather than a default value in meson_options.txt.
> > Enabling deprecated libraries is achieved by requesting all
> > libraries to be built in the CI.
> >
> > kni dependency to IOVA configuration is moved to its own meson.build.
> >
> > Note: the enabled_libs variable is renamed for sake of consistency wrt to
> > drivers and applications similar variables.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> Thanks David.
> Some comments inline below just on the usability aspects of this.
>
> /Bruce
>
> > The topic was raised again while Stephen was reviewing stale patches in
> > patchwork. The idea of this patch is the same as before.
> > I simply rebased the change (and dealt with the deprecated libraries that
> > added some little complication).
> >
> > Changes since v2:
> > - moved the IOVA check for kni build in lib/kni/meson.build,
> > - reworked deprecated libraries handling by only considering them when
> > no enable_libs option is set. With this, no need for a default value
> > in meson_options.txt, yet configurations in the CI must be adjusted,
> > - moved mandatory libraries check after enable/disable_libs evaluation,
> >
> > ---
> > .ci/linux-build.sh | 2 +-
> > app/test/meson.build | 2 +-
> > buildtools/chkincs/meson.build | 2 +-
> > devtools/test-meson-builds.sh | 4 ++--
> > lib/kni/meson.build | 5 ++++
> > lib/meson.build | 44 ++++++++++++++++++++--------------
> > meson.build | 3 ++-
> > meson_options.txt | 4 +++-
> > 8 files changed, 41 insertions(+), 25 deletions(-)
> >
> > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> > index 9631e342b5..a4e474a900 100755
> > --- a/.ci/linux-build.sh
> > +++ b/.ci/linux-build.sh
> > @@ -97,7 +97,7 @@ if [ "$MINI" = "true" ]; then
> > OPTS="$OPTS -Denable_drivers=net/null"
> > OPTS="$OPTS -Ddisable_libs=*"
> > else
> > - OPTS="$OPTS -Ddisable_libs="
> > + OPTS="$OPTS -Denable_libs=*"
> > fi
> >
> > if [ "$ASAN" = "true" ]; then
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index d0fabcbb8b..f3217ae577 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -152,7 +152,7 @@ test_sources = files(
> > 'virtual_pmd.c',
> > )
> >
> > -test_deps = enabled_libs
> > +test_deps = dpdk_libs_enabled
> > # as well as libs, the pci and vdev bus drivers are needed for a lot of tests
> > test_deps += ['bus_pci', 'bus_vdev']
> >
> > diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
> > index 378c2f19ef..f2dadcae18 100644
> > --- a/buildtools/chkincs/meson.build
> > +++ b/buildtools/chkincs/meson.build
> > @@ -22,7 +22,7 @@ sources += gen_c_files.process(dpdk_chkinc_headers)
> > # so we always include them in deps list
> > deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
> > # add the rest of the libs to the dependencies
> > -foreach l:enabled_libs
> > +foreach l:dpdk_libs_enabled
> > deps += get_variable('shared_rte_' + l)
> > endforeach
> >
> > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
> > index 9131088c9d..4f93702e1f 100755
> > --- a/devtools/test-meson-builds.sh
> > +++ b/devtools/test-meson-builds.sh
> > @@ -122,8 +122,8 @@ config () # <dir> <builddir> <meson options>
> > options=
> > # deprecated libs may be disabled by default, so for complete builds ensure
> > # no libs are disabled
> > - if ! echo $* | grep -q -- 'disable_libs' ; then
> > - options="$options -Ddisable_libs="
> > + if ! echo $* | grep -q -- 'enable_libs' ; then
> > + options="$options -Denable_libs=*"
> > fi
> > if echo $* | grep -qw -- '--default-library=shared' ; then
> > options="$options -Dexamples=all"
> > diff --git a/lib/kni/meson.build b/lib/kni/meson.build
> > index 8a71d8ba6f..5ce410f7f2 100644
> > --- a/lib/kni/meson.build
> > +++ b/lib/kni/meson.build
> > @@ -7,6 +7,11 @@ if is_windows
> > subdir_done()
> > endif
> >
> > +if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
> > + build = false
> > + reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'
> > +endif
> > +
> > if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
> > build = false
> > reason = 'only supported on 64-bit Linux'
> > diff --git a/lib/meson.build b/lib/meson.build
> > index 9677239236..6018f5230d 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -92,20 +92,18 @@ dpdk_libs_deprecated += [
> > 'kni',
> > ]
> >
> > -disabled_libs = []
> > -opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
> > +disable_libs = run_command(list_dir_globs, get_option('disable_libs'),
> > check: true).stdout().split()
> > -if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
> > - opt_disabled_libs += ['kni']
> > +enable_libs = run_command(list_dir_globs, get_option('enable_libs'),
> > + check: true).stdout().split()
> > +if enable_libs.length() == 0
> > + enable_libs = []
> > + foreach l:run_command(list_dir_globs, '*', check: true).stdout().split()
> > + if l not in dpdk_libs_deprecated
> > + enable_libs += [l]
> > + endif
> > + endforeach
>
> The enable libraries option is really only used for enabling optional
> libraries - mandatory libs should not need to be specified. I'm wondering
> about if we should append the mandatory libs to the glob search
> automatically. This specifically concerns the error messsage about
> mandatory libs being disabled below....
Good point.
Let me relook at this.
>
> > endif
> > -foreach l:opt_disabled_libs
> > - if not optional_libs.contains(l)
> > - warning('Cannot disable mandatory library "@0@"'.format(l))
> > - continue
> > - endif
> > - disabled_libs += l
> > -endforeach
> > -
> >
> > default_cflags = machine_args
> > default_cflags += ['-DALLOW_EXPERIMENTAL_API']
> > @@ -115,8 +113,6 @@ if cc.has_argument('-Wno-format-truncation')
> > default_cflags += '-Wno-format-truncation'
> > endif
> >
> > -enabled_libs = [] # used to print summary at the end
> > -
> > foreach l:libraries
> > build = true
> > reason = '<unknown reason>' # set if build == false to explain why
> > @@ -141,13 +137,25 @@ foreach l:libraries
> > deps += ['eal']
> > endif
> >
> > - if disabled_libs.contains(l)
> > + if not enable_libs.contains(l)
> > + build = false
> > + reason = 'not in enabled libraries build config'
> > + elif disable_libs.contains(l)
> > build = false
> > reason = 'explicitly disabled via build config'
> > - if dpdk_libs_deprecated.contains(l)
> > + endif
> > +
> > + if not build
> > + if not optional_libs.contains(l)
> > + warning('Cannot disable mandatory library "@0@"'.format(l))
> > + build = true
> > + reason = '<unknown reason>'
> > + elif dpdk_libs_deprecated.contains(l)
>
> This error message seems weird/wrong in the case that the user uses
> "enable_libs" option rather than "disable_libs". If the user specified he
> wanted to enable the vhost lib, for example, it would be strange getting an
> error about having disabled eal, mbuf etc. etc.
Ack.
>
> > reason += ' (deprecated lib)'
> > endif
> > - else
> > + endif
> > +
> > + if build
> > if dpdk_libs_deprecated.contains(l)
> > warning('Enabling deprecated library, "@0@"'.format(l))
> > endif
> > @@ -183,7 +191,7 @@ foreach l:libraries
> > continue
> > endif
> >
> > - enabled_libs += name
> > + dpdk_libs_enabled += name
> > dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
> > install_headers(headers)
> > install_headers(indirect_headers)
> > diff --git a/meson.build b/meson.build
> > index 992ca91e88..39cb73846d 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -44,6 +44,7 @@ dpdk_extra_ldflags = []
> > dpdk_libs_deprecated = []
> > dpdk_apps_disabled = []
> > dpdk_libs_disabled = []
> > +dpdk_libs_enabled = []
> > dpdk_drvs_disabled = []
> > testpmd_drivers_sources = []
> > testpmd_drivers_deps = []
> > @@ -134,7 +135,7 @@ message(output_message + '\n')
> > output_message = '\n=================\nLibraries Enabled\n=================\n'
> > output_message += '\nlibs:\n\t'
> > output_count = 0
> > -foreach lib:enabled_libs
> > +foreach lib:dpdk_libs_enabled
> > output_message += lib + ', '
> > output_count += 1
> > if output_count == 8
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 82c8297065..af54f8b8d4 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', description:
> > 'Comma-separated list of apps to explicitly disable.')
> > option('disable_drivers', type: 'string', value: '', description:
> > 'Comma-separated list of drivers to explicitly disable.')
> > -option('disable_libs', type: 'string', value: 'flow_classify,kni', description:
> > +option('disable_libs', type: 'string', value: '', description:
> > 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
> > option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
> > 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
> > @@ -20,6 +20,8 @@ option('enable_apps', type: 'string', value: '', description:
> > 'Comma-separated list of apps to build. If unspecified, build all apps.')
> > option('enable_drivers', type: 'string', value: '', description:
> > 'Comma-separated list of drivers to build. If unspecified, build all drivers.')
> > +option('enable_libs', type: 'string', value: '', description:
> > + 'Comma-separated list of libraries to explicitly enable.')
>
> s/libraries/optional libraries/ ??
> Maybe like with the disable option, add a note that mandatory libs are
> always enabled?
I'll update this too.
>
> > option('enable_driver_sdk', type: 'boolean', value: false, description:
> > 'Install headers to build drivers.')
> > option('enable_kmods', type: 'boolean', value: false, description:
> > --
> > 2.40.1
> >
>
Thanks Bruce.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-16 7:14 ` [PATCH v3] " David Marchand
2023-06-16 9:42 ` Bruce Richardson
@ 2023-06-19 14:11 ` David Marchand
2023-06-19 14:26 ` Bruce Richardson
1 sibling, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-19 14:11 UTC (permalink / raw)
To: dev, Bruce Richardson
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Aaron Conole, Michael Santana
On Fri, Jun 16, 2023 at 9:21 AM David Marchand
<david.marchand@redhat.com> wrote:
> @@ -141,13 +137,25 @@ foreach l:libraries
> deps += ['eal']
> endif
>
> - if disabled_libs.contains(l)
> + if not enable_libs.contains(l)
> + build = false
> + reason = 'not in enabled libraries build config'
> + elif disable_libs.contains(l)
> build = false
> reason = 'explicitly disabled via build config'
> - if dpdk_libs_deprecated.contains(l)
> + endif
There is also a change in behavior for current users of the
-Ddisable_libs= configuration (which was used for enabling deprecated
libraries, for example).
My current solution resides in making disable_libs and enable_libs
options being mutually exclusive (meaning that presence of a value for
enable_libs will ignore any configuration around disable_libs).
Does it look ok to you?
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-19 14:11 ` David Marchand
@ 2023-06-19 14:26 ` Bruce Richardson
2023-06-20 8:31 ` David Marchand
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-19 14:26 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> <david.marchand@redhat.com> wrote:
> > @@ -141,13 +137,25 @@ foreach l:libraries
> > deps += ['eal']
> > endif
> >
> > - if disabled_libs.contains(l)
> > + if not enable_libs.contains(l)
> > + build = false
> > + reason = 'not in enabled libraries build config'
> > + elif disable_libs.contains(l)
> > build = false
> > reason = 'explicitly disabled via build config'
> > - if dpdk_libs_deprecated.contains(l)
> > + endif
>
> There is also a change in behavior for current users of the
> -Ddisable_libs= configuration (which was used for enabling deprecated
> libraries, for example).
I notice the change in behaviour for enabling the deprecated libs. Is there
any other change in behaviour for current users?
> My current solution resides in making disable_libs and enable_libs
> options being mutually exclusive (meaning that presence of a value for
> enable_libs will ignore any configuration around disable_libs).
>
> Does it look ok to you?
>
Do we need to make them mutually exclusive? The current drivers
implementation allows them to be used together, I think.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-19 14:26 ` Bruce Richardson
@ 2023-06-20 8:31 ` David Marchand
2023-06-20 8:35 ` Bruce Richardson
0 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-20 8:31 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > > @@ -141,13 +137,25 @@ foreach l:libraries
> > > deps += ['eal']
> > > endif
> > >
> > > - if disabled_libs.contains(l)
> > > + if not enable_libs.contains(l)
> > > + build = false
> > > + reason = 'not in enabled libraries build config'
> > > + elif disable_libs.contains(l)
> > > build = false
> > > reason = 'explicitly disabled via build config'
> > > - if dpdk_libs_deprecated.contains(l)
> > > + endif
> >
> > There is also a change in behavior for current users of the
> > -Ddisable_libs= configuration (which was used for enabling deprecated
> > libraries, for example).
>
> I notice the change in behaviour for enabling the deprecated libs. Is there
> any other change in behaviour for current users?
The only change I see, is that this implementation breaks enabling
deprecated libs via disable_libs.
It may break existing developer build directory and maybe some
packaging scripts, this is why I am a bit puzzled.
Relooking at the disable_libs option current implementation, it seems
backward to pass a disable_libs option when you want to build some
deprecated library.
It is more straightforward to request building libraries via
-Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
implicitly.
But again, we may be breaking something for people who relied on this behavior.
>
> > My current solution resides in making disable_libs and enable_libs
> > options being mutually exclusive (meaning that presence of a value for
> > enable_libs will ignore any configuration around disable_libs).
> >
> > Does it look ok to you?
> >
> Do we need to make them mutually exclusive? The current drivers
> implementation allows them to be used together, I think.
I would prefer we are consistent with the drivers options.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 8:31 ` David Marchand
@ 2023-06-20 8:35 ` Bruce Richardson
2023-06-20 8:38 ` David Marchand
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-20 8:35 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Tue, Jun 20, 2023 at 10:31:19AM +0200, David Marchand wrote:
> On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > > @@ -141,13 +137,25 @@ foreach l:libraries
> > > > deps += ['eal']
> > > > endif
> > > >
> > > > - if disabled_libs.contains(l)
> > > > + if not enable_libs.contains(l)
> > > > + build = false
> > > > + reason = 'not in enabled libraries build config'
> > > > + elif disable_libs.contains(l)
> > > > build = false
> > > > reason = 'explicitly disabled via build config'
> > > > - if dpdk_libs_deprecated.contains(l)
> > > > + endif
> > >
> > > There is also a change in behavior for current users of the
> > > -Ddisable_libs= configuration (which was used for enabling deprecated
> > > libraries, for example).
> >
> > I notice the change in behaviour for enabling the deprecated libs. Is there
> > any other change in behaviour for current users?
>
> The only change I see, is that this implementation breaks enabling
> deprecated libs via disable_libs.
> It may break existing developer build directory and maybe some
> packaging scripts, this is why I am a bit puzzled.
>
> Relooking at the disable_libs option current implementation, it seems
> backward to pass a disable_libs option when you want to build some
> deprecated library.
> It is more straightforward to request building libraries via
> -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> implicitly.
>
> But again, we may be breaking something for people who relied on this behavior.
>
That's what I expected, and I think that is ok. I just wanted to check that
the change in behaviour was only for the deprecated libs case.
> >
> > > My current solution resides in making disable_libs and enable_libs
> > > options being mutually exclusive (meaning that presence of a value for
> > > enable_libs will ignore any configuration around disable_libs).
> > >
> > > Does it look ok to you?
> > >
> > Do we need to make them mutually exclusive? The current drivers
> > implementation allows them to be used together, I think.
>
> I would prefer we are consistent with the drivers options.
>
Yep, I definitely agree. Both drivers and libs should have the same
behaviour.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 8:35 ` Bruce Richardson
@ 2023-06-20 8:38 ` David Marchand
2023-06-20 8:44 ` Bruce Richardson
0 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-20 8:38 UTC (permalink / raw)
To: Bruce Richardson, thomas
Cc: dev, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Aaron Conole, Michael Santana
On Tue, Jun 20, 2023 at 10:36 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Tue, Jun 20, 2023 at 10:31:19AM +0200, David Marchand wrote:
> > On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > > > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > > > <david.marchand@redhat.com> wrote:
> > > > > @@ -141,13 +137,25 @@ foreach l:libraries
> > > > > deps += ['eal']
> > > > > endif
> > > > >
> > > > > - if disabled_libs.contains(l)
> > > > > + if not enable_libs.contains(l)
> > > > > + build = false
> > > > > + reason = 'not in enabled libraries build config'
> > > > > + elif disable_libs.contains(l)
> > > > > build = false
> > > > > reason = 'explicitly disabled via build config'
> > > > > - if dpdk_libs_deprecated.contains(l)
> > > > > + endif
> > > >
> > > > There is also a change in behavior for current users of the
> > > > -Ddisable_libs= configuration (which was used for enabling deprecated
> > > > libraries, for example).
> > >
> > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > any other change in behaviour for current users?
> >
> > The only change I see, is that this implementation breaks enabling
> > deprecated libs via disable_libs.
> > It may break existing developer build directory and maybe some
> > packaging scripts, this is why I am a bit puzzled.
> >
> > Relooking at the disable_libs option current implementation, it seems
> > backward to pass a disable_libs option when you want to build some
> > deprecated library.
> > It is more straightforward to request building libraries via
> > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > implicitly.
> >
> > But again, we may be breaking something for people who relied on this behavior.
> >
>
> That's what I expected, and I think that is ok. I just wanted to check that
> the change in behaviour was only for the deprecated libs case.
Thomas, wdyt?
It requires some release note, at least.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 8:38 ` David Marchand
@ 2023-06-20 8:44 ` Bruce Richardson
2023-06-20 8:48 ` David Marchand
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-20 8:44 UTC (permalink / raw)
To: David Marchand
Cc: thomas, dev, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Tue, Jun 20, 2023 at 10:38:33AM +0200, David Marchand wrote:
> On Tue, Jun 20, 2023 at 10:36 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > On Tue, Jun 20, 2023 at 10:31:19AM +0200, David Marchand wrote:
> > > On Mon, Jun 19, 2023 at 4:26 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Mon, Jun 19, 2023 at 04:11:37PM +0200, David Marchand wrote:
> > > > > On Fri, Jun 16, 2023 at 9:21 AM David Marchand
> > > > > <david.marchand@redhat.com> wrote:
> > > > > > @@ -141,13 +137,25 @@ foreach l:libraries
> > > > > > deps += ['eal']
> > > > > > endif
> > > > > >
> > > > > > - if disabled_libs.contains(l)
> > > > > > + if not enable_libs.contains(l)
> > > > > > + build = false
> > > > > > + reason = 'not in enabled libraries build config'
> > > > > > + elif disable_libs.contains(l)
> > > > > > build = false
> > > > > > reason = 'explicitly disabled via build config'
> > > > > > - if dpdk_libs_deprecated.contains(l)
> > > > > > + endif
> > > > >
> > > > > There is also a change in behavior for current users of the
> > > > > -Ddisable_libs= configuration (which was used for enabling deprecated
> > > > > libraries, for example).
> > > >
> > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > any other change in behaviour for current users?
> > >
> > > The only change I see, is that this implementation breaks enabling
> > > deprecated libs via disable_libs.
> > > It may break existing developer build directory and maybe some
> > > packaging scripts, this is why I am a bit puzzled.
> > >
> > > Relooking at the disable_libs option current implementation, it seems
> > > backward to pass a disable_libs option when you want to build some
> > > deprecated library.
> > > It is more straightforward to request building libraries via
> > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > implicitly.
> > >
> > > But again, we may be breaking something for people who relied on this behavior.
> > >
> >
> > That's what I expected, and I think that is ok. I just wanted to check that
> > the change in behaviour was only for the deprecated libs case.
>
> Thomas, wdyt?
> It requires some release note, at least.
>
I am assuming this is not targetting this release though, right? Assuming
23.11, we can put in a deprecation note informing of the change ahead of
time too.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 8:44 ` Bruce Richardson
@ 2023-06-20 8:48 ` David Marchand
2023-06-20 9:03 ` Bruce Richardson
0 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-20 8:48 UTC (permalink / raw)
To: Bruce Richardson
Cc: thomas, dev, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > any other change in behaviour for current users?
> > > >
> > > > The only change I see, is that this implementation breaks enabling
> > > > deprecated libs via disable_libs.
> > > > It may break existing developer build directory and maybe some
> > > > packaging scripts, this is why I am a bit puzzled.
> > > >
> > > > Relooking at the disable_libs option current implementation, it seems
> > > > backward to pass a disable_libs option when you want to build some
> > > > deprecated library.
> > > > It is more straightforward to request building libraries via
> > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > implicitly.
> > > >
> > > > But again, we may be breaking something for people who relied on this behavior.
> > > >
> > >
> > > That's what I expected, and I think that is ok. I just wanted to check that
> > > the change in behaviour was only for the deprecated libs case.
> >
> > Thomas, wdyt?
> > It requires some release note, at least.
> >
> I am assuming this is not targetting this release though, right? Assuming
> 23.11, we can put in a deprecation note informing of the change ahead of
> time too.
I was hoping to get it in this release.
But I am fine with postponing and announcing the change beforehand.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 8:48 ` David Marchand
@ 2023-06-20 9:03 ` Bruce Richardson
2023-06-20 14:33 ` Thomas Monjalon
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-20 9:03 UTC (permalink / raw)
To: David Marchand
Cc: thomas, dev, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > any other change in behaviour for current users?
> > > > >
> > > > > The only change I see, is that this implementation breaks enabling
> > > > > deprecated libs via disable_libs.
> > > > > It may break existing developer build directory and maybe some
> > > > > packaging scripts, this is why I am a bit puzzled.
> > > > >
> > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > backward to pass a disable_libs option when you want to build some
> > > > > deprecated library.
> > > > > It is more straightforward to request building libraries via
> > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > implicitly.
> > > > >
> > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > >
> > > >
> > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > the change in behaviour was only for the deprecated libs case.
> > >
> > > Thomas, wdyt?
> > > It requires some release note, at least.
> > >
> > I am assuming this is not targetting this release though, right? Assuming
> > 23.11, we can put in a deprecation note informing of the change ahead of
> > time too.
>
> I was hoping to get it in this release.
> But I am fine with postponing and announcing the change beforehand.
>
Given the fact that we are likely changing behaviour, and the fact that the
deprecated libs makes it more complicated than the drivers one (since we
have always on, default on and default off cases to consider), I think it's
best we don't rush this.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 9:03 ` Bruce Richardson
@ 2023-06-20 14:33 ` Thomas Monjalon
2023-06-20 14:40 ` Bruce Richardson
2023-06-20 15:01 ` Bruce Richardson
0 siblings, 2 replies; 70+ messages in thread
From: Thomas Monjalon @ 2023-06-20 14:33 UTC (permalink / raw)
To: David Marchand, Bruce Richardson
Cc: dev, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Aaron Conole, Michael Santana
20/06/2023 11:03, Bruce Richardson:
> On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > any other change in behaviour for current users?
> > > > > >
> > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > deprecated libs via disable_libs.
> > > > > > It may break existing developer build directory and maybe some
> > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > >
> > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > deprecated library.
> > > > > > It is more straightforward to request building libraries via
> > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > implicitly.
> > > > > >
> > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > >
> > > > >
> > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > the change in behaviour was only for the deprecated libs case.
> > > >
> > > > Thomas, wdyt?
> > > > It requires some release note, at least.
> > > >
> > > I am assuming this is not targetting this release though, right? Assuming
> > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > time too.
> >
> > I was hoping to get it in this release.
> > But I am fine with postponing and announcing the change beforehand.
> >
> Given the fact that we are likely changing behaviour, and the fact that the
> deprecated libs makes it more complicated than the drivers one (since we
> have always on, default on and default off cases to consider), I think it's
> best we don't rush this.
I'm not sure what is the best behaviour.
I tend to think such options should be simple to understand
with only 3 cases:
- no option -> default
- enable option -> only core mandatory and listed libraries
- disable option -> all but the listed libraries
It looks simpler to forbid having both enable and disable libraries.
Would you be open to change the behaviour of the drivers options?
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 14:33 ` Thomas Monjalon
@ 2023-06-20 14:40 ` Bruce Richardson
2023-06-20 15:01 ` Bruce Richardson
1 sibling, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-06-20 14:40 UTC (permalink / raw)
To: Thomas Monjalon
Cc: David Marchand, dev, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> 20/06/2023 11:03, Bruce Richardson:
> > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > any other change in behaviour for current users?
> > > > > > >
> > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > deprecated libs via disable_libs.
> > > > > > > It may break existing developer build directory and maybe some
> > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > >
> > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > deprecated library.
> > > > > > > It is more straightforward to request building libraries via
> > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > implicitly.
> > > > > > >
> > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > >
> > > > > >
> > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > the change in behaviour was only for the deprecated libs case.
> > > > >
> > > > > Thomas, wdyt?
> > > > > It requires some release note, at least.
> > > > >
> > > > I am assuming this is not targetting this release though, right? Assuming
> > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > time too.
> > >
> > > I was hoping to get it in this release.
> > > But I am fine with postponing and announcing the change beforehand.
> > >
> > Given the fact that we are likely changing behaviour, and the fact that the
> > deprecated libs makes it more complicated than the drivers one (since we
> > have always on, default on and default off cases to consider), I think it's
> > best we don't rush this.
>
> I'm not sure what is the best behaviour.
> I tend to think such options should be simple to understand
> with only 3 cases:
> - no option -> default
> - enable option -> only core mandatory and listed libraries
> - disable option -> all but the listed libraries
> It looks simpler to forbid having both enable and disable libraries.
>
> Would you be open to change the behaviour of the drivers options?
>
Yes, no issue there. I don't see a problem with disallowing having both
enable and disable options set.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 14:33 ` Thomas Monjalon
2023-06-20 14:40 ` Bruce Richardson
@ 2023-06-20 15:01 ` Bruce Richardson
2023-06-21 9:54 ` David Marchand
1 sibling, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-20 15:01 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: David Marchand, dev, bluca
On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> 20/06/2023 11:03, Bruce Richardson:
> > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > any other change in behaviour for current users?
> > > > > > >
> > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > deprecated libs via disable_libs.
> > > > > > > It may break existing developer build directory and maybe some
> > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > >
> > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > deprecated library.
> > > > > > > It is more straightforward to request building libraries via
> > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > implicitly.
> > > > > > >
> > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > >
> > > > > >
> > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > the change in behaviour was only for the deprecated libs case.
> > > > >
> > > > > Thomas, wdyt?
> > > > > It requires some release note, at least.
> > > > >
> > > > I am assuming this is not targetting this release though, right? Assuming
> > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > time too.
> > >
> > > I was hoping to get it in this release.
> > > But I am fine with postponing and announcing the change beforehand.
> > >
> > Given the fact that we are likely changing behaviour, and the fact that the
> > deprecated libs makes it more complicated than the drivers one (since we
> > have always on, default on and default off cases to consider), I think it's
> > best we don't rush this.
>
> I'm not sure what is the best behaviour.
> I tend to think such options should be simple to understand
> with only 3 cases:
> - no option -> default
> - enable option -> only core mandatory and listed libraries
> - disable option -> all but the listed libraries
> It looks simpler to forbid having both enable and disable libraries.
>
> Would you be open to change the behaviour of the drivers options?
>
[reducing CC list a bit]
As a further follow-up, I really think we need to move slowly and more
carefully on this. While I can see the simplicity in disallowing the two
options to be specified, depending on how we go about choosing the
enabling of the deprecated libs, we may want to keep support for allowing
both.
Specifically, my current concern/thinking is:
* David points out that using the "disabled_libs" options may not make the
most logic sense for *enabling* deprecated libs.
* While that is true, I think the usability of enabling them via
"enabled_libs" could be pretty terrible - unless we start adding more
complications. Specifically, if someone wants to just enable KNI in the
build using "enable_libs", specifying "-Denable_libs=kni" will not do
what they want - sure it will enable kni, but will disable dozens of
other parts of DPDK.
* Therefore, I think keeping the disabling of deprecated parts of DPDK via
disabled_libs is easier on users - though agreed it is slightly less
logical. However, if we forbid using enabled and disabled options
together, that would mean that to switch to enabled libs style, the user
has to set both enabled libs, *and* clear the default disabled libs option
of the deprecated ones.
* Therefore, right now I'm tending more towards something like the status
quo - disabling deprecated via disable option, but allowing both enable
and disable options together. This hasn't caused us issues with drivers
thus far, so I'd be hopeful for using it for libs.
The other alternative, is we come up with:
* another scheme for managing deprecated libs
* a special keyword for enabled libs to cover the default case, that one
can use to add on the deprecated libs, without having to call out each
and every other optional library.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-20 15:01 ` Bruce Richardson
@ 2023-06-21 9:54 ` David Marchand
2023-06-21 10:49 ` Bruce Richardson
2023-06-22 9:27 ` Juraj Linkeš
0 siblings, 2 replies; 70+ messages in thread
From: David Marchand @ 2023-06-21 9:54 UTC (permalink / raw)
To: Bruce Richardson, Juraj Linkeš; +Cc: Thomas Monjalon, dev, bluca
On Tue, Jun 20, 2023 at 5:05 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> > 20/06/2023 11:03, Bruce Richardson:
> > > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > > any other change in behaviour for current users?
> > > > > > > >
> > > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > > deprecated libs via disable_libs.
> > > > > > > > It may break existing developer build directory and maybe some
> > > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > > >
> > > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > > deprecated library.
> > > > > > > > It is more straightforward to request building libraries via
> > > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > > implicitly.
> > > > > > > >
> > > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > > >
> > > > > > >
> > > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > > the change in behaviour was only for the deprecated libs case.
> > > > > >
> > > > > > Thomas, wdyt?
> > > > > > It requires some release note, at least.
> > > > > >
> > > > > I am assuming this is not targetting this release though, right? Assuming
> > > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > > time too.
> > > >
> > > > I was hoping to get it in this release.
> > > > But I am fine with postponing and announcing the change beforehand.
> > > >
> > > Given the fact that we are likely changing behaviour, and the fact that the
> > > deprecated libs makes it more complicated than the drivers one (since we
> > > have always on, default on and default off cases to consider), I think it's
> > > best we don't rush this.
> >
> > I'm not sure what is the best behaviour.
> > I tend to think such options should be simple to understand
> > with only 3 cases:
> > - no option -> default
> > - enable option -> only core mandatory and listed libraries
> > - disable option -> all but the listed libraries
> > It looks simpler to forbid having both enable and disable libraries.
> >
> > Would you be open to change the behaviour of the drivers options?
> >
>
> [reducing CC list a bit]
>
> As a further follow-up, I really think we need to move slowly and more
> carefully on this. While I can see the simplicity in disallowing the two
> options to be specified, depending on how we go about choosing the
> enabling of the deprecated libs, we may want to keep support for allowing
> both.
I agree, we need more time on this topic, sorry for being pushy earlier :-).
>
> Specifically, my current concern/thinking is:
> * David points out that using the "disabled_libs" options may not make the
> most logic sense for *enabling* deprecated libs.
> * While that is true, I think the usability of enabling them via
> "enabled_libs" could be pretty terrible - unless we start adding more
> complications. Specifically, if someone wants to just enable KNI in the
> build using "enable_libs", specifying "-Denable_libs=kni" will not do
> what they want - sure it will enable kni, but will disable dozens of
> other parts of DPDK.
> * Therefore, I think keeping the disabling of deprecated parts of DPDK via
> disabled_libs is easier on users - though agreed it is slightly less
> logical. However, if we forbid using enabled and disabled options
> together, that would mean that to switch to enabled libs style, the user
> has to set both enabled libs, *and* clear the default disabled libs option
> of the deprecated ones.
> * Therefore, right now I'm tending more towards something like the status
> quo - disabling deprecated via disable option, but allowing both enable
> and disable options together. This hasn't caused us issues with drivers
> thus far, so I'd be hopeful for using it for libs.
Mixing enable/disable_drivers has been possible since the introduction
of enable_drivers.
Looking at the code, it is unclear the intention was to make them exclusive.
Is there no usecase for using both options in some soc configuration?
Juraj, do you have an opinion?
>
> The other alternative, is we come up with:
> * another scheme for managing deprecated libs
> * a special keyword for enabled libs to cover the default case, that one
> can use to add on the deprecated libs, without having to call out each
> and every other optional library.
I think a separate option to control deprecated libraries would be better.
This option would control whether the deprecation libs are part of the
optional libraries list.
The optional libraries list would then be evaluated according to
enable/disable_libs.
I'll try this to see how it behaves.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-21 9:54 ` David Marchand
@ 2023-06-21 10:49 ` Bruce Richardson
2023-06-21 11:35 ` Morten Brørup
2023-06-22 9:27 ` Juraj Linkeš
1 sibling, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-21 10:49 UTC (permalink / raw)
To: David Marchand; +Cc: Juraj Linkeš, Thomas Monjalon, dev, bluca
On Wed, Jun 21, 2023 at 11:54:31AM +0200, David Marchand wrote:
> On Tue, Jun 20, 2023 at 5:05 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> > > 20/06/2023 11:03, Bruce Richardson:
> > > > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > > > I notice the change in behaviour for enabling the deprecated libs. Is there
> > > > > > > > > > any other change in behaviour for current users?
> > > > > > > > >
> > > > > > > > > The only change I see, is that this implementation breaks enabling
> > > > > > > > > deprecated libs via disable_libs.
> > > > > > > > > It may break existing developer build directory and maybe some
> > > > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > > > >
> > > > > > > > > Relooking at the disable_libs option current implementation, it seems
> > > > > > > > > backward to pass a disable_libs option when you want to build some
> > > > > > > > > deprecated library.
> > > > > > > > > It is more straightforward to request building libraries via
> > > > > > > > > -Denable_libs=<deprecated_lib> explicitly or -Denable_libs=*
> > > > > > > > > implicitly.
> > > > > > > > >
> > > > > > > > > But again, we may be breaking something for people who relied on this behavior.
> > > > > > > > >
> > > > > > > >
> > > > > > > > That's what I expected, and I think that is ok. I just wanted to check that
> > > > > > > > the change in behaviour was only for the deprecated libs case.
> > > > > > >
> > > > > > > Thomas, wdyt?
> > > > > > > It requires some release note, at least.
> > > > > > >
> > > > > > I am assuming this is not targetting this release though, right? Assuming
> > > > > > 23.11, we can put in a deprecation note informing of the change ahead of
> > > > > > time too.
> > > > >
> > > > > I was hoping to get it in this release.
> > > > > But I am fine with postponing and announcing the change beforehand.
> > > > >
> > > > Given the fact that we are likely changing behaviour, and the fact that the
> > > > deprecated libs makes it more complicated than the drivers one (since we
> > > > have always on, default on and default off cases to consider), I think it's
> > > > best we don't rush this.
> > >
> > > I'm not sure what is the best behaviour.
> > > I tend to think such options should be simple to understand
> > > with only 3 cases:
> > > - no option -> default
> > > - enable option -> only core mandatory and listed libraries
> > > - disable option -> all but the listed libraries
> > > It looks simpler to forbid having both enable and disable libraries.
> > >
> > > Would you be open to change the behaviour of the drivers options?
> > >
> >
> > [reducing CC list a bit]
> >
> > As a further follow-up, I really think we need to move slowly and more
> > carefully on this. While I can see the simplicity in disallowing the two
> > options to be specified, depending on how we go about choosing the
> > enabling of the deprecated libs, we may want to keep support for allowing
> > both.
>
> I agree, we need more time on this topic, sorry for being pushy earlier :-).
>
No problem at all. I too was keen to get this done, but what initially we
thought was simple turns out (as usual!) to have hidden complexity. :-)
>
> >
> > Specifically, my current concern/thinking is:
> > * David points out that using the "disabled_libs" options may not make the
> > most logic sense for *enabling* deprecated libs.
> > * While that is true, I think the usability of enabling them via
> > "enabled_libs" could be pretty terrible - unless we start adding more
> > complications. Specifically, if someone wants to just enable KNI in the
> > build using "enable_libs", specifying "-Denable_libs=kni" will not do
> > what they want - sure it will enable kni, but will disable dozens of
> > other parts of DPDK.
> > * Therefore, I think keeping the disabling of deprecated parts of DPDK via
> > disabled_libs is easier on users - though agreed it is slightly less
> > logical. However, if we forbid using enabled and disabled options
> > together, that would mean that to switch to enabled libs style, the user
> > has to set both enabled libs, *and* clear the default disabled libs option
> > of the deprecated ones.
> > * Therefore, right now I'm tending more towards something like the status
> > quo - disabling deprecated via disable option, but allowing both enable
> > and disable options together. This hasn't caused us issues with drivers
> > thus far, so I'd be hopeful for using it for libs.
>
> Mixing enable/disable_drivers has been possible since the introduction
> of enable_drivers.
> Looking at the code, it is unclear the intention was to make them exclusive.
> Is there no usecase for using both options in some soc configuration?
If we can solve the deprecated libs management separately, having them
exclusive may well make sense. Since we can wildcard, I think the cases
where we need both simultaneously should be few.
> Juraj, do you have an opinion?
>
>
> >
> > The other alternative, is we come up with:
> > * another scheme for managing deprecated libs
> > * a special keyword for enabled libs to cover the default case, that one
> > can use to add on the deprecated libs, without having to call out each
> > and every other optional library.
>
> I think a separate option to control deprecated libraries would be better.
> This option would control whether the deprecation libs are part of the
> optional libraries list.
> The optional libraries list would then be evaluated according to
> enable/disable_libs.
>
> I'll try this to see how it behaves.
>
Ok, let's see how that goes. It does likely mean another build config flag
though, which I was hoping to avoid, but if it simplifies everything else
it may be worth it.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH v3] build: select optional libraries
2023-06-21 10:49 ` Bruce Richardson
@ 2023-06-21 11:35 ` Morten Brørup
0 siblings, 0 replies; 70+ messages in thread
From: Morten Brørup @ 2023-06-21 11:35 UTC (permalink / raw)
To: Bruce Richardson, David Marchand
Cc: Juraj Linkeš, Thomas Monjalon, dev, bluca
I haven't followed this discussion in detail, so this might have been mentioned already...
Omitting deprecated libs when building will not omit deprecated functions in other libs.
In other words: Omitting deprecated libs only achieves part of what we really want to achieve (omitting everything deprecated), so special handling of deprecated libs might not be that important.
PS: I appreciate the effort to make the libs optional at build time, like the drivers!
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 0/4] Select optional libraries
2021-11-17 11:28 ` [PATCH v2 5/5] build: select optional libraries David Marchand
2023-06-16 7:14 ` [PATCH v3] " David Marchand
@ 2023-06-21 17:00 ` David Marchand
2023-06-21 17:00 ` [PATCH v4 1/4] kni: move IOVA build check David Marchand
` (4 more replies)
2023-06-28 13:20 ` [PATCH v5 0/2] " David Marchand
2 siblings, 5 replies; 70+ messages in thread
From: David Marchand @ 2023-06-21 17:00 UTC (permalink / raw)
To: dev; +Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
This series is one implementation to try and please users who want to
select more easily which parts of DPDK are built.
It introduces a change in behavior for enabling deprecated libraries:
this series is aimed at the next release but sent early as a demo of
what changes are required.
If no strong opposition is met, a deprecation notice will be sent for
v23.07 to announce this change in v23.11.
Changes since v3:
- split kni cleanup,
- split variable rename cleanup,
- introduced a new meson option to control deprecated libraries
activation,
- simplified the actual implementation of enable_libs to mimic
enable_drivers behavior,
--
David Marchand
David Marchand (4):
kni: move IOVA build check
build: rename enabled libraries list
build: select deprecated libraries
build: select optional libraries
.ci/linux-build.sh | 2 +-
app/test/meson.build | 2 +-
buildtools/chkincs/meson.build | 2 +-
devtools/test-meson-builds.sh | 8 +++---
lib/kni/meson.build | 5 ++++
lib/meson.build | 45 ++++++++++++++++++++++------------
meson.build | 3 ++-
meson_options.txt | 8 ++++--
8 files changed, 49 insertions(+), 26 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 1/4] kni: move IOVA build check
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
@ 2023-06-21 17:00 ` David Marchand
2023-06-22 8:37 ` Bruce Richardson
2023-06-21 17:00 ` [PATCH v4 2/4] build: rename enabled libraries list David Marchand
` (3 subsequent siblings)
4 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-21 17:00 UTC (permalink / raw)
To: dev; +Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
kni dependency to IOVA configuration does not need to be expressed in
the top level lib/meson.build file.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
lib/kni/meson.build | 5 +++++
lib/meson.build | 3 ---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/lib/kni/meson.build b/lib/kni/meson.build
index 8a71d8ba6f..5ce410f7f2 100644
--- a/lib/kni/meson.build
+++ b/lib/kni/meson.build
@@ -7,6 +7,11 @@ if is_windows
subdir_done()
endif
+if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
+ build = false
+ reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'
+endif
+
if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
build = false
reason = 'only supported on 64-bit Linux'
diff --git a/lib/meson.build b/lib/meson.build
index 9677239236..f5c8a70a1d 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -95,9 +95,6 @@ dpdk_libs_deprecated += [
disabled_libs = []
opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
check: true).stdout().split()
-if dpdk_conf.get('RTE_IOVA_IN_MBUF') == 0
- opt_disabled_libs += ['kni']
-endif
foreach l:opt_disabled_libs
if not optional_libs.contains(l)
warning('Cannot disable mandatory library "@0@"'.format(l))
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 2/4] build: rename enabled libraries list
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
2023-06-21 17:00 ` [PATCH v4 1/4] kni: move IOVA build check David Marchand
@ 2023-06-21 17:00 ` David Marchand
2023-06-22 8:38 ` Bruce Richardson
2023-06-21 17:00 ` [PATCH v4 3/4] build: select deprecated libraries David Marchand
` (2 subsequent siblings)
4 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-21 17:00 UTC (permalink / raw)
To: dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Bruce Richardson
The enabled_libs variable is renamed and moved to the top level
meson.build file, for sake of consistency wrt to drivers and
applications similar variables.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/meson.build | 2 +-
buildtools/chkincs/meson.build | 2 +-
lib/meson.build | 4 +---
meson.build | 3 ++-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/app/test/meson.build b/app/test/meson.build
index d0fabcbb8b..f3217ae577 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -152,7 +152,7 @@ test_sources = files(
'virtual_pmd.c',
)
-test_deps = enabled_libs
+test_deps = dpdk_libs_enabled
# as well as libs, the pci and vdev bus drivers are needed for a lot of tests
test_deps += ['bus_pci', 'bus_vdev']
diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 378c2f19ef..f2dadcae18 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -22,7 +22,7 @@ sources += gen_c_files.process(dpdk_chkinc_headers)
# so we always include them in deps list
deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
# add the rest of the libs to the dependencies
-foreach l:enabled_libs
+foreach l:dpdk_libs_enabled
deps += get_variable('shared_rte_' + l)
endforeach
diff --git a/lib/meson.build b/lib/meson.build
index f5c8a70a1d..363a4fd79f 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -112,8 +112,6 @@ if cc.has_argument('-Wno-format-truncation')
default_cflags += '-Wno-format-truncation'
endif
-enabled_libs = [] # used to print summary at the end
-
foreach l:libraries
build = true
reason = '<unknown reason>' # set if build == false to explain why
@@ -180,7 +178,7 @@ foreach l:libraries
continue
endif
- enabled_libs += name
+ dpdk_libs_enabled += name
dpdk_conf.set('RTE_LIB_' + name.to_upper(), 1)
install_headers(headers)
install_headers(indirect_headers)
diff --git a/meson.build b/meson.build
index 992ca91e88..39cb73846d 100644
--- a/meson.build
+++ b/meson.build
@@ -44,6 +44,7 @@ dpdk_extra_ldflags = []
dpdk_libs_deprecated = []
dpdk_apps_disabled = []
dpdk_libs_disabled = []
+dpdk_libs_enabled = []
dpdk_drvs_disabled = []
testpmd_drivers_sources = []
testpmd_drivers_deps = []
@@ -134,7 +135,7 @@ message(output_message + '\n')
output_message = '\n=================\nLibraries Enabled\n=================\n'
output_message += '\nlibs:\n\t'
output_count = 0
-foreach lib:enabled_libs
+foreach lib:dpdk_libs_enabled
output_message += lib + ', '
output_count += 1
if output_count == 8
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 3/4] build: select deprecated libraries
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
2023-06-21 17:00 ` [PATCH v4 1/4] kni: move IOVA build check David Marchand
2023-06-21 17:00 ` [PATCH v4 2/4] build: rename enabled libraries list David Marchand
@ 2023-06-21 17:00 ` David Marchand
2023-06-22 8:43 ` Bruce Richardson
2023-06-21 17:00 ` [PATCH v4 4/4] build: select optional libraries David Marchand
2023-06-22 9:09 ` [PATCH v4 0/4] Select " Bruce Richardson
4 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-21 17:00 UTC (permalink / raw)
To: dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Aaron Conole, Michael Santana, Bruce Richardson
Rework deprecated libraries selection by introducing a new configuration
option.
This breaks existing configurations that were relying on disable_libs=''
for enabling deprecated libraries.
On the other hand, it will make enabling optional libraries more
straightforward by taking the deprecated libraries out of the picture.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
.ci/linux-build.sh | 2 +-
devtools/test-meson-builds.sh | 8 ++++----
lib/meson.build | 9 +++++++--
meson_options.txt | 4 +++-
4 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 9631e342b5..da51f8e55e 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -97,7 +97,7 @@ if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
else
- OPTS="$OPTS -Ddisable_libs="
+ OPTS="$OPTS -Denable_deprecated_libs=true"
fi
if [ "$ASAN" = "true" ]; then
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 9131088c9d..efeddb36f3 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -120,10 +120,10 @@ config () # <dir> <builddir> <meson options>
return
fi
options=
- # deprecated libs may be disabled by default, so for complete builds ensure
- # no libs are disabled
- if ! echo $* | grep -q -- 'disable_libs' ; then
- options="$options -Ddisable_libs="
+ # deprecated libs are disabled by default, so for complete builds
+ # enable them
+ if ! echo $* | grep -q -- 'enable_deprecated_libs' ; then
+ options="$options -Denable_deprecated_libs=true"
fi
if echo $* | grep -qw -- '--default-library=shared' ; then
options="$options -Dexamples=all"
diff --git a/lib/meson.build b/lib/meson.build
index 363a4fd79f..a6e9e19b42 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -136,13 +136,18 @@ foreach l:libraries
deps += ['eal']
endif
- if disabled_libs.contains(l)
+ if dpdk_libs_deprecated.contains(l) and not get_option('enable_deprecated_libs')
+ build = false
+ reason = 'deprecated libraries disabled via build config'
+ elif disabled_libs.contains(l)
build = false
reason = 'explicitly disabled via build config'
if dpdk_libs_deprecated.contains(l)
reason += ' (deprecated lib)'
endif
- else
+ endif
+
+ if build
if dpdk_libs_deprecated.contains(l)
warning('Enabling deprecated library, "@0@"'.format(l))
endif
diff --git a/meson_options.txt b/meson_options.txt
index 82c8297065..f959015e46 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', description:
'Comma-separated list of apps to explicitly disable.')
option('disable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to explicitly disable.')
-option('disable_libs', type: 'string', value: 'flow_classify,kni', description:
+option('disable_libs', type: 'string', value: '', description:
'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
@@ -18,6 +18,8 @@ option('enable_docs', type: 'boolean', value: false, description:
'build documentation')
option('enable_apps', type: 'string', value: '', description:
'Comma-separated list of apps to build. If unspecified, build all apps.')
+option('enable_deprecated_libs', type: 'boolean', value: false, description:
+ 'Add deprecated libraries to the list of optional libraries to build')
option('enable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to build. If unspecified, build all drivers.')
option('enable_driver_sdk', type: 'boolean', value: false, description:
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 4/4] build: select optional libraries
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
` (2 preceding siblings ...)
2023-06-21 17:00 ` [PATCH v4 3/4] build: select deprecated libraries David Marchand
@ 2023-06-21 17:00 ` David Marchand
2023-06-22 8:49 ` Bruce Richardson
2023-06-22 9:09 ` [PATCH v4 0/4] Select " Bruce Richardson
4 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-21 17:00 UTC (permalink / raw)
To: dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Bruce Richardson
There is currently no way to know which libraries are optional.
Introduce a enable_libs option (close to what we have for drivers) so
that packagers or projects consuming DPDK can more easily select the
optional libraries that matter to them and disable other optional
libraries.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v3:
- split non related cleanup changes,
- fixed false positive complaints about disabling mandatory libs by
building the list of always enabled libs, appending this list to
enable_list and checking the disable side,
- updated enable/disable_libs descriptions,
Changes since v2:
- moved the IOVA check for kni build in lib/kni/meson.build,
- reworked deprecated libraries handling by only considering them when
no enable_libs option is set. With this, no need for a default value
in meson_options.txt, yet configurations in the CI must be adjusted,
- moved mandatory libraries check after enable/disable_libs evaluation,
---
lib/meson.build | 33 +++++++++++++++++++++++----------
meson_options.txt | 4 +++-
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/lib/meson.build b/lib/meson.build
index a6e9e19b42..407769b270 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -92,17 +92,20 @@ dpdk_libs_deprecated += [
'kni',
]
-disabled_libs = []
-opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
- check: true).stdout().split()
-foreach l:opt_disabled_libs
+disable_libs = run_command(list_dir_globs, get_option('disable_libs'), check: true).stdout().split()
+
+enable_libs = run_command(list_dir_globs, get_option('enable_libs'), check: true).stdout().split()
+if enable_libs.length() == 0
+ enable_libs += optional_libs
+endif
+
+always_enable = []
+foreach l:libraries
if not optional_libs.contains(l)
- warning('Cannot disable mandatory library "@0@"'.format(l))
- continue
+ always_enable += l
endif
- disabled_libs += l
endforeach
-
+enable_libs += always_enable
default_cflags = machine_args
default_cflags += ['-DALLOW_EXPERIMENTAL_API']
@@ -139,12 +142,22 @@ foreach l:libraries
if dpdk_libs_deprecated.contains(l) and not get_option('enable_deprecated_libs')
build = false
reason = 'deprecated libraries disabled via build config'
- elif disabled_libs.contains(l)
+ elif not enable_libs.contains(l)
build = false
- reason = 'explicitly disabled via build config'
+ reason = 'not in enabled libraries build config'
if dpdk_libs_deprecated.contains(l)
reason += ' (deprecated lib)'
endif
+ elif disable_libs.contains(l)
+ if always_enable.contains(l)
+ warning('Cannot disable mandatory library "@0@"'.format(l))
+ else
+ build = false
+ reason = 'explicitly disabled via build config'
+ if dpdk_libs_deprecated.contains(l)
+ reason += ' (deprecated lib)'
+ endif
+ endif
endif
if build
diff --git a/meson_options.txt b/meson_options.txt
index f959015e46..9951563146 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -11,7 +11,7 @@ option('disable_apps', type: 'string', value: '', description:
option('disable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to explicitly disable.')
option('disable_libs', type: 'string', value: '', description:
- 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
+ 'Comma-separated list of optional libraries to explicitly disable. [NOTE: mandatory libs cannot be disabled]')
option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
option('enable_docs', type: 'boolean', value: false, description:
@@ -26,6 +26,8 @@ option('enable_driver_sdk', type: 'boolean', value: false, description:
'Install headers to build drivers.')
option('enable_kmods', type: 'boolean', value: false, description:
'build kernel modules')
+option('enable_libs', type: 'string', value: '', description:
+ 'Comma-separated list of optional libraries to explicitly enable. [NOTE: mandatory libs are always enabled]')
option('examples', type: 'string', value: '', description:
'Comma-separated list of examples to build by default')
option('ibverbs_link', type: 'combo', choices : ['static', 'shared', 'dlopen'], value: 'shared', description:
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 1/4] kni: move IOVA build check
2023-06-21 17:00 ` [PATCH v4 1/4] kni: move IOVA build check David Marchand
@ 2023-06-22 8:37 ` Bruce Richardson
0 siblings, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-06-22 8:37 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
On Wed, Jun 21, 2023 at 07:00:55PM +0200, David Marchand wrote:
> kni dependency to IOVA configuration does not need to be expressed in
> the top level lib/meson.build file.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> lib/kni/meson.build | 5 +++++
> lib/meson.build | 3 ---
> 2 files changed, 5 insertions(+), 3 deletions(-)
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
even if the rest of the set gets pushed to next release, I think this patch
should go in this one anyway. It's a simple cleanup.
Thanks.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/4] build: rename enabled libraries list
2023-06-21 17:00 ` [PATCH v4 2/4] build: rename enabled libraries list David Marchand
@ 2023-06-22 8:38 ` Bruce Richardson
0 siblings, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-06-22 8:38 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
On Wed, Jun 21, 2023 at 07:00:56PM +0200, David Marchand wrote:
> The enabled_libs variable is renamed and moved to the top level
> meson.build file, for sake of consistency wrt to drivers and
> applications similar variables.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Again, good cleanup for consistency.
thanks.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 3/4] build: select deprecated libraries
2023-06-21 17:00 ` [PATCH v4 3/4] build: select deprecated libraries David Marchand
@ 2023-06-22 8:43 ` Bruce Richardson
2023-06-22 8:50 ` Bruce Richardson
2023-06-23 9:35 ` David Marchand
0 siblings, 2 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-06-22 8:43 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> Rework deprecated libraries selection by introducing a new configuration
> option.
>
> This breaks existing configurations that were relying on disable_libs=''
> for enabling deprecated libraries.
> On the other hand, it will make enabling optional libraries more
> straightforward by taking the deprecated libraries out of the picture.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
This gives us a single on/off value for the deprecated libs. So if you
wants to build only a single deprecated lib, you need to turn on this
option and then use "disable_libs/enable_libs" option to then selectively
pick which of the deprecated libs you actually want. Is that the expected
behaviour? Just checking that we don't want this to be a list too.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 4/4] build: select optional libraries
2023-06-21 17:00 ` [PATCH v4 4/4] build: select optional libraries David Marchand
@ 2023-06-22 8:49 ` Bruce Richardson
0 siblings, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-06-22 8:49 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
On Wed, Jun 21, 2023 at 07:00:58PM +0200, David Marchand wrote:
> There is currently no way to know which libraries are optional.
> Introduce a enable_libs option (close to what we have for drivers) so
> that packagers or projects consuming DPDK can more easily select the
> optional libraries that matter to them and disable other optional
> libraries.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
LGTM - one minor suggestion inline below.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Changes since v3:
> - split non related cleanup changes,
> - fixed false positive complaints about disabling mandatory libs by
> building the list of always enabled libs, appending this list to
> enable_list and checking the disable side,
> - updated enable/disable_libs descriptions,
>
> Changes since v2:
> - moved the IOVA check for kni build in lib/kni/meson.build,
> - reworked deprecated libraries handling by only considering them when
> no enable_libs option is set. With this, no need for a default value
> in meson_options.txt, yet configurations in the CI must be adjusted,
> - moved mandatory libraries check after enable/disable_libs evaluation,
>
> ---
> lib/meson.build | 33 +++++++++++++++++++++++----------
> meson_options.txt | 4 +++-
> 2 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/lib/meson.build b/lib/meson.build
> index a6e9e19b42..407769b270 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -92,17 +92,20 @@ dpdk_libs_deprecated += [
> 'kni',
> ]
>
> -disabled_libs = []
> -opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
> - check: true).stdout().split()
> -foreach l:opt_disabled_libs
> +disable_libs = run_command(list_dir_globs, get_option('disable_libs'), check: true).stdout().split()
> +
> +enable_libs = run_command(list_dir_globs, get_option('enable_libs'), check: true).stdout().split()
> +if enable_libs.length() == 0
> + enable_libs += optional_libs
> +endif
> +
> +always_enable = []
> +foreach l:libraries
> if not optional_libs.contains(l)
> - warning('Cannot disable mandatory library "@0@"'.format(l))
> - continue
> + always_enable += l
> endif
> - disabled_libs += l
> endforeach
Minor nit, I'd actually put this block further up the file just after the
optional_libs list. It's more an array definition, and moving it up puts
the next line (below) to add to enable_libs immediately after the other
assignments to enable_libs.
> -
> +enable_libs += always_enable
>
> default_cflags = machine_args
> default_cflags += ['-DALLOW_EXPERIMENTAL_API']
> @@ -139,12 +142,22 @@ foreach l:libraries
> if dpdk_libs_deprecated.contains(l) and not get_option('enable_deprecated_libs')
> build = false
> reason = 'deprecated libraries disabled via build config'
> - elif disabled_libs.contains(l)
> + elif not enable_libs.contains(l)
> build = false
> - reason = 'explicitly disabled via build config'
> + reason = 'not in enabled libraries build config'
> if dpdk_libs_deprecated.contains(l)
> reason += ' (deprecated lib)'
> endif
> + elif disable_libs.contains(l)
> + if always_enable.contains(l)
> + warning('Cannot disable mandatory library "@0@"'.format(l))
> + else
> + build = false
> + reason = 'explicitly disabled via build config'
> + if dpdk_libs_deprecated.contains(l)
> + reason += ' (deprecated lib)'
> + endif
> + endif
> endif
>
> if build
> diff --git a/meson_options.txt b/meson_options.txt
> index f959015e46..9951563146 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -11,7 +11,7 @@ option('disable_apps', type: 'string', value: '', description:
> option('disable_drivers', type: 'string', value: '', description:
> 'Comma-separated list of drivers to explicitly disable.')
> option('disable_libs', type: 'string', value: '', description:
> - 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
> + 'Comma-separated list of optional libraries to explicitly disable. [NOTE: mandatory libs cannot be disabled]')
> option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
> 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
> option('enable_docs', type: 'boolean', value: false, description:
> @@ -26,6 +26,8 @@ option('enable_driver_sdk', type: 'boolean', value: false, description:
> 'Install headers to build drivers.')
> option('enable_kmods', type: 'boolean', value: false, description:
> 'build kernel modules')
> +option('enable_libs', type: 'string', value: '', description:
> + 'Comma-separated list of optional libraries to explicitly enable. [NOTE: mandatory libs are always enabled]')
> option('examples', type: 'string', value: '', description:
> 'Comma-separated list of examples to build by default')
> option('ibverbs_link', type: 'combo', choices : ['static', 'shared', 'dlopen'], value: 'shared', description:
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 3/4] build: select deprecated libraries
2023-06-22 8:43 ` Bruce Richardson
@ 2023-06-22 8:50 ` Bruce Richardson
2023-06-23 9:35 ` David Marchand
1 sibling, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-06-22 8:50 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Thu, Jun 22, 2023 at 09:43:18AM +0100, Bruce Richardson wrote:
> On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> > Rework deprecated libraries selection by introducing a new configuration
> > option.
> >
> > This breaks existing configurations that were relying on disable_libs=''
> > for enabling deprecated libraries.
> > On the other hand, it will make enabling optional libraries more
> > straightforward by taking the deprecated libraries out of the picture.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> This gives us a single on/off value for the deprecated libs. So if you
> wants to build only a single deprecated lib, you need to turn on this
> option and then use "disable_libs/enable_libs" option to then selectively
> pick which of the deprecated libs you actually want. Is that the expected
> behaviour? Just checking that we don't want this to be a list too.
>
The patch is fine itself though, so:
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 0/4] Select optional libraries
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
` (3 preceding siblings ...)
2023-06-21 17:00 ` [PATCH v4 4/4] build: select optional libraries David Marchand
@ 2023-06-22 9:09 ` Bruce Richardson
2023-06-22 16:41 ` Thomas Monjalon
4 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-22 9:09 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
On Wed, Jun 21, 2023 at 07:00:54PM +0200, David Marchand wrote:
> This series is one implementation to try and please users who want to
> select more easily which parts of DPDK are built.
>
> It introduces a change in behavior for enabling deprecated libraries:
> this series is aimed at the next release but sent early as a demo of
> what changes are required.
>
> If no strong opposition is met, a deprecation notice will be sent for
> v23.07 to announce this change in v23.11.
>
>
> Changes since v3:
> - split kni cleanup,
> - split variable rename cleanup,
> - introduced a new meson option to control deprecated libraries
> activation,
> - simplified the actual implementation of enable_libs to mimic
> enable_drivers behavior,
>
>
> --
> David Marchand
>
> David Marchand (4):
> kni: move IOVA build check
> build: rename enabled libraries list
> build: select deprecated libraries
> build: select optional libraries
>
I think the first 2 patches should definitely go into 23.07. The latter two
cause a change in how one needs to build DPDK with libs enabled, so we may
want to push that out, and flag the change in advance. Since it's a
relatively minor change, and a build-time one only, I'm fine with it going
in either 23.07 or 23.11, as maintainers decide.
Series-reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3] build: select optional libraries
2023-06-21 9:54 ` David Marchand
2023-06-21 10:49 ` Bruce Richardson
@ 2023-06-22 9:27 ` Juraj Linkeš
1 sibling, 0 replies; 70+ messages in thread
From: Juraj Linkeš @ 2023-06-22 9:27 UTC (permalink / raw)
To: David Marchand; +Cc: Bruce Richardson, Thomas Monjalon, dev, bluca
[-- Attachment #1: Type: text/plain, Size: 6392 bytes --]
On Wed, Jun 21, 2023 at 11:54 AM David Marchand <david.marchand@redhat.com>
wrote:
> On Tue, Jun 20, 2023 at 5:05 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Jun 20, 2023 at 04:33:15PM +0200, Thomas Monjalon wrote:
> > > 20/06/2023 11:03, Bruce Richardson:
> > > > On Tue, Jun 20, 2023 at 10:48:50AM +0200, David Marchand wrote:
> > > > > On Tue, Jun 20, 2023 at 10:45 AM Bruce Richardson
> > > > > <bruce.richardson@intel.com> wrote:
> > > > > > > > > > I notice the change in behaviour for enabling the
> deprecated libs. Is there
> > > > > > > > > > any other change in behaviour for current users?
> > > > > > > > >
> > > > > > > > > The only change I see, is that this implementation breaks
> enabling
> > > > > > > > > deprecated libs via disable_libs.
> > > > > > > > > It may break existing developer build directory and maybe
> some
> > > > > > > > > packaging scripts, this is why I am a bit puzzled.
> > > > > > > > >
> > > > > > > > > Relooking at the disable_libs option current
> implementation, it seems
> > > > > > > > > backward to pass a disable_libs option when you want to
> build some
> > > > > > > > > deprecated library.
> > > > > > > > > It is more straightforward to request building libraries
> via
> > > > > > > > > -Denable_libs=<deprecated_lib> explicitly or
> -Denable_libs=*
> > > > > > > > > implicitly.
> > > > > > > > >
> > > > > > > > > But again, we may be breaking something for people who
> relied on this behavior.
> > > > > > > > >
> > > > > > > >
> > > > > > > > That's what I expected, and I think that is ok. I just
> wanted to check that
> > > > > > > > the change in behaviour was only for the deprecated libs
> case.
> > > > > > >
> > > > > > > Thomas, wdyt?
> > > > > > > It requires some release note, at least.
> > > > > > >
> > > > > > I am assuming this is not targetting this release though, right?
> Assuming
> > > > > > 23.11, we can put in a deprecation note informing of the change
> ahead of
> > > > > > time too.
> > > > >
> > > > > I was hoping to get it in this release.
> > > > > But I am fine with postponing and announcing the change beforehand.
> > > > >
> > > > Given the fact that we are likely changing behaviour, and the fact
> that the
> > > > deprecated libs makes it more complicated than the drivers one
> (since we
> > > > have always on, default on and default off cases to consider), I
> think it's
> > > > best we don't rush this.
> > >
> > > I'm not sure what is the best behaviour.
> > > I tend to think such options should be simple to understand
> > > with only 3 cases:
> > > - no option -> default
> > > - enable option -> only core mandatory and listed libraries
> > > - disable option -> all but the listed libraries
> > > It looks simpler to forbid having both enable and disable libraries.
> > >
> > > Would you be open to change the behaviour of the drivers options?
> > >
> >
> > [reducing CC list a bit]
> >
> > As a further follow-up, I really think we need to move slowly and more
> > carefully on this. While I can see the simplicity in disallowing the two
> > options to be specified, depending on how we go about choosing the
> > enabling of the deprecated libs, we may want to keep support for allowing
> > both.
>
> I agree, we need more time on this topic, sorry for being pushy earlier
> :-).
>
>
> >
> > Specifically, my current concern/thinking is:
> > * David points out that using the "disabled_libs" options may not make
> the
> > most logic sense for *enabling* deprecated libs.
> > * While that is true, I think the usability of enabling them via
> > "enabled_libs" could be pretty terrible - unless we start adding more
> > complications. Specifically, if someone wants to just enable KNI in the
> > build using "enable_libs", specifying "-Denable_libs=kni" will not do
> > what they want - sure it will enable kni, but will disable dozens of
> > other parts of DPDK.
> > * Therefore, I think keeping the disabling of deprecated parts of DPDK
> via
> > disabled_libs is easier on users - though agreed it is slightly less
> > logical. However, if we forbid using enabled and disabled options
> > together, that would mean that to switch to enabled libs style, the
> user
> > has to set both enabled libs, *and* clear the default disabled libs
> option
> > of the deprecated ones.
> > * Therefore, right now I'm tending more towards something like the status
> > quo - disabling deprecated via disable option, but allowing both enable
> > and disable options together. This hasn't caused us issues with drivers
> > thus far, so I'd be hopeful for using it for libs.
>
> Mixing enable/disable_drivers has been possible since the introduction
> of enable_drivers.
> Looking at the code, it is unclear the intention was to make them
> exclusive.
> Is there no usecase for using both options in some soc configuration?
> Juraj, do you have an opinion?
>
>
I seem to remember that there wasn't any intention of using them together -
I think I did it this way because it was essentially "free" (i.e. the
ability to use them together was an extra feature on top of what we needed).
The SoC considerations were for cross compilation between Arm
microarchitectures/SoCs - use enable_drivers mainly to enable only a
handful of relevant drivers for small SoC's when building on a server grade
SoC. The same applies for disable_drivers - the server grade SoC where
we're building may have extra dependencies that we may not want/need on the
target (possibly server-grade) system. They don't need to be used in
conjunction in these scenarios.
>
> >
> > The other alternative, is we come up with:
> > * another scheme for managing deprecated libs
> > * a special keyword for enabled libs to cover the default case, that one
> > can use to add on the deprecated libs, without having to call out each
> > and every other optional library.
>
> I think a separate option to control deprecated libraries would be better.
> This option would control whether the deprecation libs are part of the
> optional libraries list.
> The optional libraries list would then be evaluated according to
> enable/disable_libs.
>
> I'll try this to see how it behaves.
>
>
> --
> David Marchand
>
>
[-- Attachment #2: Type: text/html, Size: 8227 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 0/4] Select optional libraries
2023-06-22 9:09 ` [PATCH v4 0/4] Select " Bruce Richardson
@ 2023-06-22 16:41 ` Thomas Monjalon
0 siblings, 0 replies; 70+ messages in thread
From: Thomas Monjalon @ 2023-06-22 16:41 UTC (permalink / raw)
To: David Marchand
Cc: dev, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Bruce Richardson
22/06/2023 11:09, Bruce Richardson:
> On Wed, Jun 21, 2023 at 07:00:54PM +0200, David Marchand wrote:
> > This series is one implementation to try and please users who want to
> > select more easily which parts of DPDK are built.
> >
> > It introduces a change in behavior for enabling deprecated libraries:
> > this series is aimed at the next release but sent early as a demo of
> > what changes are required.
> >
> > If no strong opposition is met, a deprecation notice will be sent for
> > v23.07 to announce this change in v23.11.
> >
> >
> > Changes since v3:
> > - split kni cleanup,
> > - split variable rename cleanup,
> > - introduced a new meson option to control deprecated libraries
> > activation,
> > - simplified the actual implementation of enable_libs to mimic
> > enable_drivers behavior,
> >
> >
> I think the first 2 patches should definitely go into 23.07. The latter two
> cause a change in how one needs to build DPDK with libs enabled, so we may
> want to push that out, and flag the change in advance. Since it's a
> relatively minor change, and a build-time one only, I'm fine with it going
> in either 23.07 or 23.11, as maintainers decide.
>
> Series-reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
Applied first 2 patches, thanks.
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 3/4] build: select deprecated libraries
2023-06-22 8:43 ` Bruce Richardson
2023-06-22 8:50 ` Bruce Richardson
@ 2023-06-23 9:35 ` David Marchand
2023-06-23 11:04 ` Bruce Richardson
1 sibling, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-23 9:35 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
Hello Bruce,
On Thu, Jun 22, 2023 at 10:43 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> > Rework deprecated libraries selection by introducing a new configuration
> > option.
> >
> > This breaks existing configurations that were relying on disable_libs=''
> > for enabling deprecated libraries.
> > On the other hand, it will make enabling optional libraries more
> > straightforward by taking the deprecated libraries out of the picture.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> This gives us a single on/off value for the deprecated libs. So if you
> wants to build only a single deprecated lib, you need to turn on this
> option and then use "disable_libs/enable_libs" option to then selectively
> pick which of the deprecated libs you actually want. Is that the expected
> behaviour? Just checking that we don't want this to be a list too.
Yes, I wanted a single unified filtering stage.
But I think your suggestion is easier to use.
- That would make it simpler for people who simply want to enable kni,
as you mentionned before:
$ meson setup plop -Denable_deprecated_libs=kni
But I would make this list not overlap with the disable/enable_libs
options evaluation.
Otherwise, in the case of a enable_libs user, the user would have to
set kni in both lists, which is not that great:
$ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=kni,vhost
Instead, I would make it so the config is done as:
$ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=vhost
Is this what you had in mind?
- I don't have a usage for this, but if we go with separating
deprecated and "normal" optional libs filtering, should I introduce a
disable_deprecated_libs too?
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 3/4] build: select deprecated libraries
2023-06-23 9:35 ` David Marchand
@ 2023-06-23 11:04 ` Bruce Richardson
2023-06-23 11:15 ` Morten Brørup
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-23 11:04 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Fri, Jun 23, 2023 at 11:35:29AM +0200, David Marchand wrote:
> Hello Bruce,
>
> On Thu, Jun 22, 2023 at 10:43 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> > > Rework deprecated libraries selection by introducing a new configuration
> > > option.
> > >
> > > This breaks existing configurations that were relying on disable_libs=''
> > > for enabling deprecated libraries.
> > > On the other hand, it will make enabling optional libraries more
> > > straightforward by taking the deprecated libraries out of the picture.
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> >
> > This gives us a single on/off value for the deprecated libs. So if you
> > wants to build only a single deprecated lib, you need to turn on this
> > option and then use "disable_libs/enable_libs" option to then selectively
> > pick which of the deprecated libs you actually want. Is that the expected
> > behaviour? Just checking that we don't want this to be a list too.
>
> Yes, I wanted a single unified filtering stage.
>
> But I think your suggestion is easier to use.
Slightly easier for the simple case.
>
> - That would make it simpler for people who simply want to enable kni,
> as you mentionned before:
> $ meson setup plop -Denable_deprecated_libs=kni
>
> But I would make this list not overlap with the disable/enable_libs
> options evaluation.
> Otherwise, in the case of a enable_libs user, the user would have to
> set kni in both lists, which is not that great:
> $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=kni,vhost
>
> Instead, I would make it so the config is done as:
> $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=vhost
>
> Is this what you had in mind?
>
I'm not sure myself what I had in mind, just asking if it had been
considered as much as anything else.
Having them not-overlap would seem to be necessary to provide a meaningful
interface.
>
> - I don't have a usage for this, but if we go with separating
> deprecated and "normal" optional libs filtering, should I introduce a
> disable_deprecated_libs too?
>
That would give us *way* to many options. I think for the sake of simplicity
we probably are as well to just go with what you are proposing in this set.
Since we only have two deprecated libraries - and hopefully never many more -
the benefit of the list for that setting is probably minimal. I'm keen to
avoid too much complexity if we can manage it.
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH v4 3/4] build: select deprecated libraries
2023-06-23 11:04 ` Bruce Richardson
@ 2023-06-23 11:15 ` Morten Brørup
2023-06-23 11:19 ` Bruce Richardson
0 siblings, 1 reply; 70+ messages in thread
From: Morten Brørup @ 2023-06-23 11:15 UTC (permalink / raw)
To: Bruce Richardson, David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 23 June 2023 13.04
>
> On Fri, Jun 23, 2023 at 11:35:29AM +0200, David Marchand wrote:
> > Hello Bruce,
> >
> > On Thu, Jun 22, 2023 at 10:43 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> > > > Rework deprecated libraries selection by introducing a new configuration
> > > > option.
> > > >
> > > > This breaks existing configurations that were relying on disable_libs=''
> > > > for enabling deprecated libraries.
> > > > On the other hand, it will make enabling optional libraries more
> > > > straightforward by taking the deprecated libraries out of the picture.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > >
> > > This gives us a single on/off value for the deprecated libs. So if you
> > > wants to build only a single deprecated lib, you need to turn on this
> > > option and then use "disable_libs/enable_libs" option to then selectively
> > > pick which of the deprecated libs you actually want. Is that the expected
> > > behaviour? Just checking that we don't want this to be a list too.
> >
> > Yes, I wanted a single unified filtering stage.
> >
> > But I think your suggestion is easier to use.
>
> Slightly easier for the simple case.
>
> >
> > - That would make it simpler for people who simply want to enable kni,
> > as you mentionned before:
> > $ meson setup plop -Denable_deprecated_libs=kni
> >
> > But I would make this list not overlap with the disable/enable_libs
> > options evaluation.
> > Otherwise, in the case of a enable_libs user, the user would have to
> > set kni in both lists, which is not that great:
> > $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=kni,vhost
> >
> > Instead, I would make it so the config is done as:
> > $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=vhost
> >
> > Is this what you had in mind?
> >
> I'm not sure myself what I had in mind, just asking if it had been
> considered as much as anything else.
>
> Having them not-overlap would seem to be necessary to provide a meaningful
> interface.
>
> >
> > - I don't have a usage for this, but if we go with separating
> > deprecated and "normal" optional libs filtering, should I introduce a
> > disable_deprecated_libs too?
> >
>
> That would give us *way* to many options. I think for the sake of simplicity
> we probably are as well to just go with what you are proposing in this set.
> Since we only have two deprecated libraries - and hopefully never many more -
> the benefit of the list for that setting is probably minimal. I'm keen to
> avoid too much complexity if we can manage it.
I strongly agree with Bruce about avoiding too many options. Here's an idea:
How about just having the disable/enable_libs options, and by default omit the deprecated libs.
Then, the deprecated libs can be enabled by using the enable_libs option.
Do we have special treatment for deprecated drivers?
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 3/4] build: select deprecated libraries
2023-06-23 11:15 ` Morten Brørup
@ 2023-06-23 11:19 ` Bruce Richardson
2023-06-23 11:32 ` Morten Brørup
0 siblings, 1 reply; 70+ messages in thread
From: Bruce Richardson @ 2023-06-23 11:19 UTC (permalink / raw)
To: Morten Brørup
Cc: David Marchand, dev, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
On Fri, Jun 23, 2023 at 01:15:00PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 23 June 2023 13.04
> >
> > On Fri, Jun 23, 2023 at 11:35:29AM +0200, David Marchand wrote:
> > > Hello Bruce,
> > >
> > > On Thu, Jun 22, 2023 at 10:43 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> > > > > Rework deprecated libraries selection by introducing a new configuration
> > > > > option.
> > > > >
> > > > > This breaks existing configurations that were relying on disable_libs=''
> > > > > for enabling deprecated libraries.
> > > > > On the other hand, it will make enabling optional libraries more
> > > > > straightforward by taking the deprecated libraries out of the picture.
> > > > >
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > >
> > > > This gives us a single on/off value for the deprecated libs. So if you
> > > > wants to build only a single deprecated lib, you need to turn on this
> > > > option and then use "disable_libs/enable_libs" option to then selectively
> > > > pick which of the deprecated libs you actually want. Is that the expected
> > > > behaviour? Just checking that we don't want this to be a list too.
> > >
> > > Yes, I wanted a single unified filtering stage.
> > >
> > > But I think your suggestion is easier to use.
> >
> > Slightly easier for the simple case.
> >
> > >
> > > - That would make it simpler for people who simply want to enable kni,
> > > as you mentionned before:
> > > $ meson setup plop -Denable_deprecated_libs=kni
> > >
> > > But I would make this list not overlap with the disable/enable_libs
> > > options evaluation.
> > > Otherwise, in the case of a enable_libs user, the user would have to
> > > set kni in both lists, which is not that great:
> > > $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=kni,vhost
> > >
> > > Instead, I would make it so the config is done as:
> > > $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=vhost
> > >
> > > Is this what you had in mind?
> > >
> > I'm not sure myself what I had in mind, just asking if it had been
> > considered as much as anything else.
> >
> > Having them not-overlap would seem to be necessary to provide a meaningful
> > interface.
> >
> > >
> > > - I don't have a usage for this, but if we go with separating
> > > deprecated and "normal" optional libs filtering, should I introduce a
> > > disable_deprecated_libs too?
> > >
> >
> > That would give us *way* to many options. I think for the sake of simplicity
> > we probably are as well to just go with what you are proposing in this set.
> > Since we only have two deprecated libraries - and hopefully never many more -
> > the benefit of the list for that setting is probably minimal. I'm keen to
> > avoid too much complexity if we can manage it.
>
> I strongly agree with Bruce about avoiding too many options. Here's an idea:
>
> How about just having the disable/enable_libs options, and by default omit the deprecated libs.
>
> Then, the deprecated libs can be enabled by using the enable_libs option.
>
That was the original suggestion and implementation. The trouble is that
if you specify, for example, -Denable_libs=kni, you have just disabled
every other optional library in DPDK, since the enable_libs option switches
DPDK to "allowlist"-only mode.
> Do we have special treatment for deprecated drivers?
>
Its the best solution we have come up with so far to work around the above
problem.
/Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH v4 3/4] build: select deprecated libraries
2023-06-23 11:19 ` Bruce Richardson
@ 2023-06-23 11:32 ` Morten Brørup
2023-06-28 12:10 ` David Marchand
0 siblings, 1 reply; 70+ messages in thread
From: Morten Brørup @ 2023-06-23 11:32 UTC (permalink / raw)
To: Bruce Richardson
Cc: David Marchand, dev, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed, Aaron Conole, Michael Santana
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 23 June 2023 13.19
>
> On Fri, Jun 23, 2023 at 01:15:00PM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Friday, 23 June 2023 13.04
> > >
> > > On Fri, Jun 23, 2023 at 11:35:29AM +0200, David Marchand wrote:
> > > > Hello Bruce,
> > > >
> > > > On Thu, Jun 22, 2023 at 10:43 AM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > >
> > > > > On Wed, Jun 21, 2023 at 07:00:57PM +0200, David Marchand wrote:
> > > > > > Rework deprecated libraries selection by introducing a new
> configuration
> > > > > > option.
> > > > > >
> > > > > > This breaks existing configurations that were relying on
> disable_libs=''
> > > > > > for enabling deprecated libraries.
> > > > > > On the other hand, it will make enabling optional libraries more
> > > > > > straightforward by taking the deprecated libraries out of the
> picture.
> > > > > >
> > > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > >
> > > > > This gives us a single on/off value for the deprecated libs. So if you
> > > > > wants to build only a single deprecated lib, you need to turn on this
> > > > > option and then use "disable_libs/enable_libs" option to then
> selectively
> > > > > pick which of the deprecated libs you actually want. Is that the
> expected
> > > > > behaviour? Just checking that we don't want this to be a list too.
> > > >
> > > > Yes, I wanted a single unified filtering stage.
> > > >
> > > > But I think your suggestion is easier to use.
> > >
> > > Slightly easier for the simple case.
> > >
> > > >
> > > > - That would make it simpler for people who simply want to enable kni,
> > > > as you mentionned before:
> > > > $ meson setup plop -Denable_deprecated_libs=kni
> > > >
> > > > But I would make this list not overlap with the disable/enable_libs
> > > > options evaluation.
> > > > Otherwise, in the case of a enable_libs user, the user would have to
> > > > set kni in both lists, which is not that great:
> > > > $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=kni,vhost
> > > >
> > > > Instead, I would make it so the config is done as:
> > > > $ meson setup plop -Denable_deprecated_libs=kni -Denable_libs=vhost
> > > >
> > > > Is this what you had in mind?
> > > >
> > > I'm not sure myself what I had in mind, just asking if it had been
> > > considered as much as anything else.
> > >
> > > Having them not-overlap would seem to be necessary to provide a meaningful
> > > interface.
> > >
> > > >
> > > > - I don't have a usage for this, but if we go with separating
> > > > deprecated and "normal" optional libs filtering, should I introduce a
> > > > disable_deprecated_libs too?
> > > >
> > >
> > > That would give us *way* to many options. I think for the sake of
> simplicity
> > > we probably are as well to just go with what you are proposing in this
> set.
> > > Since we only have two deprecated libraries - and hopefully never many
> more -
> > > the benefit of the list for that setting is probably minimal. I'm keen to
> > > avoid too much complexity if we can manage it.
> >
> > I strongly agree with Bruce about avoiding too many options. Here's an idea:
> >
> > How about just having the disable/enable_libs options, and by default omit
> the deprecated libs.
> >
> > Then, the deprecated libs can be enabled by using the enable_libs option.
> >
>
> That was the original suggestion and implementation. The trouble is that
> if you specify, for example, -Denable_libs=kni, you have just disabled
> every other optional library in DPDK, since the enable_libs option switches
> DPDK to "allowlist"-only mode.
OK. Hmmm.... Perhaps we can accept that, using this argument:
If you are in a situation where you are using some deprecated libs, you probably also know exactly which other optional libs you are using. So you must specify the full list on enable_libs.
I won't object if anyone disagrees with this idea. I'm only trying to keep it simple. :-)
>
> > Do we have special treatment for deprecated drivers?
> >
> Its the best solution we have come up with so far to work around the above
> problem.
>
> /Bruce
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 3/4] build: select deprecated libraries
2023-06-23 11:32 ` Morten Brørup
@ 2023-06-28 12:10 ` David Marchand
0 siblings, 0 replies; 70+ messages in thread
From: David Marchand @ 2023-06-28 12:10 UTC (permalink / raw)
To: Morten Brørup, Bruce Richardson
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Aaron Conole, Michael Santana
On Fri, Jun 23, 2023 at 1:33 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > > - I don't have a usage for this, but if we go with separating
> > > > > deprecated and "normal" optional libs filtering, should I introduce a
> > > > > disable_deprecated_libs too?
> > > > >
> > > >
> > > > That would give us *way* to many options. I think for the sake of
> > simplicity
> > > > we probably are as well to just go with what you are proposing in this
> > set.
> > > > Since we only have two deprecated libraries - and hopefully never many
> > more -
> > > > the benefit of the list for that setting is probably minimal. I'm keen to
> > > > avoid too much complexity if we can manage it.
> > >
> > > I strongly agree with Bruce about avoiding too many options. Here's an idea:
> > >
> > > How about just having the disable/enable_libs options, and by default omit
> > the deprecated libs.
> > >
> > > Then, the deprecated libs can be enabled by using the enable_libs option.
> > >
> >
> > That was the original suggestion and implementation. The trouble is that
> > if you specify, for example, -Denable_libs=kni, you have just disabled
> > every other optional library in DPDK, since the enable_libs option switches
> > DPDK to "allowlist"-only mode.
>
> OK. Hmmm.... Perhaps we can accept that, using this argument:
>
> If you are in a situation where you are using some deprecated libs, you probably also know exactly which other optional libs you are using. So you must specify the full list on enable_libs.
>
> I won't object if anyone disagrees with this idea. I'm only trying to keep it simple. :-)
- Mixing deprecated and "normal" libraries in a single generic option
means that people (who are selecting some optional libraries only atm)
could miss when an optional library is deprecated (in the future).
On the other hand, if we go with a specialised option for deprecated
libs, users will have to explicitly ask for keeping them.
I think it is also clearer from a user pov.
So I would stick with a separate handling of normal and deprecated libs.
- Adding a disable_deprecated_libs option does not make much sense for
only two libraries.
- For v23.07, I'll announce a change in behavior in the build system
wrt deprecated libraries.
I'll respin on top of Bruce idea (implementing enable_deprecated_libs
as a list) for now.
And we can conclude on how to best expose/configure this by v23.11.
--
David Marchand
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v5 0/2] Select optional libraries
2021-11-17 11:28 ` [PATCH v2 5/5] build: select optional libraries David Marchand
2023-06-16 7:14 ` [PATCH v3] " David Marchand
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
@ 2023-06-28 13:20 ` David Marchand
2023-06-28 13:20 ` [PATCH v5 1/2] build: select deprecated libraries David Marchand
` (2 more replies)
2 siblings, 3 replies; 70+ messages in thread
From: David Marchand @ 2023-06-28 13:20 UTC (permalink / raw)
To: dev; +Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed
This series is one implementation to try and please users who want to
select more easily which parts of DPDK are built.
It introduces a change in behavior for enabling deprecated libraries:
this series is aimed at the next release but sent early as a demo of
what changes are required.
If no strong opposition is met, a deprecation notice will be sent for
v23.07 to announce this change in v23.11.
Changes since v4:
- rebased on main,
- switched to a list of enabled deprecated libraries,
Changes since v3:
- split kni cleanup,
- split variable rename cleanup,
- introduced a new meson option to control deprecated libraries
activation,
- simplified the actual implementation of enable_libs to mimic
enable_drivers behavior,
--
David Marchand
David Marchand (2):
build: select deprecated libraries
build: select optional libraries
.ci/linux-build.sh | 2 +-
devtools/test-meson-builds.sh | 8 +++---
lib/meson.build | 50 +++++++++++++++++++++++++----------
meson_options.txt | 8 ++++--
4 files changed, 47 insertions(+), 21 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v5 1/2] build: select deprecated libraries
2023-06-28 13:20 ` [PATCH v5 0/2] " David Marchand
@ 2023-06-28 13:20 ` David Marchand
2023-06-29 12:44 ` Aaron Conole
2023-06-28 13:20 ` [PATCH v5 2/2] build: select optional libraries David Marchand
2023-06-28 14:48 ` [PATCH v5 0/2] Select " Morten Brørup
2 siblings, 1 reply; 70+ messages in thread
From: David Marchand @ 2023-06-28 13:20 UTC (permalink / raw)
To: dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Aaron Conole, Michael Santana, Bruce Richardson
Rework deprecated libraries selection by introducing a new configuration
option.
This breaks existing configurations that were relying on disable_libs=''
for enabling deprecated libraries.
On the other hand, it will make enabling optional libraries more
straightforward by taking the deprecated libraries out of the picture.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v4:
- changed the option to a list of libraries instead of a global on/off
knob,
- moved all deprecated libs checks under a single block for readability,
---
.ci/linux-build.sh | 2 +-
devtools/test-meson-builds.sh | 8 ++++----
lib/meson.build | 28 ++++++++++++++++++++--------
meson_options.txt | 4 +++-
4 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 45f2729996..e0b62bac90 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -97,7 +97,7 @@ if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
else
- OPTS="$OPTS -Ddisable_libs="
+ OPTS="$OPTS -Denable_deprecated_libs=*"
fi
OPTS="$OPTS -Dlibdir=lib"
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 84b907d2ea..c41659d28b 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -120,10 +120,10 @@ config () # <dir> <builddir> <meson options>
return
fi
options=
- # deprecated libs may be disabled by default, so for complete builds ensure
- # no libs are disabled
- if ! echo $* | grep -q -- 'disable_libs' ; then
- options="$options -Ddisable_libs="
+ # deprecated libs are disabled by default, so for complete builds
+ # enable them
+ if ! echo $* | grep -q -- 'enable_deprecated_libs' ; then
+ options="$options -Denable_deprecated_libs=*"
fi
if echo $* | grep -qw -- '--default-library=shared' ; then
options="$options -Dexamples=all"
diff --git a/lib/meson.build b/lib/meson.build
index fac2f52cad..73afd90b38 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -93,6 +93,15 @@ dpdk_libs_deprecated += [
'kni',
]
+enable_deprecated_libs = []
+foreach l:run_command(list_dir_globs, get_option('enable_deprecated_libs'),
+ check: true).stdout().split()
+ if not dpdk_libs_deprecated.contains(l)
+ continue
+ endif
+ enable_deprecated_libs += l
+endforeach
+
disabled_libs = []
opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
check: true).stdout().split()
@@ -137,16 +146,19 @@ foreach l:libraries
deps += ['eal']
endif
- if disabled_libs.contains(l)
- build = false
- reason = 'explicitly disabled via build config'
- if dpdk_libs_deprecated.contains(l)
- reason += ' (deprecated lib)'
- endif
- else
- if dpdk_libs_deprecated.contains(l)
+ if dpdk_libs_deprecated.contains(l)
+ if not enable_deprecated_libs.contains(l)
+ build = false
+ reason = 'not in enabled deprecated libraries build config'
+ else
warning('Enabling deprecated library, "@0@"'.format(l))
endif
+ elif disabled_libs.contains(l)
+ build = false
+ reason = 'explicitly disabled via build config'
+ endif
+
+ if build
subdir(l)
endif
if name != l
diff --git a/meson_options.txt b/meson_options.txt
index 82c8297065..839d4266c6 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', description:
'Comma-separated list of apps to explicitly disable.')
option('disable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to explicitly disable.')
-option('disable_libs', type: 'string', value: 'flow_classify,kni', description:
+option('disable_libs', type: 'string', value: '', description:
'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
@@ -18,6 +18,8 @@ option('enable_docs', type: 'boolean', value: false, description:
'build documentation')
option('enable_apps', type: 'string', value: '', description:
'Comma-separated list of apps to build. If unspecified, build all apps.')
+option('enable_deprecated_libs', type: 'string', value: '', description:
+ 'Comma-separated list of deprecated libraries to explicitly enable.')
option('enable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to build. If unspecified, build all drivers.')
option('enable_driver_sdk', type: 'boolean', value: false, description:
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v5 2/2] build: select optional libraries
2023-06-28 13:20 ` [PATCH v5 0/2] " David Marchand
2023-06-28 13:20 ` [PATCH v5 1/2] build: select deprecated libraries David Marchand
@ 2023-06-28 13:20 ` David Marchand
2023-06-28 14:48 ` [PATCH v5 0/2] Select " Morten Brørup
2 siblings, 0 replies; 70+ messages in thread
From: David Marchand @ 2023-06-28 13:20 UTC (permalink / raw)
To: dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
Bruce Richardson
There is currently no way to know which libraries are optional.
Introduce a enable_libs option (close to what we have for drivers) so
that packagers or projects consuming DPDK can more easily select the
optional libraries that matter to them and disable other optional
libraries.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v4:
- moved always_enable earlier for readability,
Changes since v3:
- split non related cleanup changes,
- fixed false positive complaints about disabling mandatory libs by
building the list of always enabled libs, appending this list to
enable_list and checking the disable side,
- updated enable/disable_libs descriptions,
Changes since v2:
- moved the IOVA check for kni build in lib/kni/meson.build,
- reworked deprecated libraries handling by only considering them when
no enable_libs option is set. With this, no need for a default value
in meson_options.txt, yet configurations in the CI must be adjusted,
- moved mandatory libraries check after enable/disable_libs evaluation,
---
lib/meson.build | 34 ++++++++++++++++++++++------------
meson_options.txt | 4 +++-
2 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/lib/meson.build b/lib/meson.build
index 73afd90b38..b8834fc277 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -88,6 +88,13 @@ optional_libs = [
'vhost',
]
+always_enable = []
+foreach l:libraries
+ if not optional_libs.contains(l)
+ always_enable += l
+ endif
+endforeach
+
dpdk_libs_deprecated += [
'flow_classify',
'kni',
@@ -102,17 +109,13 @@ foreach l:run_command(list_dir_globs, get_option('enable_deprecated_libs'),
enable_deprecated_libs += l
endforeach
-disabled_libs = []
-opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
- check: true).stdout().split()
-foreach l:opt_disabled_libs
- if not optional_libs.contains(l)
- warning('Cannot disable mandatory library "@0@"'.format(l))
- continue
- endif
- disabled_libs += l
-endforeach
+disable_libs = run_command(list_dir_globs, get_option('disable_libs'), check: true).stdout().split()
+enable_libs = run_command(list_dir_globs, get_option('enable_libs'), check: true).stdout().split()
+if enable_libs.length() == 0
+ enable_libs += optional_libs
+endif
+enable_libs += always_enable
default_cflags = machine_args
default_cflags += ['-DALLOW_EXPERIMENTAL_API']
@@ -153,9 +156,16 @@ foreach l:libraries
else
warning('Enabling deprecated library, "@0@"'.format(l))
endif
- elif disabled_libs.contains(l)
+ elif not enable_libs.contains(l)
build = false
- reason = 'explicitly disabled via build config'
+ reason = 'not in enabled libraries build config'
+ elif disable_libs.contains(l)
+ if always_enable.contains(l)
+ warning('Cannot disable mandatory library "@0@"'.format(l))
+ else
+ build = false
+ reason = 'explicitly disabled via build config'
+ endif
endif
if build
diff --git a/meson_options.txt b/meson_options.txt
index 839d4266c6..7312da3640 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -11,7 +11,7 @@ option('disable_apps', type: 'string', value: '', description:
option('disable_drivers', type: 'string', value: '', description:
'Comma-separated list of drivers to explicitly disable.')
option('disable_libs', type: 'string', value: '', description:
- 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
+ 'Comma-separated list of optional libraries to explicitly disable. [NOTE: mandatory libs cannot be disabled]')
option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
option('enable_docs', type: 'boolean', value: false, description:
@@ -26,6 +26,8 @@ option('enable_driver_sdk', type: 'boolean', value: false, description:
'Install headers to build drivers.')
option('enable_kmods', type: 'boolean', value: false, description:
'build kernel modules')
+option('enable_libs', type: 'string', value: '', description:
+ 'Comma-separated list of optional libraries to explicitly enable. [NOTE: mandatory libs are always enabled]')
option('examples', type: 'string', value: '', description:
'Comma-separated list of examples to build by default')
option('ibverbs_link', type: 'combo', choices : ['static', 'shared', 'dlopen'], value: 'shared', description:
--
2.40.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH v5 0/2] Select optional libraries
2023-06-28 13:20 ` [PATCH v5 0/2] " David Marchand
2023-06-28 13:20 ` [PATCH v5 1/2] build: select deprecated libraries David Marchand
2023-06-28 13:20 ` [PATCH v5 2/2] build: select optional libraries David Marchand
@ 2023-06-28 14:48 ` Morten Brørup
2023-07-17 12:54 ` Bruce Richardson
2 siblings, 1 reply; 70+ messages in thread
From: Morten Brørup @ 2023-06-28 14:48 UTC (permalink / raw)
To: David Marchand, dev
Cc: thomas, bluca, tredaelli, i.maximets, james.r.harris, mohammed,
bruce.richardson
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 28 June 2023 15.20
>
> This series is one implementation to try and please users who want to
> select more easily which parts of DPDK are built.
/me feels pleased :-)
>
> It introduces a change in behavior for enabling deprecated libraries:
> this series is aimed at the next release but sent early as a demo of
> what changes are required.
>
> If no strong opposition is met, a deprecation notice will be sent for
> v23.07 to announce this change in v23.11.
+1 for deprecation notice.
Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v5 1/2] build: select deprecated libraries
2023-06-28 13:20 ` [PATCH v5 1/2] build: select deprecated libraries David Marchand
@ 2023-06-29 12:44 ` Aaron Conole
0 siblings, 0 replies; 70+ messages in thread
From: Aaron Conole @ 2023-06-29 12:44 UTC (permalink / raw)
To: David Marchand
Cc: dev, thomas, bluca, tredaelli, i.maximets, james.r.harris,
mohammed, Michael Santana, Bruce Richardson, Dave Young
David Marchand <david.marchand@redhat.com> writes:
> Rework deprecated libraries selection by introducing a new configuration
> option.
>
> This breaks existing configurations that were relying on disable_libs=''
> for enabling deprecated libraries.
> On the other hand, it will make enabling optional libraries more
> straightforward by taking the deprecated libraries out of the picture.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v4:
> - changed the option to a list of libraries instead of a global on/off
> knob,
> - moved all deprecated libs checks under a single block for readability,
Looks nice. I guess we can also update
guides/prog_guide/kernel_nic_interface.rst
guides/prog_guide/flow_classify_lib.rst
with something like the following:
diff --git a/doc/guides/prog_guide/flow_classify_lib.rst b/doc/guides/prog_guide/flow_classify_lib.rst
index ad2e10ec26..c2706b8d46 100644
--- a/doc/guides/prog_guide/flow_classify_lib.rst
+++ b/doc/guides/prog_guide/flow_classify_lib.rst
@@ -10,8 +10,9 @@ Flow Classification Library
See :doc:`../rel_notes/deprecation`.
It is disabled by default in the DPDK build.
- To re-enable the library, remove 'flow_classify' from the "disable_libs"
- meson option when configuring a build.
+ To re-enable the library, tell meson to enable 'flow_classify' as part of
+ the deprecated libraries meson
+ option ``-Denable_deprecated_libs=flow_classify``.
DPDK provides a Flow Classification library that provides the ability
to classify an input packet by matching it against a set of Flow rules.
diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst b/doc/guides/prog_guide/kernel_nic_interface.rst
index 392e5df75f..1097ecc695 100644
--- a/doc/guides/prog_guide/kernel_nic_interface.rst
+++ b/doc/guides/prog_guide/kernel_nic_interface.rst
@@ -18,7 +18,8 @@ Kernel NIC Interface
.. note::
KNI is disabled by default in the DPDK build.
- To re-enable the library, remove 'kni' from the "disable_libs" meson option when configuring a build.
+ To re-enable the library, tell meson to enable 'kni' as part of the
+ deprecated libraries meson option ``-Denable_deprecated_libs=kni``.
The DPDK Kernel NIC Interface (KNI) allows userspace applications access to the Linux* control plane.
---
> ---
> .ci/linux-build.sh | 2 +-
> devtools/test-meson-builds.sh | 8 ++++----
> lib/meson.build | 28 ++++++++++++++++++++--------
> meson_options.txt | 4 +++-
> 4 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 45f2729996..e0b62bac90 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -97,7 +97,7 @@ if [ "$MINI" = "true" ]; then
> OPTS="$OPTS -Denable_drivers=net/null"
> OPTS="$OPTS -Ddisable_libs=*"
> else
> - OPTS="$OPTS -Ddisable_libs="
> + OPTS="$OPTS -Denable_deprecated_libs=*"
> fi
> OPTS="$OPTS -Dlibdir=lib"
>
> diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
> index 84b907d2ea..c41659d28b 100755
> --- a/devtools/test-meson-builds.sh
> +++ b/devtools/test-meson-builds.sh
> @@ -120,10 +120,10 @@ config () # <dir> <builddir> <meson options>
> return
> fi
> options=
> - # deprecated libs may be disabled by default, so for complete builds ensure
> - # no libs are disabled
> - if ! echo $* | grep -q -- 'disable_libs' ; then
> - options="$options -Ddisable_libs="
> + # deprecated libs are disabled by default, so for complete builds
> + # enable them
> + if ! echo $* | grep -q -- 'enable_deprecated_libs' ; then
> + options="$options -Denable_deprecated_libs=*"
> fi
> if echo $* | grep -qw -- '--default-library=shared' ; then
> options="$options -Dexamples=all"
> diff --git a/lib/meson.build b/lib/meson.build
> index fac2f52cad..73afd90b38 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -93,6 +93,15 @@ dpdk_libs_deprecated += [
> 'kni',
> ]
>
> +enable_deprecated_libs = []
> +foreach l:run_command(list_dir_globs, get_option('enable_deprecated_libs'),
> + check: true).stdout().split()
> + if not dpdk_libs_deprecated.contains(l)
> + continue
> + endif
> + enable_deprecated_libs += l
> +endforeach
> +
> disabled_libs = []
> opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
> check: true).stdout().split()
> @@ -137,16 +146,19 @@ foreach l:libraries
> deps += ['eal']
> endif
>
> - if disabled_libs.contains(l)
> - build = false
> - reason = 'explicitly disabled via build config'
> - if dpdk_libs_deprecated.contains(l)
> - reason += ' (deprecated lib)'
> - endif
> - else
> - if dpdk_libs_deprecated.contains(l)
> + if dpdk_libs_deprecated.contains(l)
> + if not enable_deprecated_libs.contains(l)
> + build = false
> + reason = 'not in enabled deprecated libraries build config'
> + else
> warning('Enabling deprecated library, "@0@"'.format(l))
> endif
> + elif disabled_libs.contains(l)
> + build = false
> + reason = 'explicitly disabled via build config'
> + endif
> +
> + if build
> subdir(l)
> endif
> if name != l
> diff --git a/meson_options.txt b/meson_options.txt
> index 82c8297065..839d4266c6 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -10,7 +10,7 @@ option('disable_apps', type: 'string', value: '', description:
> 'Comma-separated list of apps to explicitly disable.')
> option('disable_drivers', type: 'string', value: '', description:
> 'Comma-separated list of drivers to explicitly disable.')
> -option('disable_libs', type: 'string', value: 'flow_classify,kni', description:
> +option('disable_libs', type: 'string', value: '', description:
> 'Comma-separated list of libraries to explicitly disable. [NOTE: not all libs can be disabled]')
> option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>', description:
> 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
> @@ -18,6 +18,8 @@ option('enable_docs', type: 'boolean', value: false, description:
> 'build documentation')
> option('enable_apps', type: 'string', value: '', description:
> 'Comma-separated list of apps to build. If unspecified, build all apps.')
> +option('enable_deprecated_libs', type: 'string', value: '', description:
> + 'Comma-separated list of deprecated libraries to explicitly enable.')
> option('enable_drivers', type: 'string', value: '', description:
> 'Comma-separated list of drivers to build. If unspecified, build all drivers.')
> option('enable_driver_sdk', type: 'boolean', value: false, description:
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v5 0/2] Select optional libraries
2023-06-28 14:48 ` [PATCH v5 0/2] Select " Morten Brørup
@ 2023-07-17 12:54 ` Bruce Richardson
0 siblings, 0 replies; 70+ messages in thread
From: Bruce Richardson @ 2023-07-17 12:54 UTC (permalink / raw)
To: Morten Brørup
Cc: David Marchand, dev, thomas, bluca, tredaelli, i.maximets,
james.r.harris, mohammed
On Wed, Jun 28, 2023 at 04:48:13PM +0200, Morten Brørup wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Wednesday, 28 June 2023 15.20
> >
> > This series is one implementation to try and please users who want to
> > select more easily which parts of DPDK are built.
>
> /me feels pleased :-)
>
> >
> > It introduces a change in behavior for enabling deprecated libraries:
> > this series is aimed at the next release but sent early as a demo of
> > what changes are required.
> >
> > If no strong opposition is met, a deprecation notice will be sent for
> > v23.07 to announce this change in v23.11.
>
> +1 for deprecation notice.
>
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 70+ messages in thread
end of thread, other threads:[~2023-07-17 12:54 UTC | newest]
Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 16:48 [PATCH 0/5] Extend optional libraries list David Marchand
2021-11-10 16:48 ` [PATCH 1/5] ci: test build with minimum configuration David Marchand
2021-11-16 17:06 ` Thomas Monjalon
2021-11-16 20:39 ` David Marchand
2021-11-16 21:47 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 2/5] build: make GRO/GSO optional David Marchand
2021-11-16 17:11 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 3/5] build: make metrics libraries optional David Marchand
2021-11-16 17:12 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 4/5] build: make pdump optional David Marchand
2021-11-16 17:14 ` Thomas Monjalon
2021-11-10 16:48 ` [PATCH 5/5] build: select optional libraries David Marchand
2021-11-10 17:34 ` Bruce Richardson
2021-11-16 17:25 ` Thomas Monjalon
2021-11-17 10:47 ` Bruce Richardson
2021-11-17 11:27 ` David Marchand
2022-01-07 16:13 ` Morten Brørup
2022-01-07 16:47 ` Stephen Hemminger
2021-11-10 17:34 ` [PATCH 0/5] Extend optional libraries list Bruce Richardson
2021-11-17 11:28 ` [PATCH v2 " David Marchand
2021-11-17 11:28 ` [PATCH v2 1/5] ci: test minimum configuration David Marchand
2021-11-17 11:50 ` Thomas Monjalon
2021-11-17 13:38 ` Aaron Conole
2021-11-17 11:28 ` [PATCH v2 2/5] build: make GRO/GSO optional David Marchand
2021-11-17 11:28 ` [PATCH v2 3/5] build: make metrics libraries optional David Marchand
2021-11-17 11:28 ` [PATCH v2 4/5] build: make pdump optional David Marchand
2021-11-17 11:28 ` [PATCH v2 5/5] build: select optional libraries David Marchand
2023-06-16 7:14 ` [PATCH v3] " David Marchand
2023-06-16 9:42 ` Bruce Richardson
2023-06-19 8:00 ` David Marchand
2023-06-19 14:11 ` David Marchand
2023-06-19 14:26 ` Bruce Richardson
2023-06-20 8:31 ` David Marchand
2023-06-20 8:35 ` Bruce Richardson
2023-06-20 8:38 ` David Marchand
2023-06-20 8:44 ` Bruce Richardson
2023-06-20 8:48 ` David Marchand
2023-06-20 9:03 ` Bruce Richardson
2023-06-20 14:33 ` Thomas Monjalon
2023-06-20 14:40 ` Bruce Richardson
2023-06-20 15:01 ` Bruce Richardson
2023-06-21 9:54 ` David Marchand
2023-06-21 10:49 ` Bruce Richardson
2023-06-21 11:35 ` Morten Brørup
2023-06-22 9:27 ` Juraj Linkeš
2023-06-21 17:00 ` [PATCH v4 0/4] Select " David Marchand
2023-06-21 17:00 ` [PATCH v4 1/4] kni: move IOVA build check David Marchand
2023-06-22 8:37 ` Bruce Richardson
2023-06-21 17:00 ` [PATCH v4 2/4] build: rename enabled libraries list David Marchand
2023-06-22 8:38 ` Bruce Richardson
2023-06-21 17:00 ` [PATCH v4 3/4] build: select deprecated libraries David Marchand
2023-06-22 8:43 ` Bruce Richardson
2023-06-22 8:50 ` Bruce Richardson
2023-06-23 9:35 ` David Marchand
2023-06-23 11:04 ` Bruce Richardson
2023-06-23 11:15 ` Morten Brørup
2023-06-23 11:19 ` Bruce Richardson
2023-06-23 11:32 ` Morten Brørup
2023-06-28 12:10 ` David Marchand
2023-06-21 17:00 ` [PATCH v4 4/4] build: select optional libraries David Marchand
2023-06-22 8:49 ` Bruce Richardson
2023-06-22 9:09 ` [PATCH v4 0/4] Select " Bruce Richardson
2023-06-22 16:41 ` Thomas Monjalon
2023-06-28 13:20 ` [PATCH v5 0/2] " David Marchand
2023-06-28 13:20 ` [PATCH v5 1/2] build: select deprecated libraries David Marchand
2023-06-29 12:44 ` Aaron Conole
2023-06-28 13:20 ` [PATCH v5 2/2] build: select optional libraries David Marchand
2023-06-28 14:48 ` [PATCH v5 0/2] Select " Morten Brørup
2023-07-17 12:54 ` Bruce Richardson
2021-11-17 11:52 ` [PATCH v2 0/5] Extend optional libraries list Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).