Refactor option getting v2 #13441
Refactor option getting v2 #13441
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jpakkane
force-pushed
the
optionrefactor2
branch
from
e5c9a29
to
2ccdbcd
Compare
|
Cherry-picked the relevant remaining commits. Expect everything to be still broken. Will work on fixing things again next. |
| optstore = OptionStore() | ||
| name = 'cpp_std' | ||
| sub_name = 'sub' | ||
| sub2_name = 'sub2' |
Check notice
Code scanning / CodeQL
Unused local variable Note
jpakkane
force-pushed
the
optionrefactor2
branch
from
b6023ce
to
484a90b
Compare
|
Fixed enough that all No point in commenting on typing, though, it has not been done at all. |
|
Now passes all project tests. On my machine. |
jpakkane
force-pushed
the
optionrefactor2
branch
from
2166f0a
to
ae783b7
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
from
b3ca516
to
43b2a7b
Compare
| def hard_reset_from_prefix(self, prefix:str): | ||
| for optkey, prefix_mapping in BUILTIN_DIR_NOPREFIX_OPTIONS.items(): | ||
| valobj = self.options[optkey] | ||
| new_value = valobj.value |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
jpakkane
force-pushed
the
optionrefactor2
branch
3 times, most recently
from
609bf4a
to
8111038
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
from
243e096
to
9734ea3
Compare
| general version of enabling optimizations on all subprojects but not | ||
| the top project would be done like this: | ||
|
|
||
| meson configure -Doptimization=2 -A:optimization=0 |
There was a problem hiding this comment.
I think this should be meson configure -Doptimization=0 -A:optimization=2 with the values passed to the optimization option swapped (this applies to your recent blog post too).
There was a problem hiding this comment.
Not really:
-Doptimization=2 -A:optimization=0 means "optimize everything except the top level project.
-Doptimization=0 -A:optimization=2 means "optimize top project only".
The former is you want most of the time and what happens when you use Linux distro dependencies. The -A:bob syntax means apply only to the top level project, not apply to all subprojects.
If this syntax is confusing we can change it. I did go through a bunch of them and this seemed to be the most consistant and simple, but if someone comes up with a better syntax, great.
There was a problem hiding this comment.
Uhm. I indeed misunderstood the syntax for the -A option argument. After thinking for a brief moment, I think the syntax implemented here makes sense.
However, I'm not sure of the reason why both the -A and -D options are needed. I think that meson configure -Doptimization=0 -A:optimization=2 could be written as meson configure -Doptimization=0 -D:optimization=2 and that the argument to the second -D option could be parsed to make it into something that applies to the top project only (valid option names cannot contain columns, AFAIK). Having two syntax for specifying setup options feels strange. Similarly, the -U option would apply both to global options ans to project specific options.
There was a problem hiding this comment.
The idea was that -A is used to create a new augment, -U is for unsetting it and -D is for changing the value of an existing thing. So for example:
meson configure -Asub:opt=v
This creates a new augment and then doing
meson configure -Asub:opt=v
is an error, because creating an augment that already exists would be a logical error. Then doing
meson configure -Dsub:opt=w
would change the value,
meson configure -Usub:opt
would delete it and after that doing
meson configure -Dsub:opt=w
would be an error again, as the subproject option does not exist.
There was a problem hiding this comment.
Why does the action of creating an augment need to be explicit, from the point of view of the user? I'm also not sure about how it combines with meson setup --reconfigure. Should -A or -D be used to set an augment when running meson setup --reconfigure? This is particularly confusing because recent meson versions accent the --reconfigure option also when the specified build directory is not yet initialized.
Currently there is no way to reset an option to the project default, thus it would be nice to be able to use the -U option to unset auments and global options.
There was a problem hiding this comment.
I think probably the common mental model that most people have of options as they are now, before this PR, is:
-Dfoo:bar=truesets the value of an existing option in a subproject,-Dbar=truesets the value of an existing option in the superproject- options specified in a meson_options.txt / meson.options file only exist for a single project (maybe the superproject)
- options built into meson create an option for every subproject, but some of them are read-only aliases of the superproject version of the option, such as
prefix, and some of them are distinct settable options such asdefault_library - using
get_option('bar')in a meson.build file will look up the subproject option, even if it is a read-only alias.
The part of this mental model we are trying to fix is the fact that:
- "what is a read-only alias and what is actually settable" is hugely inconsistent and we want most things to be settable
- there's no straightforward way to set a per-subproject option without enumerating all subprojects plus the superproject
There was a problem hiding this comment.
I haven't taken a good look at how this PR handles it all, but one straightforward way to define the distinction is by looking at something like prefix.
With this PR, what happens if you try to "augment" the prefix option?
With this PR, what option gets looked up when you do get_option('prefix') in a subproject? What is the user-facing mental model of that option? This scenario doesn't appear to be covered by the unittests, at least, and it should.
There was a problem hiding this comment.
I added a commit that removes -A in favour of -D. Docs not updates. Does this make the UI better?
There was a problem hiding this comment.
Thanks for being receptive of my comments @jpakkane. I haven't had the occasion to delve into the implementation, and my previous comments were based almost entirely on reading the additions to the documentation, which is not yet updated here. However, having only the -D option clearly solves most of my issue with the new feature.
| a single subproject called `numbercruncher` that does heavy | ||
| computation. During development you want to build that subproject with | ||
| optimizations enabled but your main project without | ||
| optimizations. This can be done by specifying an augment to the given |
There was a problem hiding this comment.
The first tow times I read "augment" I thought it was "argument" misspelled. IIUC, augments are subproject-specific options. Can't the concept be called "subproject-specific option" instead of "augment"? I think it would be clearer and I don't think it need to be written so often for the lengthy name to be a problem.
There was a problem hiding this comment.
Originally I wanted to call it "override", but that is already used for per-target option values, so no. I guess we can call them "subproject specific options" in docs, but internally we need a more concise name. And in any case it would be nice to have the same name for this feature internally and externally.
"Augment" was the best I could come up with, better ideas welcome.
There was a problem hiding this comment.
dumb thought: call it an augmentation instead of an augment and no one will get it confused with an argument?
There was a problem hiding this comment.
"Augment" was the best I could come up with, better ideas welcome.
Supersede or Negate might be considered. But expanding them to "supersedes" or "Negates" or "Negations" fouls things up. Then there is supersession, where the main advantage is that it's a long way from `argument'. ;)
jpakkane
force-pushed
the
optionrefactor2
branch
from
06f255b
to
1b276ab
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
from
262d16e
to
9aba065
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
2 times, most recently
from
72a484a
to
70f2c67
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
2 times, most recently
from
fd617cb
to
1a9f046
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
from
c3dcb9a
to
5f67e70
Compare
jpakkane
force-pushed
the
optionrefactor2
branch
from
5f67e70
to
9d5594c
Compare
|
Fixed docs. |
|
One more things to note is that we can add a |
This is basically a rebased version of #13086. Well, half of it currently. Will do the rest after this.
The refactor operation turned out to be so big I though it better to open a new one and leave that MR exist in its original state in case it is ever needed for debugging. Will close that once this is merged.
This has a bunch of broken commits and fixing them would be too much work, so this must be merged with a merge commit rather than rebasing.