Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .github/workflows/os_comp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,10 @@ jobs:
- CC: 'gcc'
CXX: 'g++'
- MESON_ARGS: '--unity=on -Ddefault_library=static'
MESON_TEST_DDEFAULT_LIBRARY: yes
RUN_TESTS_ARGS: '--no-unittests'
CC: 'gcc'
CXX: 'g++'
- MESON_ARGS: '-Ddefault_library=both'
MESON_TEST_DDEFAULT_LIBRARY: yes
RUN_TESTS_ARGS: '--no-unittests'
CC: 'gcc'
CXX: 'g++'
Expand All @@ -132,7 +130,6 @@ jobs:
env:
MESON_RSP_THRESHOLD: ${{ matrix.cfg.MESON_RSP_THRESHOLD }}
MESON_ARGS: ${{ matrix.cfg.MESON_ARGS }}
MESON_TEST_DDEFAULT_LIBRARY: ${{ matrix.cfg.MESON_TEST_DDEFAULT_LIBRARY }}
RUN_TESTS_ARGS: ${{ matrix.cfg.RUN_TESTS_ARGS }}
CC: ${{ matrix.cfg.CC }}
CXX: ${{ matrix.cfg.CXX }}
Expand Down
19 changes: 12 additions & 7 deletions docs/markdown/Builtin-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,10 @@ command line: `-Dfoo:default_library=static`.
The value is overridden in this order:
- `opt=value` from parent project's `default_options`
- `opt=value` from subproject's `default_options`
- `subp:opt=value` from parent project's default options
- `opt=value` from `subproject()` `default_options`
- `opt=value` from machine file
- `opt=value` from command line
- `subp:opt=value` from parent project's default options
- `opt=value` from `subproject()` `default_options`
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect. The command line should override anything set in project files. (given the description this list is sorted on increasing precedence)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was also what I thought when preparing the changes that went into 1.8.4. But it turns out that making two separate groups for global settings and augments (the machine file and command line winning in each group only) is more useful, and there are projects in the wild that rely on it.

Copy link
Member

Choose a reason for hiding this comment

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

How is it more useful specifically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For example you can give -Dwerror=true but meson.build can override it to false for subprojects that are known to not behave; likewise for -Dwarning_level. It is also more in line with how default_library works in pre-1.8 releases (see the reverted test changes in this PR).

Besides, we got reports like #15006 where the new ordering breaks existing projects that worked before 1.8.4. I didn't have much time to debug what exactly caused the breakage, but this patch does fix it. So, while overall the new option code is now tamed, I think this part has to be rolled back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even the documentation says so:

This is useful, for example, when building shared libraries in the main project and statically linking a subproject, or when the main project must build with no warnings but some subprojects cannot.

But the trick doesn't work anymore in 1.8.4

Copy link
Member

Choose a reason for hiding this comment

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

The problem here seems to be that the table collates two different things making it hard to comprehend. Maybe it should be split in two, with a structure like:

  • Per-subproject settings always override the default value, no matter how or where they are defined
  • When configuring for the first time, values for options (and overrides) are used in the following order

And maybe have the array in decreasing order of priority, as that seems to be the more logical order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even that is not unambiguously true, though I agree it's as close as you can get, because -Dopt does override "opt=value from subproject's default_options" which is subproject-specific. So I don't think it's much better than listing the possibilities one by one.

That said do you agree now that the ordering as implemented by the patch makes sense?

- `subp:opt=value` from machine file
- `subp:opt=value` from command line

Expand All @@ -400,12 +400,17 @@ Between *0.54.0* and *1.7.x* only a few options could be defined per subproject:

The value was overridden in this order:

- Value from parent project
- Value from subproject's `default_options`
- Value from `subproject()` `default_options`
- Value from machine file
- Value from command line
- `opt=value` from parent project's `default_options`
- `opt=value` from machine file
- `opt=value` from command line
- `opt=value` from subproject's `default_options`
- `subp:opt=value` from parent project's default options
- `opt=value` from `subproject()` `default_options`
- `subp:opt=value` from machine file
- `subp:opt=value` from command line

In other word, the subproject's `default_options` had a *higher* priority
than `opt=value` from machine file or command line.

## Module options

Expand Down
16 changes: 8 additions & 8 deletions mesonbuild/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -1380,6 +1380,14 @@ def initialize_from_subproject_call(self,
key = key.evolve(subproject=subproject)
options[key] = valstr

# then global settings from machine file and command line
# **but not if they are toplevel project options**
for key, valstr in itertools.chain(machine_file_options.items(), cmd_line_options.items()):
if key.subproject is None and not self.is_project_option(key.as_root()):
subp_key = key.evolve(subproject=subproject)
# just leave in place the value that was set for the toplevel project
options.pop(subp_key, None)

# augments from the toplevel project() default_options
for key, valstr in self.pending_subproject_options.items():
if key.subproject == subproject:
Expand All @@ -1395,14 +1403,6 @@ def initialize_from_subproject_call(self,
key = key.evolve(subproject=subproject)
options[key] = valstr

# then global settings from machine file and command line
# **but not if they are toplevel project options**
for key, valstr in itertools.chain(machine_file_options.items(), cmd_line_options.items()):
if key.subproject is None and not self.is_project_option(key.as_root()):
subp_key = key.evolve(subproject=subproject)
self.pending_subproject_options.pop(subp_key, None)
options.pop(subp_key, None)

# then finally per project augments from machine file and command line
for key, valstr in itertools.chain(machine_file_options.items(), cmd_line_options.items()):
if key.subproject == subproject:
Expand Down
2 changes: 1 addition & 1 deletion test cases/common/223 persubproject options/meson.build
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
project('persubproject options', 'c', 'cpp',
default_options : ['werror=true', 'default_library=both',
default_options : ['werror=true',
'warning_level=3',
'cpp_std=c++11'])

Expand Down
6 changes: 5 additions & 1 deletion test cases/common/223 persubproject options/test.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
{
"skip_on_env": ["MESON_TEST_DDEFAULT_LIBRARY"]
"matrix": {
"options": {
"default_library": [ { "val": "both" } ]
}
}
}
13 changes: 3 additions & 10 deletions unittests/machinefiletests.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,18 +563,11 @@ def test_builtin_options_machinefile_overrides_subproject(self):
check = cm.exception.stdout
self.assertIn(check, 'Parent should override default_library')

def test_builtin_options_machinefile_global_overrides_subproject(self):
# The buildfile says subproject(... default_library: static), ensure that's overridden
def test_builtin_options_machinefile_global_loses_over_subproject(self):
# The buildfile says subproject(... default_library: static), ensure that it overrides the machinefile
testcase = os.path.join(self.common_test_dir, '223 persubproject options')
config = self.helper_create_native_file({'built-in options': {'default_library': 'both'}})

with self.assertRaises((RuntimeError, subprocess.CalledProcessError)) as cm:
self.init(testcase, extra_args=['--native-file', config])
if isinstance(cm, RuntimeError):
check = str(cm.exception)
else:
check = cm.exception.stdout
self.assertIn(check, 'Parent should override default_library')
self.init(testcase, extra_args=['--native-file', config])

def test_builtin_options_compiler_properties(self):
# the properties section can have lang_args, and those need to be
Expand Down
Loading