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

Refactor getting option values #13086

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from
Draft

Refactor getting option values #13086

wants to merge 37 commits into from

Conversation

jpakkane
Copy link
Member

@jpakkane jpakkane commented Apr 14, 2024

Currently option values for a target are store in the target object as a "view". This works for the current setup but is very inconvenient for #9398. This MR refactors things both to make things simpler and to make overriding values from the command line possible.

  • Eventually build objects shall only contain "raw data" and computing "option values" for them
  • Backends shall use the methods added to the base backend class
  • get_option shall use a similar sort of centralised method (not yet added)
  • Both of these shall (probably) use the same implementation somewhere

This is WIP and will probably fail tests on other platforms. Do not merge yet, I need to add and squash things as outlined in the first commit message before this is ready.

This needs to be merged with a merge commit, because the intermediate commits are many and definitely not working so rebasing would break bisect.

@jpakkane jpakkane force-pushed the optionrefactor branch 10 times, most recently from a6eb4b7 to 9a5f92b Compare April 20, 2024 17:49
@jpakkane jpakkane requested a review from dcbaker as a code owner April 20, 2024 17:49
@jpakkane jpakkane marked this pull request as draft April 20, 2024 18:40
@@ -669,6 +669,9 @@ def get_option(self, key: 'OptionKey') -> T.Union[str, int, bool, 'WrapMode']:
# in the future we might be able to remove the cast here
return T.cast('T.Union[str, int, bool, WrapMode]', self.options[key].value)

def get_raw_override(self, key: str) -> T.Optional(str):
Copy link
Member

Choose a reason for hiding this comment

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

T.Optional[str]

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no point in reviewing typing at this point, the methods are probably going to change before this is ready.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mostly did a cursory scan to see if there was any design stuff I was concerned about, which there isn't :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is one design question still unanswered, though. Should options defined in subproject and project become these additional options? I think they should, because then you can alter them after the fact from the command line without editing source files.

Copy link
Member

Choose a reason for hiding this comment

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

Can you give me an example of what you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well suppose you have a subproject that starts with this:

project('something', default_options: ['cpp_std=c++17'])

Should the output of configure be something like:

<default options here>

Per-project options overrides [or whatever]:

something:cpp_std=c++17

And should you be able to unset that so it reverts to the global default with:

meson configure -U something:cpp_std

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion, yes that is what should happen. This implies two different things:

  • Any "basic" option should be overriddable
  • All such deviations from the default (whether from subproject override, project setting or meson configure -A) should be stored in the same way

Note that "override" is a bad term because we already have override_options but I did not come up with a better one.

Copy link
Member

Choose a reason for hiding this comment

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

On thing that I've always wanted is the ability to override the superproject but not the subproject, so assuming mesa:

meson setup builddir -Ddebug=false -Dmesa:debug=true

so that I can easily set a default, but override the main project just like subprojects. Any chance that would work after your changes?

Copy link
Member Author

@jpakkane jpakkane Apr 23, 2024

Choose a reason for hiding this comment

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

That functionality is planned, yes. The actual syntax is not yet final, though. The one I'm currently leaning towards is this:

meson configure -A :cpp_std=c++20

would set the option on the top level project. Thus if you want to build your project so that it is in debug mode, but optimisations are enabled on subprojects only you'd do something like:

meson setup --buildtype=debug <other args>
meson configure -Doptimization=2
meson configure -A :optimization=0

These could be all in one command, I just split it into three for easier readability.

mesonbuild/compilers/c.py Fixed Show fixed Hide fixed
@jpakkane
Copy link
Member Author

This is ugly as sin, but it now finally works on Linux at least (more specifically run_project_tests.py --only common does not produce any errors).

There is no actual override ability yet, but it should be fairly straightforward to add on top of this.

This requires a bunch of cleanup before it can be considered for merging. But at least the groundwork is there.

def create_sp_options(self, A) -> bool:
if A is None:
return False
import copy

Check notice

Code scanning / CodeQL

Module is imported more than once Note

This import of module copy is redundant, as it was previously imported
on line 7
.
@jpakkane
Copy link
Member Author

jpakkane commented May 6, 2024

Now it works enough to run the brand new unit test. Do read it to get a grasp on how this is supposed to be used. Design comments welcome, actual code is missing a ton of cleanup still so maybe hold off on that unless you see something egregious.

unittests/linuxliketests.py Fixed Show fixed Hide fixed
unittests/linuxliketests.py Fixed Show fixed Hide fixed
unittests/linuxliketests.py Fixed Show fixed Hide fixed
@jpakkane
Copy link
Member Author

The unit test that checks per project overrides now passes, but a ton of other unit tests have started failing. Next I need to fix those.

It seems that this requires writing a new class OptionDatabase or somesuch that concentrates all the complexity of option handing in one place as opposed to the current approach, where the functionality is spread everywhere.

mesonbuild/options.py Fixed Show fixed Hide fixed
unittests/optiontests.py Dismissed Show dismissed Hide dismissed
mesonbuild/options.py Fixed Show fixed Hide fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
options Meson configuration options refactoring No behavior changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants