mintro: Introspect --buildoptions without a build directory #4564
Conversation
|
Since this is a big change, it will have to wait until 0.49.0 is out. |
mensinda
force-pushed
the
introBuildOpts
branch
from
fd293eb
to
3984e98
Compare
|
One thing this does not cover yet is subproject options. I think we should at least have plan for those though. With the current output format (which matches the format for the configured case, which we should try to keep) we can't even have the subproject options separate so the users of this interface need to know what is a subproject option and what is not. |
|
I don't think that we should support subproject options here. Implementing this feature would require a lot more effort and even more code duplication. From what I know we would also have to (partially) implement To your last point: It is already possible to group subproject options since they follow the naming convention |
Well we need something that parses meson_options.txt from the subprojects and exposes that. From my point of view meson can not really offer more for unconfigured projects.
I agree that we should not even try to predict which subprojects will be used. That simply is not feasible.
My point is that adding this later will break expectations of IDEs. Or we could have a separate command that just parses the subproject options files. But that seems a little odd to require two shell outs. What i imagine a UI to look like is that it per default shows the options of the main project (incl. meson options) and has some picker somewhere to add one of the subprojects at a time. So if you can't configure the main project because a subproject has a (possibly only on your setup) mandatory option you can get a dialog / section for these options. The alternative would be to force the users to find out what option is needed and it's syntax completely manually and offer just an input box for additional options. |
mesonbuild/mintro.py
Outdated
| subpr = os.path.join(subproject_dir_abs, dirname) | ||
| self.global_args_frozen = True | ||
| try: | ||
| subi = BuildoptionsInterperter(subpr, '', self.backend.name, cross_file=self.cross_file, subproject=dirname, subproject_dir=self.subproject_dir) |
There was a problem hiding this comment.
I would just use optinterpreter.OptionInterpreter here. Doing that does include options of subprojects that have a obviously bad meson.build file, but i don't think it's worth having more complicated code here just to detect that case. And also i think error reporting and parameter checking is best left to the full interpreter.
There was a problem hiding this comment.
However, this would not cover the case where a new language is added in a subproject. The options for this language would be missing in this case. So I think that keeping this extra logic is worth it.
There was a problem hiding this comment.
indeed. I didn't think of that. The language detection is useful.
And i think it's not a big problem that the IDE will not know if that language is used by the main project or a subproject.
So i agree we should keep this.
mesonbuild/mintro.py
Outdated
| if comp is None: | ||
| return False | ||
| self.add_base_options(comp) | ||
| return True |
There was a problem hiding this comment.
The return value is never used. Also we can move self.add_base_options into the branch for newly added compilers and delete the code for lang in self.coredata.compilers
There was a problem hiding this comment.
Done with the last two commits.
mesonbuild/mintro.py
Outdated
|
|
||
| options = BuildoptionsOptionHelper(cross_file) | ||
| self.cross_file = cross_file | ||
| self.environment = environment.Environment(source_root, None, options) |
There was a problem hiding this comment.
maybe this should use a environment that is shared by all subprojects?
There was a problem hiding this comment.
I would not do this, since Environment stores coredata and I want to avoid conflicts between (sub)projects.
Also, see my last comment.
mesonbuild/mintro.py
Outdated
| return | ||
|
|
||
| self.coredata.user_options.update(subi.coredata.user_options) | ||
| self.coredata.compiler_options.update(subi.coredata.compiler_options) |
There was a problem hiding this comment.
These two lines are needed because each interpreter has it's own environment and thus coredata, right?
There was a problem hiding this comment.
Yes. And having a shared coredata and / or environment may break stuff when the subproject modifies entries from the root project.
There was a problem hiding this comment.
How could that be a problem when the real interpreter does share its environment and coredate as well. (via self.environment = build.environment and self.coredata = self.environment.get_coredata() in Interpreter.init
There was a problem hiding this comment.
It doesn't: Coredata is stored in Environment which is stored in Build:
https://github.com/mesonbuild/meson/blob/master/mesonbuild/interpreter.py#L1948-L1950
And for each subproject a copy of Build is created, which is later merged:
https://github.com/mesonbuild/meson/blob/master/mesonbuild/interpreter.py#L2381
I don't exactly know what happens during the merge process and how that affects the build options.
There was a problem hiding this comment.
Yes, but that copy does not actually deep copy environment. It only copies OrderedDict·s. All other attributes of Build are just reference copied.
There was a problem hiding this comment.
Oh yes, I didn't look at copy() itself. But isn't this a bug? I mean, what happens when the subproject fails and is not required?
It should be fin for the introspection, however (unless it breaks my test case).
There was a problem hiding this comment.
I think mostly by not containing much mutable data that would need rollback. But i can not say for sure that there are no dragons in there. I don't know that corner of meson well.
|
As i don't think anyone else is currently reviewing this, can you squash this down to a state where all the initial refactorings have a dedicated commit and the new functionality is added in one (or more) commits on top of that? |
|
I am not a git mage, but I will try. Also, are you done with the review, did I forget something? I would like to keep the rewriting of the git history to a minimum :) |
I hope that i'm done. But my plan is to look at this again tomorrow when i'm less tired. If you go with a single commit for the newly added functionality, adding in a few final fixes should not be hard though. Of course someone with actual commit powers has to look at this too, but i think that would be better with a cleaned up commit structure. |
| without a configured build directory. | ||
|
|
||
| Running `--buildoptions` without a build directory produces the same output as running | ||
| it with a freshly configured build directory. |
There was a problem hiding this comment.
I think we need to update the snippet with an caveat about this outputting the options of all subprojects even when they will not be active after configuration. (And that IDEs should be careful about displaying subproject options without user request)
mesonbuild/mintro.py
Outdated
| self.subproject_dir = subproject_dir | ||
| self.coredata = self.environment.get_coredata() | ||
| self.option_file = os.path.join(self.source_root, self.subdir, 'meson_options.txt') | ||
| self.backend = backends.get_backend_from_name(backend, None) |
There was a problem hiding this comment.
It seems from the backend only the name is used. We could just store the name. Not sure if that would be an improvement
mesonbuild/mintro.py
Outdated
| self.sanity_check_ast() | ||
| self.parse_project() | ||
| self.run() | ||
| self.coredata.set_options(self.default_options) |
There was a problem hiding this comment.
There are some restrictions what default options a subproject can set. I think they are restricted to setting defaults for their own user defined options. Interpreter.set_options automatically applies the subproject prefix to these options also.
There was a problem hiding this comment.
Also done. Although this had required a bit more refactoring.
|
looks good now. |
mensinda
force-pushed
the
introBuildOpts
branch
from
3e6ea5f
to
44f8b15
Compare
|
OK. Squashed everything down to two commits. Let's hope that this didn't break anything and the CI passes. |
mensinda
force-pushed
the
introBuildOpts
branch
from
44f8b15
to
e59d757
Compare
mensinda
force-pushed
the
introBuildOpts
branch
from
e59d757
to
e509935
Compare
docs/markdown/IDE-integration.md
Outdated
| @@ -70,6 +70,15 @@ The possible values for `section` are: | |||
|
|
|||
| To set the options, use the `meson configure` command. | |||
|
|
|||
| It is also possible to get the default buildoptions without a build directory by providing the root `meson.build` instead of a build directory to `meson introspect --buildoptions`. | |||
There was a problem hiding this comment.
This should mention that this is only available since version 0.50. The line should also be rewrapped to 80 characters.
There was a problem hiding this comment.
Done with the last commit
|
Hi just noticed this rebasing a PR of mine. I really like the feature, but am concerned about the amount of new code in mintro and the degree to which it duplicates existing code. Any idea how that might be improved? |
|
I don't see a way to make this a lot simpler. There might be some parts that can be shared with other AstParser users (i.e. maybe parts of the func_project) but this PR already did move the easy code parts out of interpreter into shared code. If somebody has good ideas having this with less duplicated logic would of course be nice. But the "don't really execute" way of introspection does differ in some important parts from the normal execution flow. |
|
I am also not really a fan of the amount of code duplication in this PR. However, (as @textshell already mentioned) refactoring the remaining parts to avoid code duplication is not easy. The only real way I see to resolve this is to create a new base interpreter that implements the shared parts of the "real" and the "introspection" interpreter. Of course, this would require massive amounts of refactoring. |
|
Well at least it's all contained within Perhaps another way to do it would be to make a fake coredata and just use one interpereter. I dunno. |
|
It might be possible to move |
|
Oh awesome! That would completely solve it for my PRs, thanks! |
This PR adds the possibility to run
meson introspect --buildoptions /path/to/meson.buildwithout a configured build directory.Running
--buildoptionswithout a build directory produces the same output as running it with a freshly configured build directory.This works similar to the other #4191 but with more copy and pasted code. Unfortunately, I have not found a way to implement this in another way, without refactoring the language handling in the main interpreter.
@textshell You might be interested in this.
Update:
Added the
--backendoption to the introspector to make sure the introspected build options are exactly the same.Also did some refactoring to reduce the amount of code duplication.