On Mon, Jun 16, 2025 at 08:09:44AM -0700, Andre Muezerie wrote:
> On Mon, Jun 16, 2025 at 09:27:00AM +0100, Bruce Richardson wrote:
> > On Mon, Jun 16, 2025 at 08:36:31AM +0100, Bruce Richardson wrote:
> > > On Fri, Jun 13, 2025 at 09:14:38AM -0700, Andre Muezerie wrote:
> > > > There is a minimum meson version specified in the DPDK meson project
> > > > section, which has been documented. This string is parsed by
> > > > buildtools\get-min-meson-version.py and this information is used by
> > > > lab automation to install the corresponding meson package on the
> > > > build machine.
> > > >
> > > > Turns out that the meson version specified on the DPDK project
> > > > (0.57.x) is buggy on Windows: it has issues related to path
> > > > manipulation, and these issues are causing failures. Therefore,
> > > > a newer meson is required on Windows.
> > > >
> > > > To avoid bringing the minimum requirements up for all operating
> > > > systems before a more appropriate release, this patch implements
> > > > a mechanism allowing a different version to be specified for Windows.
> > > >
> > > > Signed-off-by: Andre Muezerie <andre...@linux.microsoft.com>
> > >
> > > I am fine with the principle, few comments inline below just on the
> > > specific implementation.
> > >
> > > /Bruce
> > >
> > > > ---
> > > > buildtools/get-min-meson-version.py | 12 ++++++++++--
> > > > meson.build | 9 +++++++++
> > > > 2 files changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/buildtools/get-min-meson-version.py
> > > > b/buildtools/get-min-meson-version.py
> > > > index f0de8fdf2b..10a2596a5a 100755
> > > > --- a/buildtools/get-min-meson-version.py
> > > > +++ b/buildtools/get-min-meson-version.py
> > > > @@ -11,12 +11,19 @@
> > > > basedir = dirname(buildtools_path)
> > > >
> > > > with open(join(basedir, "meson.build")) as f:
> > > > + if os.name == "nt":
> > > > + keyword = "windows_meson_version"
> > > > + pattern = r"windows_meson_version:\s*'>=\s*([0-9\.]+)'"
> > > > + else:
> > > > + keyword = "meson_version"
> > > > + pattern = r"meson_version:\s*'>=\s*([0-9\.]+)'"
> > > > +
> > >
> > > Minor nit, but I'd suggest shortening this a little to:
> > >
> > > keyword = "meson_version"
> > > if os.name == "nt":
> > > keyword = "windows_" + keyword
> > > pattern = fr"{keyword}:\s*'>=\s*([0-9\.]+)'"
> > >
> > > or even further shortening gives:
> > >
> > > keyword = "windows_meson_version" if os.name == "nt" else
> > > "meson_version"
> > > pattern = fr"{keyword}:\s*'>=\s*([0-9\.]+)'"
> > >
> > > > for ln in f.readlines():
> > > > - if "meson_version" not in ln:
> > > > + if keyword not in ln:
> > > > continue
> > > >
> > > > ln = ln.strip()
> > > > - ver_match = re.search(r"meson_version:\s*'>=\s*([0-9\.]+)'",
> > > > ln)
> > > > + ver_match = re.search(pattern, ln)
> > > > if not ver_match:
> > > > print(
> > > > f"Meson version specifier not in recognised format:
> > > > '{ln}'",
> > > > @@ -24,3 +31,4 @@
> > > > )
> > > > sys.exit(1)
> > > > print(ver_match.group(1), end="")
> > > > + break
> > > > diff --git a/meson.build b/meson.build
> > > > index 5ff68cb3af..1ae51996be 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -13,6 +13,15 @@ project('DPDK', 'c',
> > > > meson_version: '>= 0.57.2'
> > > > )
> > > >
> > > > +# Meson does not support dynamic/conditional values for meson_version
> > > > in
> > > > +# the project section.
> > > > +# The line below contains the minimum meson version required on
> > > > Windows,
> > > > +# which gets parsed by get-min-meson-version.py.
> > > > +# It is important to keep this line below the "project" section, so
> > > > that
> > > > +# the script can make the correct determination according to the
> > > > operating
> > > > +# system.
> > > > +# windows_meson_version: '>= 1.5.2'
> > > > +
> > >
> > > Why not pick a string that is not a superset of the general meson_version
> > > one? That would avoid the need to position it below the project one, and
> > > save you having to document that in a long comment. (also avoid the need
> > > to
> > > add the "break" into the python script, not that that is a big deal!)
> > > For example, how about using:
> > >
> > > * windows_min_meson
> > > * windows_meson_ver
> > > * windows meson_min_ver
> > >
> > Or even: "meson_version_windows", since the rest of the pattern we look for
> > occurs after this string.
>
> I like the 'meson_version_windows' suggested and will send out an updated
> patch
> along with the shorter syntax.
> I'll keep the "break" though at it will still be needed to avoid a match on
> Linux when "if keyword not in ln" encounters "meson_version_windows". That
> break makes sense as there's no point in parsing the rest of the file once
> the line we're looking for is found.
>
Agreed. No prob with keeping it.
If using meson_version_windows, maybe have the comment specifying that
immediately on the next line after the regular meson_version statement. It
would be good to keep the two specifiers together.
/Bruce
