Re: [dpdk-dev] [PATCH v3 0/7] Add ABI compatibility checks to the meson build

[David Marchand <david.marchand@redhat.com>]

On Fri, Nov 29, 2019 at 10:09 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
>
> With the recent changes made to stabilize ABI versioning in DPDK, it will
> become increasingly important to check patches for ABI compatibility. We
> propose adding the ABI compatibility checking to be done as part of the
> build.
>
> The advantages to adding the ABI compatibility checking to the build are
> two-fold. Firstly, developers can easily check their patches to make sure
> they don’t break the ABI without adding any extra steps. Secondly, it
> makes the integration into existing CI seamless since there are no extra
> scripts to make the CI run. The build will run as usual and if an
> incompatibility is detected in the ABI, the build will fail and show the
> incompatibility. As an added bonus, enabling the ABI compatibility checks
> does not impact the build speed.
>
> The proposed solution works as follows:
> 1. Generate the ABI dump of the baseline. This can be done with the new
>    script added in this RFC. This step will only need to be done when the
>    ABI version changes (so once a year) and can be added to master so it
>    exists by default. This step can be skipped if the dump files for the
>    baseline already exist.
> 2. Build with meson. If there is an ABI incompatibility, the build will
>    fail and print the incompatibility information.
>
> The patches accompanying this RFC add the ABI dump file generating script,
> the meson option required to enable/disable the checks, and the required
> meson changes to run the compatibility checks during the build.

Global comments:
- why are patch 1 and 2 in this series, is this really needed?
- please squash patches 3, 4, 5 and 6, reading them separately is a
pain and they are quite small,
- if Windows does not support the ABI check, enforce this earlier in
meson and refuse enabling it rather than silently ignoring it,
- I would not enable this check by default
  - this is a developer option, people just building the dpdk don't
care about it,
  - can we have something like a tristate "auto" (default, triggers
the check if abidiff is installed), "disabled" and "enabled" (not
having abidiff installed triggers an error) ?
- please update the travis packages so that we can run those checks
for developers submitting patches
- I don't think you tested this series with
devtools/test-meson-builds.sh, please do. It fails with a custom build
directory and in the dpdk tree too:

Build targets in project: 1019
WARNING: Project specifies a minimum meson_version '>= 0.47.1' but
uses features which were added in newer versions:
 * 0.48.0: {'console arg in custom_target'}
 * 0.50.0: {'install arg in configure_file'}
Found ninja-1.9.0 at /usr/bin/ninja
ninja -C /home/dmarchan/builds/build-gcc-static
ninja: Entering directory `/home/dmarchan/builds/build-gcc-static'
[48/2291] Generating librte_kvargs.abi_chk with a meson_exe.py custom command.
FAILED: lib/librte_kvargs.abi_chk
/usr/bin/meson --internal exe
/home/dmarchan/builds/build-gcc-static/meson-private/meson_exe_abidiff_6511538ddd95d9672028017110fa45c67f01f7be.dat
file /home/dmarchan/dpdk/lib/abi/librte_kvargs.dump does not exist
[77/2291] Compiling C object
'lib/76b5a35@@rte_mbuf@sta/librte_mbuf_rte_mbuf.c.o'.
ninja: build stopped: subcommand failed.


--
David Marchand