Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meson fixes #621

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Kangie
Copy link
Contributor

@Kangie Kangie commented Dec 11, 2024

Bump the minimum meson version to reflect what is actually in-use in the project and make a boolean value actually boolean.

@ppisar
Copy link
Collaborator

ppisar commented Dec 11, 2024

We keep compatibility with RHEL 8 where is is meson 0.55.3. That means we cannot increase the minimal version above it.

@Kangie
Copy link
Contributor Author

Kangie commented Dec 12, 2024

We keep compatibility with RHEL 8 where is is meson 0.55.3. That means we cannot increase the minimal version above it.

You already have - you're using features only available since 0.56.0.

Edit: It looks like you have a case handling old meson. Feel free to merge the second commit. Recommend bumping the older meson on RHEL 8, there's really no disadvantage to it.

@eli-schwartz
Copy link

You already have - you're using features only available since 0.56.0.

Edit: It looks like you have a case handling old meson.

if meson.version().version_compare('< 0.56.0')
project_source_root = meson.source_root()
project_build_root = meson.build_root()
else
project_source_root = meson.project_source_root()
project_build_root = meson.project_build_root()
endif

Meson will automatically parse if meson.version().version_compare(XXX) and respect its contract by not emitting warnings inside of that code block for the main project's supported version. However, this suppression only works for if nodes, not else nodes. Getting it to work correctly in an else node could be tricky as there isn't a direct map from version_compare()'s return value to "is this node being evaluated".

I don't see this style at all, to be honest. Existing uses of version_compare() conditionals inevitably tend to use >= comparisons, with the newer style in the if and the fallback code in the else. Under such usage, meson will avoid warning about too-new features.

@eli-schwartz
Copy link

meson: make boolean options boolean

Later versions of meson deprecate this "feature", and every
other boolean value in meson.options is actually a bool.

To help clarify here, the grammar has always said that option('xxx', type: 'boolean') accepts a value: kwarg where the value is the actual value, not a string representing the value. Same logic applies for e.g. integer options. The reason that passing the string representation worked was actually because of an unintended CLI handling leaking into the wrong layer (since CLI parameters are always inherently strings, it needs to lex the string into rich object tokens).

It was deprecated without replacement in meson 1.1.0, since it has no use case and all existing instances in the wild are assumed to be typos. This PR fixes that typo.

meson will automatically suppress the warnings about using
features from later versions in the `if` statement.

This is not the case when the later version features are in
`else`.

Rewrite the conditional to switch to comparing `>=${NEW_VERSION}`
so that meson stops warning the use of newer features than our
`meson_version`.

Signed-off-by: Matt Jolly <[email protected]>
Later versions of meson deprecate this "feature", and every
other boolean value in meson.options is actually a bool.

Signed-off-by: Matt Jolly <[email protected]>
Copy link
Collaborator

@sgallagher sgallagher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these patches look good to me now.

Getting rid of that annoying warning about the minimum version is great.

Lastly: we have no control over what version of meson is shipped in RHEL 8, because it's 1) locked due to support guarantees and 2) an OS in maintenance-only mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants