Skip to content

Conversation

bonzini
Copy link
Collaborator

@bonzini bonzini commented May 15, 2025

I'm not sure if this is the right fix or too big a hammer, but let's try.

Fixes: #14608

@bonzini bonzini marked this pull request as ready for review May 15, 2025 17:41
@bonzini bonzini requested a review from jpakkane as a code owner May 15, 2025 17:41
@bonzini bonzini added this to the 1.8.1 milestone May 15, 2025
@bonzini bonzini added regression options Meson configuration options labels May 15, 2025
@bonzini
Copy link
Collaborator Author

bonzini commented May 15, 2025

Old code indeed used the same order as after the patch. From CoreData.set_default_options:

        options: T.MutableMapping[OptionKey, T.Any] = OrderedDict()
        for k, v in default_options.items(): # process project default options
            if not subproject or k.subproject == subproject:
                options[k] = v
        options.update(env.options)          # override them with machine default and command line options
        env.options = options

@bonzini
Copy link
Collaborator Author

bonzini commented May 16, 2025

@jpakkane can you confirm that this wasn't intentional?

@jpakkane
Copy link
Member

Needs a rebase after #14635.

@dcbaker
Copy link
Member

dcbaker commented May 22, 2025

That was merged, a rebase should clear up most or all of the failing tests

@dcbaker
Copy link
Member

dcbaker commented May 22, 2025

I'm going to say that even if it was intentional, it's a pretty big behavioral change that, IMHO, would be a breaking backwards incompatible change and require a Meson 2.0 with proper deprecation cycle

@jpakkane
Copy link
Member

The documentation says:

You can set some Meson built-in options on a per-subproject basis,
such as `default_library` and `werror`. The order of precedence is:

1) Command line
2) Machine file
3) Build system definitions

So no, the change was not intentional. The new unit test proves that this implementation is correct.

proj_value = 'xcode'
mfile_value = 'vs2010'
k = OptionKey(name)
prefix = UserStringOption('prefix', 'This is needed by OptionStroe', '/usr')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
prefix = UserStringOption('prefix', 'This is needed by OptionStroe', '/usr')
prefix = UserStringOption('prefix', 'This is needed by OptionStore', '/usr')

@jpakkane
Copy link
Member

With the typo fix + rebase, LGTM.

Restore the behavior from before commit d37d649 ("Make all Meson level
options overridable per subproject.", 2025-02-13).  The old code was:

        options: T.MutableMapping[OptionKey, T.Any] = OrderedDict()

        # process project default options
        for k, v in default_options.items():
            if not subproject or k.subproject == subproject:
                options[k] = v

        # override them with machine default and command line options
        options.update(env.options)
        env.options = options

Fixes: mesonbuild#14608
Signed-off-by: Paolo Bonzini <[email protected]>
@eli-schwartz eli-schwartz merged commit 82fca50 into mesonbuild:master May 23, 2025
29 of 31 checks passed
@bonzini bonzini deleted the fix14608 branch May 23, 2025 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
options Meson configuration options regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meson 1.8 ignores buildtype defined in native-file
4 participants