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

Set project and meson options in cross/native files #6597

Merged
merged 13 commits into from
Aug 2, 2020

Conversation

dcbaker
Copy link
Member

@dcbaker dcbaker commented Feb 7, 2020

This is a partial implementation of #3001, but not everything.

This series allows meson builtins and project options to be specified in the cross and/or native files. This should make it easier to set up subprojects in a reliable way, as all of the options can be kept in a native/cross file. With file layering a projects can even ship preconfigured options, that the end user can overlay with their own cross/native files.

For mesa we'd like to couple this with the ability to set some builtin options (namely default_library) on a per-subproject basis to control wraps in a predictable way.

@dcbaker
Copy link
Member Author

dcbaker commented Feb 7, 2020

@marc-h38, I think this was what you were wanting.

@xclaesse
Copy link
Member

xclaesse commented Feb 7, 2020

I haven't read the whole thing yet, but one thing I'm wondering is why do you split options in different sections?

I was expecting a simpler format, like the one we already use in <builddir>/meson-private/cmd_line.txt. For example:

[options]
prefix = '/opt/gnome'
default_library = 'both'
ffmpeg:default_library = 'static'
glib:gtk_doc = 'disabled'

That way you can load that into a single dict that would match exactly as if the user passed them with -D.

I think one common use-case is when you have only a cross file, and I think you should be able to set all options from there. But then what happens if you have both a cross file and a native file with options defined in both? I guess we should abort with an error if the same option is set in both files. That would be similar to what we currently do when passing both --default-library and -Ddefault_library. Of course build. options must only be in cross file.

@dcbaker
Copy link
Member Author

dcbaker commented Feb 7, 2020

@xclaesse I really want to get rid of the one off format in cmd_line.txt and replace it with a native/cross file.

I haven't read the whole thing yet, but one thing I'm wondering is why do you split options in different sections?

Composition, for one. cross and native files can be composed by passing the argument more than once. Since each component is it's own section you can re-use a cross file in more than one build. Say you need zlib in two different projects, and you want to statically link it in both cases, now you can have a single zlib file.

For meson builtin versus project arguments it makes sense to split them as well, because they're conceptually different. Project arguments by definition apply only to one project, meson builtins (at least generally) apply to all projects, so being able to specify them separately makes sense to me.

But then what happens if you have both a cross file and a native file with options defined in both?

This is clearly defined. If you have both a cross and native file project options from the native file are ignored, meson options that have a per-machine value like pkg_config_path are read from the native-file and applied to the build machine. values from the cross file are applied to the host arguments.

@dcbaker dcbaker changed the title Full project config Set project and meson options in cross/native files Feb 7, 2020
@jpakkane
Copy link
Member

jpakkane commented Feb 7, 2020

I'm so glad you posted this, because I was going to start working on it myself today. :)

It would be great to get all functionality needed for configuration so we can merge #6595 to get a general solution for all options rather than a one-off feature for one of them. One possible syntax change comes to mind. Rather than doing this:

[meson options]
c_std = 'c99'
[zlib:meson options]
default_library = 'static'

could we instead do this:

[meson options]
c_std = 'c99'
zlib:default_library = 'static'

@dcbaker
Copy link
Member Author

dcbaker commented Feb 7, 2020

I thought about that, but I really dislike it because it means you have to repeat yourself a lot if you have more than one options:

[meson options]
zlib:foo = 'bar'
zlib:bar = 'foo'
zlib:thing = false

vs

[zlib:meson options]
foo = 'bar'
bar = 'foo'
thing = false

And on the implementation side this makes things easier because we know what subproject we're in when we're applying defaults, so we load the right options from the start and don't have to loop over them asking "are you a zlib option?"

It would be great to get all functionality needed for configuration so we can merge #6595 to get a general solution for all options rather than a one-off feature for one of them

I've based this on @xclaesse's series, I'm not sure exactly what parts you don't like, but all of the machinery he put in place except in the interpreter changes is needed to make per-subproject builtins work. The language options are more complicated because they're generated dynamically. I have a branch for that, but there are still bugs and no tests. It's more complicated because, again, language options are added when a language is added to meson, we don't by default have c_std and friends. It also exposes all of the language args, and I'm not sure that's what we really want.

@jpakkane
Copy link
Member

I'm not sure exactly what parts you don't like, but all of the machinery he put in place except in the interpreter changes is needed to make per-subproject builtins work

Per-project values of options should only be settable via the native/cross file. There are usually more than one of these, so concentrating them all in one place makes it easier to review.

@dcbaker
Copy link
Member Author

dcbaker commented Feb 12, 2020

Per-project values of options should only be settable via the native/cross file. There are usually more than one of these, so concentrating them all in one place makes it easier to review.

I think we should be able to set them via the command line. We allow setting project options on the command line, and we're going to get no end of complaints about -Dsub:foo=c works but -Dsub:c_std=c99 doesn't.

@nirbheek
Copy link
Member

We allow setting project options on the command line, and we're going to get no end of complaints about -Dsub:foo=c works but -Dsub:c_std=c99 doesn't.

It's also going to be very inconsistent to not be able to set arguments on the command-line like that. We already have a bunch of unavoidable inconsistency in our argument handling, let's not increase it.

Setting arguments on the command-line vs a file is also much clearer for CI purposes since it's right there in the yaml file, and it shows up in the CI logs for debugging.

@scivision
Copy link
Member

scivision commented Feb 19, 2020

Would this make it possible to set the Ninja binary location as well in a native file, to avoid use of NINJA or PATH environment variable?

@dcbaker
Copy link
Member Author

dcbaker commented Feb 25, 2020

@scivision That would be a separate change, because I don't think we need any infrastructure changes for that, we just need to find ninja using the find_program implementation.

Copy link
Contributor

@marc-h38 marc-h38 left a comment

Choose a reason for hiding this comment

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

I thought the only conceptual difference between "meson build options" and "project options" was: meson options apply globally to all (sub)projects whereas project options are (sub)project specific. No conceptual difference when only one project. Simple, but now I'm confused by these:

For meson builtin versus project arguments it makes sense to split them as well, because they're conceptually different. Project arguments by definition apply only to one project, meson builtins (at least generally) apply to all projects
...
You can set some meson level options on a per-subproject basis (!)

Emphasis mine.

I've read https://mesonbuild.com/Build-options.html (please rename to "meson options") and https://mesonbuild.com/Reference-manual.html#project a good few times and I'm still confused.

While we're very far from CMake yet, I'm starting to have this sinking feeling: #5859 (comment).

While this, #3001, #4637, #5061, etc. all seem very useful, I'm worried they will increase the configuration possibilities and complexity before clearly and briefly documenting the high-level and already existing concepts behind them. BTW "whatever is well conceived is clearly said, and the words to say it flow with ease." So either the difference between meson level options and project level options can be stated in one or two completely unambiguous sentences, or there's some design issue with them.

I understand life is not simple and build systems much less, however it should at least be possible to describe the top-level concepts and features clearly and briefly. Maybe I only found low-level / reference documentation and missed these high-level descriptions? In that case apologies and thanks for the pointer.

Feeling of sinking much deeper with c_args #6362

Native files allow layering (cross files can be layered since meson 0.52.0).
More than one native file can be loaded, with values from a previous file being
overridden by the next. The intention of this is not overriding, but to allow
composing native files.
Copy link
Contributor

Choose a reason for hiding this comment

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

While overriding is not the intention, it's still allowed correct? For instance you could have a clang-common.ini with -foptimization-fubar=true but -foptimization-fubar=false in clang5.ini and only in clang5.ini because clang 5 is broken for that feature. This already works BTW.

@xclaesse
Copy link
Member

[meson options]
c_std = 'c99'
zlib:default_library = 'static'

I still strongly prefer that syntax over splitting into different sections. We use the subproject:opt syntax everywhere, it's consistent with command line options and with output of meson configure. It also can trivially be parsed into a dict used as default values to options.cmd_line_options, we already have that code when we parse cmd_line.txt.

I also do not agree this supersede cmd_line.txt, we still have to remember options set from command line and from meson configure -Dfoo=bar. Both concepts are complementary, and IMHO should share as much code and file format as possible.

Ultimately what I would like to see happening is projects shipping a set of cross files with their source code, that contains default values to build for specific targets (e.g. Android NDK, Linux distro mingw, etc). That often means specific values for a set of options, but still allowing them to be overriden from command line by the user.

@xclaesse
Copy link
Member

Also, nitpicking, but I don't think we need "meson" in [meson options], we know it's a meson file. We don't have [meson binaries], etc.

@marc-h38
Copy link
Contributor

Also, nitpicking, but I don't think we need "meson" in [meson options], we know it's a meson file.

While pretty much everything around here is meson-related and the term "meson" is almost as vague and overloaded as "build", I suspect [meson options] was a relatively specific hence useful reference to meson_options.txt, was it not?

@xclaesse
Copy link
Member

I suspect [meson options] was a relatively specific hence useful reference to meson_options.txt, was it not?

meson_options.txt only contains project options, so that would be a misleading reference since in the current proposal [meson options] actually does NOT contain project options, afaiu.

@marc-h38
Copy link
Contributor

meson_options.txt only contains project options,

Mmmm... so this PR wants to add [project options] and [meson options] and out of these two the one that is more closely related to meson_options.txt is... [project options]?!

meson_options.txt only contains project options,

... yet meson_options.txt is not mentioned in https://mesonbuild.com/Reference-manual.html#project , only in https://mesonbuild.com/Build-options.html ?!

Am I really the only one worried about the concepts and terminology? As a sanity check, do you guys try from time to time to discuss these terms and concepts with developers not familiar with meson?

@xclaesse
Copy link
Member

Am I really the only one worried about the concepts and terminology? As a sanity check, do you guys try from time to time to discuss these terms and concepts with developers not familiar with meson?

I'm glad you volunteer to improve our documentation, there is certainly a lot to do :D

@marc-h38
Copy link
Contributor

@xclaesse to a reasonable extent I do volunteer to improve the documentation, in fact I already started that a little bit in other places. However no one can improve the documentation if developers use the terms "meson options" and "project options" in one place but vice-versa in another (really?), in that case it's a lost cause.
Before writing any documentation, the design must have clearly defined concepts and language first. Super vague words like "build" and "meson" are not good names to start with. I also volunteer to find and suggest names (if not too late) but the high-level concepts must be summarized in a few sentences first, otherwise the concepts are flawed anyway.

@xclaesse
Copy link
Member

I never checked what the doc says exactly. From meson configure there are those terms: Core options, Backend options, Base options, Compiler options, Directories, Testing options, and project options. core+directories+testing are often referred as builtin options (that's how they are named in the code) because those options are always present.

meson_options.txt are project options because they are the options defined by the project, and not defined by meson itself.

@dcbaker
Copy link
Member Author

dcbaker commented Feb 26, 2020

I still strongly prefer that syntax over splitting into different sections. We use the subproject:opt syntax everywhere, it's consistent with command line options and with output of meson configure. It also can trivially be parsed into a dict used as default values to options.cmd_line_options, we already have that code when we parse cmd_line.txt.

And I'm strongly in favor of having separate sections for each project. I think it makes sense because of composition (ie, mesa will probably ship a config file for just zlib for windows, but unless you're doing a windows->windows build you'll need to compose it with another cross file) and it makes it clear, at a glance, exactly what projects are being modified by the file.

Also, nitpicking, but I don't think we need "meson" in [meson options], we know it's a meson file. We don't have [meson binaries], etc.

How about "builtin options" then? I'd like to make it clear that one section is for meson level arguments, and the other is for options defined in your meson_options.txt, which I guess should be called "build options" as it is in the docs?

@xclaesse
Copy link
Member

And I'm strongly in favor of having separate sections for each project. I think it makes sense because of composition (ie, mesa will probably ship a config file for just zlib for windows, but unless you're doing a windows->windows build you'll need to compose it with another cross file) and it makes it clear, at a glance, exactly what projects are being modified by the file.

Composition works just fine with a single section, they will be merged.

Also, nitpicking, but I don't think we need "meson" in [meson options], we know it's a meson file. We don't have [meson binaries], etc.

How about "builtin options" then? I'd like to make it clear that one section is for meson level arguments, and the other is for options defined in your meson_options.txt, which I guess should be called "build options" as it is in the docs?

The simple fact that it's hard to name is already a proof we should not split options, because whatever name we pick it will not be understood by the user without reading long documentation. If we insist on splitting options, it should at least follow the naming we have in meson configure, so I would call them [core options] and [project options]. For subproject options maybe e.g. [zlib options].

@dcbaker
Copy link
Member Author

dcbaker commented Feb 26, 2020

Composition works just fine with a single section, they will be merged.

Certainly, but if you have three files

[zlib:builtin options]
c_std = 'c99'
c_args = ['-someopt']
[expat:builtin options]
c_std = 'gnu99'
[meson options]
c_std = 'c11'
cpp_std = 'c++11'

It's super clear what is being done in each one, the first only only affects zlib, I can see that just by reading the section header, the second one only affects expat, and the thrid only affects the primary project. compare that to:

[builtin options]
zlib:c_std = 'c99'
zlib:c_args = ['-someopt']
[builtin options]
expat:c_std = 'gnu99'

Where I can't tell at a glance that a file affects more than one project, but lets have a bigger more complicated example:

[zlib:builtin options]
c_std = 'c99'
c_args = ['-foo', '-bar', 'something else']
b_ndebug = true
b_lto = false

[expat:builtin options]
c_std = 'gnu99'
c_args = ['-foo']
c_link_args = ['-Wl,--arg']
b_ndebug = true

[builtin options]
b_ndebug = true
c_std = 'c11'

vs

```ini
zlib:c_std = 'c99'
zlib:c_args = ['-foo', '-bar', 'something else']
zlib:b_ndebug = true
zlib:b_lto = false
expat:c_std = 'gnu99'
expat:c_args = ['-foo']
expat:c_link_args = ['-Wl,--arg']
b_ndebug = true
c_std = 'c11'

The first way is much more readable to me, and it forces you to keep per-project options sorted, you would end up with

[builtin options]
c_std = 'c99'
zlib:b_ndeug = false
c_args = ['foo', 'bar']
c_link_args = ['-Wl,-something']
expat:c_std = 'c11'
zlib:c_args = ['-foo']
expat:b_ndebug=false

The simple fact that it's hard to name is already a proof we should not split options, because whatever name we pick it will not be understood by the user without reading long documentation. If we insist on splitting options, it should at least follow the naming we have in meson configure, so I would call them [core options] and [project options]. For subproject options maybe e.g. [zlib options].

That's a terrible argument. Naming things is one of the hardest things in computer science, by that logic no software should be written at all!

But I don't think we're going to solve this by arguing about it anymore, clearly I like blue bikesheds and you like yellow ones. Maybe some other people have opinions?

@xclaesse
Copy link
Member

zlib:c_std = 'c99'
zlib:c_args = ['-foo', '-bar', 'something else']
zlib:b_ndebug = true
zlib:b_lto = false
expat:c_std = 'gnu99'
expat:c_args = ['-foo']
expat:c_link_args = ['-Wl,--arg']
b_ndebug = true
c_std = 'c11'

Exactly what I have in mind, pretty readable to me, it's a matter of coding style, like in any language. It's also exactly what I already have in script that calls meson with all options set.

But I don't think we're going to solve this by arguing about it anymore, clearly I like blue bikesheds and you like yellow ones. Maybe some other people have opinions?

Indeed, I think it's pretty clear we'll never agree, so we need others to vote :-)

@marc-h38
Copy link
Contributor

marc-h38 commented Mar 2, 2020

I'm glad you volunteer to improve our documentation, there is certainly a lot to do :D

Monday morning, fresh mind let's give it a go.

I found the name "User" in the first sentence of that page, didn't take me much effort. "User" is IMHO a relatively good negation of "built-in" but please suggest better ones.

Nothing gets called "build options" because in a meson context, it's hard to come with a name more vague, less "searchable" and generally less useful than "build".

In the same meson context, the name... "meson" (!) options is slightly less vague but barely. The name "meson BUILTINS" is tolerated in the rare case of potential confusion with some other sort of builtins.

BTW "build" is already used in the build/host/target autoconf triplet which is very relevant to... cross files! Another, huge reason not to overload it with anything else in the same area. It's a bit too late to change autoconf names, like 30 years late. There's also a "build step", a "build directory" and I bet we could find other "build"/nameless things (most of them also much older than meson).

In case anyone doubts "build" is an empty placeholder, see how "build options" sometimes refers to not-builtins-options, example here: https://mesonbuild.com/Builtin-options.html

two kinds of options: build options provided by the build files and built-in options that..

In some other places "build options" refers to ALL option kinds! Example: https://mesonbuild.com/Configuring-a-build-directory.html

Since 0.50.0, it is also possible to get a list of all build options by invoking meson configure

Global options such as buildtype can only be specified in the master project, settings in subprojects are ignored....

https://mesonbuild.com/Subprojects.html#build-options-in-subproject

The names "Project and "Global" are forbidden outside subproject-related topics to avoid overloading and intolerable confusion.

I really, really hope I didn't misunderstand the concepts here. If I have please correct the concepts and do not discuss the names I just suggested as gold standards, that would be pointless. I may have misunderstood the concepts because... they don't have good, standard names yet :-)

https://martinfowler.com/bliki/TwoHardThings.html

@dcbaker
Copy link
Member Author

dcbaker commented Mar 4, 2020

@nirbheek, @jpakkane, do either of you have strong opinions regarding the "one section per project" or "one sections for all projects" debate that @xclaesse and I are having?

@dcbaker
Copy link
Member Author

dcbaker commented Mar 4, 2020

Once we have the big bikesheding debate worked out I'll also rename the sections to match our documentation, builtin-in options and user options.

Which is super helpful in debuggers
This puts all of them together, in the next patch they'll be pulled back
out, but it's convenient to start that refactor by moving them all
there, then moving them into env as a whole.
This creates a full set of option in environment that mirror those in
coredata, this mirroring of the coredata structure is convenient because
lookups int env (such as when initializing compilers) becomes a straight
dict lookup, with no list iteration. It also means that all of the
command line and machine files are read and stored in the correct order
before they're ever accessed, simplifying the logic of using them.
@dcbaker
Copy link
Member Author

dcbaker commented Aug 2, 2020

Okay, I think I have them all resolved now.

Copy link
Member

@mensinda mensinda left a comment

Choose a reason for hiding this comment

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

Yes, LGTM.

@nirbheek
Copy link
Member

nirbheek commented Aug 2, 2020

Test failure is unrelated.

@dcbaker
Copy link
Member Author

dcbaker commented Aug 2, 2020

I'm going to merge then. \o/

@dcbaker dcbaker merged commit 3040f3c into mesonbuild:master Aug 2, 2020
@dcbaker dcbaker deleted the full-project-config branch August 2, 2020 17:03
@xclaesse
Copy link
Member

xclaesse commented Aug 2, 2020

@dcbaker I'm sorry I've been too arch last week. I know you worked a lot on this PR but to be honest I've said many times I don't like neither the syntax nor the implementation. I have suggested multiple times a much simpler implementation that could have been merged months ago. I personally need this feature and like it a lot, but I'm not the one to give the +1 on it given that I'm not happy with how it's done.

That being said, cool that it got merged, I'll read that code while rebasing my branch.

@xclaesse
Copy link
Member

xclaesse commented Aug 3, 2020

You got PKG_CONFIG_PATH wrong, the env override value from native file, and it warn uselessly on reconfigure if you have the value in native file. The warning should be removed, it never worked (was dead code before this PR). It requires more work to actually do it properly. I'm preparing a fix.

@mensinda
Copy link
Member

mensinda commented Aug 3, 2020

Wait, do I understand this correctly: Env variables override stuff in native/cross files? If yes, why?

@xclaesse
Copy link
Member

xclaesse commented Aug 3, 2020

It's a bug in this PR, I'll fix it.

@mensinda
Copy link
Member

mensinda commented Aug 3, 2020

OK, thanks for clarifying. I thought this was a general thing in meson for a second :)

@xclaesse
Copy link
Member

xclaesse commented Aug 3, 2020

Actually, a comment suggests it's intentional: https://github.com/mesonbuild/meson/pull/6597/files#diff-638d32e2abc720f45ee2279d5befe1e5R690

@dcbaker what's the rational to make that change? Seems weird that -Dpkg_config_path=foo overrides PKG_CONFIG_PATH but not setting pkg_config_path in machine file.

@xclaesse
Copy link
Member

xclaesse commented Aug 3, 2020

I don't think test_builtin_options_subprojects_inherits_parent_override() is correct, and I think it's a regression.

Before this PR if you run meson builddir -Ddefault_library=both in test cases/common/230 persubproject options it pass, but with this PR it raises ERROR: Assert failed: Parent should override default_library. At least it's consistent with the behaviour when setting default_library=both in the machine file, but I'm not sure it's the right behaviour.

@Ericson2314
Copy link
Member

I did also think the precedence we CLI > config files > env vars.

@xclaesse
Copy link
Member

xclaesse commented Aug 3, 2020

test cases/common/230 persubproject options also now print that warning:

|WARNING: In subproject sub1: Unknown options: "sub1:build.pkg_config_path, sub1:pkg_config_path, sub1:prefix"

@xclaesse
Copy link
Member

xclaesse commented Aug 9, 2020

It also print that warning when the main project sets options in project():

|WARNING: In subproject sub1: Unknown options: "sub1:buildtype, sub1:cpp_eh, sub1:cpp_std, sub1:prefix, sub1:sub1:test_option"

@xclaesse
Copy link
Member

xclaesse commented Aug 16, 2020

@nirbheek @Ericson2314 Since you gave approval to this PR, did you make a in depth review of it and approve this change? Do you agree with @dcbaker that his approach is better and more maintainable than #7293? Since I get no support from anyone when pushing for a simpler solution, and if it is a well thought decision, then I'll have to accept the decision of the majority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants