mesonbuild
Refactor option getting v2
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.
Cherry-picked the relevant remaining commits. Expect everything to be still broken. Will work on fixing things again next.
Fixed enough that all common tests pass. Feel free to start reviewing those bits.
No point in commenting on typing, though, it has not been done at all.
It's finally green! Which is not to say that is is done. There is a whole lot of stuff still to do. Some tests have been disabled and there are review comments and such to fix. It also needs usability testing with real world projects. I'd expect that at least some compilers not in the test matrix are going to fail.
Thus I propose the following:
- Create a new release without this change ASAP
- Add a release note snippet warning of the upcoming feature change and ask people to test it
- Merge this as the first thing after the release is done
- Fix and update things until things generally work
This would give this feature the maximum amount of time to work out regressions, alter command line options and so on. We can keep changing pretty much everything until the following release happens. Then things get frozen.
There are two different things here. Firstly, and this is something I had not considered at all before, prefix is special Setting it affects other things as well. It might be entirely reasonable to prohibit adding an augoment on prefix.
But assuming some other option foo that is set to some value basic and there is a subproject augment advanced for it. Then in the top project this happens:
get_option('foo') # returns the value 'basic'
whereas in the subproject this happens:
get_option('foo') # returns the value 'advanced'
For all intents and purposes when you are inside the subproject it is "as if" the value of the option had been set to advanced at the top level. You can not query the existence of augments nor can you ask what the value would be in some other subproject.
You never could, though. So nothing changed there.
There's a reason why I describe the previous mental model as "get_option queries the per-subproject value, but some per-subproject options are read-only aliases of a global setting".
The mental model is NOT "it is as if the top-level option was set to something else". If that had been the mental model it would break hard on the fact that you can have a top-level meson_options.txt that defines a "foo" option, and a subproject meson_options.txt that defines a "bar" option. In the subproject, you CANNOT do get_option('foo') because it's not a valid option for that subproject.
meson global-builtin options simply happen to exist in all projects, even subprojects. Hence they are valid subproject options, even if unfortunately before now they were read-only options when people would really like to modify them.
If that had been the mental model it would break hard on the fact that you can have a top-level meson_options.txt that defines a "foo" option, and a subproject meson_options.txt that defines a "bar" option.
But those are project options, they keep working as they always have. You can only set augments on "meson level options" (or, alternatively, "not project options").
When do you think you'll have this in shape for review? Just glance at the code a lot the decisions you've made don't make sense to me, and having a clean commit history with commits that explain not just what's changing, but why would be helpful for that.
This is as good of a review state this is probably ever going to get. The change was too massive to make happen cleanly. :( Those bits that could be done independently (OptionKey changes etc) were submitted separately, but when the main implementation changed, it lead to changing everything.
Some things to note:
-
There are multiple getters in OptionStore that do pretty much the same thing, there should be only one of each type (onee was the "clean design version" while the other was the "mimic what the old code did" version). Getting to a single version was not really feasible until all tests passed.
-
OptionStore has been written so that you can write tests to it that do not need the rest of the machinery, leading to more unit tests (which are fast) and fewer integration tests (which are slow)
-
Introspection format changes have been ignored, it needs a way to specify augments.
When I have put in large refactors in this format this (I don't know how many times I've rewritten the entire build module) I got significant push back that individually reviewable patches that make one logical change; fail no tests at any point; don't regress any linters or typing checking at any point; and have clear, detailed commit messages are required. Often the attempt to get to that point has meant that the ground has moved out from under me faster than I could rewrite and I end up having to abandon the whole effort. So consider me very frustrated to find out that our merge rules contain none of those features.
I will make an attempt to review this as is then, but expect it to take me several weeks and to have lots of questions.
significant push back that individually reviewable patches that make one logical change; fail no tests at any point; don't regress any linters or typing checking at any point; and have clear, detailed commit messages are required.
It is certainly a requirement for me to feel comfortable reviewing the PR. I'm not comfortable reviewing this PR!
I cannot necessarily say any further than that, though if no one feels comfortable reviewing a PR then sometimes that ends up in the PR never being merged. Jussi is the project lead, though.
I will say that usually I don't check for every commit to pass linters or typechecking, though I do find it distracting to review when individual commits "walk all over" each other due to retroactive linter fixing. No one bisects a codebase to check if linters pass on each commit -- I have stronger feelings about tests because people do bisect and run the tests to see which commit breaks something.
While it's possible to tell git bisect to treat an entire merge as one bisectable unit, and you can bisect before/after a giant refactor lands, this still does mean that you're basically faced with 3k lines of "somewhere in here, a bug happened". Jussi is saying there is no other choice -- that's not a decision I myself would make.
For the record, I hate having to put up this patchset for review as much as everyone hates having to look at it (possibly more). But this is a major feature that does something that no other build system has ever done (that I know of at least). The unfortunate thing iis that I had to work for several months of my unpaid free time to get even this far. Even that was a massive slog. Doing the whole thing "by the book" would have lead to the whole thing never getting finished and me suffering from a massive burnout.
All of that said, the commits do have a logical structure (of sorts). Maybe 10-20% of commits actually change functionality, while the remaining ones just fix things all around the the code base to make the tests pass. So review-wise you should be able to mostly ignore all commits with messages like "fix xxx in yyy". Those are ancillary.
Perhaps the biggest logical change which is not obvious is that OptionKey changes ins semantics so that if ssubproject is the empty string, then it means the top level project but if it is None, it means a global Meson builtin option.
Jussi is saying there is no other choice -- that's not a decision I myself would make.
Of course there is a choice. What I'm saying is that it would require resources (time and personnel) we just don't have. Which totally sucks, but as the old saying goes, you go to battle with the army you have, not the one you wish you had.
And in case people msised it, I wrote a blog post about this very issue.
I went trough the commits to see if anything could be merged in isolation. I could not really find any. Here is some reasoning and motivation to hopefully explain why that is.
In the existing code option values are enriched (for lack of a better term) in several different place. For example build targets "know" what the values for each option are. This used to work fine, but does not work any more for this new requirement. The code needs one and exactly one place where we can ask "given this target in this subproject, what is the value of option foo ". That place is the OptionStore. It also improves debuggability since you can add breakpoints to events like creating a new option or setting their value. With the current implementation this is impossible, since the location of ground truth is spread all around (with the proxy classes and such).
The responsibility of all configuration code is to pass their input data unchanged to the OptionStore object. For example there is the set_from_configure_command method that has all the special case code needed to set things from the command line. All of these separate setups have their own named methods and they are all in one file. Optionstore has also been written so that it should have no dependencies to other Meson internals so you can write unit tests for it (as there already are in this MR). That makes development and debugging again much easier and faster.
This patch set has a lot of commits in it but basically it boils down to "rip out old implementation and add the new" followed by a ton of "keep fixing issues it created". Conceptually this could be a single commit but that would be unwieldy and hard to review. This seems to be pretty much unavoidable when you need to change something this fundamental that touches every part of the code base.
And one thing I forgot to mention was that once this is in and stabilized, we can add the possibility to override options not just on a per-subproject basis but on a per-target basis. So you can, for example. disable werror for a single build target somwhere in your subproject tree without needing to edit its build files.
Another thing to consider is that regardless of how much this is tested, it will most likely cause regressions. Maybe we just have to do the merge as a flag day, ask people to test it on their stuff and keep fixing things until they are stableish?
One more things to note is that we can add a subproject_default_options kwarg to project so that projects can specify only the things needed when built as a subproject (such as setting c_std to gnu99 or smth).-
- The optstore is already approaching being a "divine" object. I'd like to be careful, because in many ways Interpreter and Environment are already so big and powerful,
That is somewhat true, but on the other hand it is quite self contained. It handles one and only one thing: option storing and evaluating. It also moves functionality away from Interpreter and Environment because they no longer need (or should need) to process options. They just pass user data directly to the option store and query it for values.
I'm going to say again, for the last time because I don't feel like I actually have any power here that:
- I'm still not convinced that I like the command line, and I really wish that this had been developed as a number of smaller series that would have allowed better review. Specifically splitting the refactoring of the implementation and the changes to the command line. But that's water under the bridge at this point.
- It's really frustrating that I feel like when I (who also have limited Meson development time) send huge series they are expected to meet the same standards as all other series, ie: each patch must be complete, and work without regression, while making one logical change.
- I still think this is a terrible approach. the optstore is a divine object antipattern, you can't do anything with options without it, not even calculate the key that an option can use. So now we have to pass the optstore into everything, where previously we had numerous, small, simple objects that were self contained. It also goes back to stringly typing a bunch of stuff that used to have unique types associated with them
I don't feel like I'm going to make any headway here on my design concerns so I'll stop now. But I really feel like after this merges Eli and I are going to spend a significant amount of time fixing all of the issues that this MR introduces.
I have no more comments to make on this, so just merge it.
I said I wouldn't look at this anymore.... But I did.
I have a fundamental question about this.
Why does -A exist, which seems to only work for built-in options.
So if I want to set a subproject project option (right now) I say meson setup/configure builddir -Dsubproject:option=.... We cannot regress this (I haven't verified that this still works). But now It seems like what we'll have is: meson setup/configure builddir -Dsubproject:project=... -Asubproject:builtin=.... Which leads me to, what does -A do that -D doesn't? More specifically: meson setup builddir mesa -Dlibdrm:intel=true -Alibdrm:buildtype=release
Why does -A exist
It shouldnt? This commit should take it out as per discussion above:
Move configure option -A functionality to -D.
The first two commits in this seem to be in master with different IDs. I'll remove them.
This patch leads to crash for me, see https://github.com/mesonbuild/meson/issues/12526#issuecomment-2659625828 for details.
If you have an issue created by the option refactor changes please see if there is an open issue that matches yours and comment there, or open a new issue please.
To avoid further comments here were a new issue would be more helpful I'm locking this thread.