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

Add general override functionality. Closes #3001. [WIP] #8753

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jpakkane
Copy link
Member

@jpakkane jpakkane commented May 9, 2021

This is the very beginning of #3001. Currently only allows overriding an option value for all subprojects (not wired to the backend so does not actually work yet). Should permit at least:

  • overriding an option value (c_std) per-subproject
  • overriding options per target (i.e. optimization value for one target)
  • whether a target gets built as a shared or static lib

I currently put the definitions inside the native file. This is probably bad. Maybe we should create its own file type and argument for this? Definitely editing the override file should cause a reconfiguration and rebuild as needed.

Ideas and design requirements welcome.

@jpakkane
Copy link
Member Author

Some rethinking. Override file is now an option so it can be set from the command line allowing you to enable it to an existing build dir or switch between override files. Also added a new option type that point to files and validates that they exist.

@eli-schwartz
Copy link
Member

It would be extremely nice to be able to override dependency lookups too. In the style of autotools having $NAME_CFLAGS and $NAME_LIBS except, you know, not an environment variable ;)

@jpakkane
Copy link
Member Author

That can be added too. It's mostly a question of syntax.

@jpakkane
Copy link
Member Author

Added functionality to override an option for a single subproject rather than all of them.

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 2 alerts when merging 9838a03 into 1730778 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@dcbaker
Copy link
Member

dcbaker commented May 14, 2021

@jpakkane I had thought about dependency overrides as well. I think that probably should be in the machine file, since they're a per-machine option, my thought had bee something like:

[dependencies.zlib]
include_directories = ['/usr/local/include']
c_link_args = ['-L/usr/local/lib', '-lz']
c_args = []
; Additional language args could be placed here if you had both a C and C++ ABI

@dcbaker
Copy link
Member

dcbaker commented May 14, 2021

Also, while we're talking about it, I'd really like to have a single "machine" file, instead of separate cross and native files, an imaginary syntax could be:

[build.machine]
  system = 'linux'
[build.built-in options]
  c_args = ['-foo']
[host.machine]
  system = 'windows'
[host.dependencies.zlib]
  ...

Obviously my sort-of-toml-in-ini isn't a very nice format, but we don't have a ton of options without external dependencies unless you want to use json (I don't advice json for humans to write by hand), or write something by hand. With our already available layering then it's easy to compose them together.

Also, I have a branch that makes *_std a per-subproject option, I didn't quite get it finished, but it's probably useful since currently the standard is part of the ninja compile rule template.

@jpakkane
Copy link
Member Author

Also, I have a branch that makes *_std a per-subproject option

Actually, one of the outcomes of this PR is that we would no longer need per-subproject options. Instead they could be written in the override file. The proliferation of per-subproject options is not a nice thing and makes the number of options explode and complicate things.

@lgtm-com
Copy link

lgtm-com bot commented May 15, 2021

This pull request introduces 1 alert when merging f8529b3 into 1730778 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@dcbaker
Copy link
Member

dcbaker commented May 18, 2021

And, having two files sucks. Let's not do that. Let's have one file. just one, with one syntax, one argument, and not have a proliferation of files with slightly different formats.

@jpakkane
Copy link
Member Author

This is something I have thought about for a lot and unfortunately there does not seem to be a simple solution. First of all if you put this and cross and native files in a single file then it must be JSON or something else that has multiple hierarchical nested levels. A single flat ini-style thing becomes unwieldy if it has tens of entries. Writing JSON by hand is doable, but not that pleasant.

Further, there are cases where you'd want to switch between different override sets but keep all other info the same. As an example you'd typically want to use the setup described in the doc where the top project is debug but all others are debugoptimized. Then you might want to switch to a different set with additional debugs or warnings. Or build some individual projects as shared rather than static for build speedups. If everything is in the same file, you need to copy all the other definitions as well. If you specify multiple conf files (like with cross files) then specifying "-Doverride_file=['foo/bar', 'baz']" by hand gets tedious.

If someone comes up with a nicer UI for this, then we could and should definitely do that.

@dcbaker
Copy link
Member

dcbaker commented May 18, 2021

Realistically we have three choices:

  1. JSON
  2. YAML
  3. TOML

All of them suck in some way.

Ini stinks because it's flat, underdefined, and there's a ton of variants all called "ini".

JSON is really annoying to write by hand, since it has obnoxious rules about commas. It was designed as a human inspectable language. It's great for machine -> machine situations. I've done a ton of work with JSON, I really don't recomend it for human use. There's a python builtin.

YAML is really big and complicated, it has some odd whitespace interaction between tabs and spaces. It is, on the other hand, very flexible and thanks to github and gitlab quite popular and thus well known. There's several good implementaitons for python. It's also a strict superset of JSON.

TOML is the current darling of the world, it's nice in that it's basically a speced ini format with heiarchy. It's got some odd syntax, it can be a little verbose, but it's generally not bad to write. There's one good implementaiton for python.

The thing they all have in common is that they are structured. You can represent exactly the same data in all three formats:

{
  "a": 6,
  "b": [
    1,
    2
  ],
  "c": {
    "x": "foo"
  }
}
a: 6
b:
  - 1
  - 2
 c:
  x: 'foo'
a = 6
b = [1, 2]

[c]
x = "foo"

And, pyyaml, json, and toml all will give you exactly the same data structure. So, here's a crazy idea. We could use all of them. JSON always, and one or both of the other two if you have the necessary dependencies. That gives us a format we can always accept, even if it's very annoying to write by hand and one or two formats that are actually designed to be written by humans that we can accept in most cases (really other than boostrapping).

@eli-schwartz
Copy link
Member

Toml is flat too. Nesting is defined by encoding the full hierarchy, dot-separated, into each section name. This is not hierarchical, this is flat but with some easy-for-humans-to-mess-up convention for inferring hierarchies. (You can arbitrarily decide the same rule for your project's ini files, leading to an identical experience and no better or worse wieldiness.) It quickly becomes unwieldy.

yaml solves that problem, but does add different problems.

I'm not aware of a single truly good file format. At least configparser is well defined as being configparser, and is in the stdlib!

@dcbaker
Copy link
Member

dcbaker commented May 19, 2021

Toml is flat too. Nesting is defined by encoding the full hierarchy, dot-separated, into each section name. This is not hierarchical, this is flat but with some easy-for-humans-to-mess-up convention for inferring hierarchies. (You can arbitrarily decide the same rule for your project's ini files, leading to an identical experience and no better or worse wieldiness.) It quickly becomes unwieldy.

Whether it is unwieldy, it is speced that a . in a table name is hierarchical. I understand you don't like it, but please, the spec is super clear on this: https://github.com/toml-lang/toml/blob/master/toml.md#user-content-table

The reality is that we need a hierarchical format if we're going to have "one file to rule them all". I'd gladly pick an imperfect format (since, as you say, there is no perfect config format) to have one file with one format than have two files with different formats.

@xclaesse
Copy link
Member

Currently machine files are ConfigParser but with values interpreted for basic types such as strings, lists, additions. Note that the "ini" format leaves values undefined, so our machine files are still valid ini, just with extra formatting.

I worked a long time ago on bringing the same syntax to wrap files: xclaesse@ab1caf1. I think it's important to have the same syntax to all files formats exposed to users.

I personally would like to keep using that format if possible because that's what our users know already. With a bit of structured section names, it shouldn't be that bad, we do that already for options in machine files. If we start using json or something else maybe we should switch wrap files too for consistency.

@dcbaker
Copy link
Member

dcbaker commented May 20, 2021

I coded up a . separated syntax using our config parser, it's not bad. I need to write a few real tests and I'll push it for review.

@dcbaker
Copy link
Member

dcbaker commented May 20, 2021

@dcbaker
Copy link
Member

dcbaker commented May 20, 2021

fully backwards compatible, but allows . separate section headers like toml.

@jpakkane
Copy link
Member Author

For reference, here is a copypaste of the syntax:

[host.machine]
system = 'linux'
endian = 'little'
cpu_family = 'aarch64'
cpu = 'aarch64'

[host.built-in options]
foo = 'bar'

[host.built-in options.zlib]
default_library = 'static'

I'm not a fan of the host. prefix. Needing to write that everywhere gets tedious. Maybe make it so that the names are foo and 'cross.foo`. Thinking further, that only makes sense for machine definitions because there are, for example, no separate built-in options for cross building.

One major question here is what should be done if override and cross files are combined? I'd be inclined to say that that should be prohibited. If you specify an override file then:

  • specifying cross files is an error
  • per-subproject options can not be defined via command line flags, only by editing the override file
  • eventually we could deprecate and remove setting per-subproject options via the command line

The latter is a bit controversial, however I have found that when you have a project with something like 5 subprojects then the output of meson configure gets close to unreadable because there is just so much stuff in it. A plain text file that you can modify with an editor is more convenient, not to mention once you have written the file you can use it as a build configuration template later.

As an idea that came to mind: if the user does not specify an override file then we could write an empty one to the build dir and use that. It could have some comments at the top to tell people how to use it.

@dcbaker
Copy link
Member

dcbaker commented May 31, 2021

I would prefer to have a single file that replaces the separate files. Combing cross/native files with the new file should be prohibited.

I wonder if we could do something like:

[Host]
[@machine]
....

Where @ means "under the previous section"?, Ie that would be equvalent to [host.machine]

@xclaesse
Copy link
Member

xclaesse commented Jun 1, 2021

I personally don't see the problem in having host. prefix, we don't have that many sections anyway. But I still totally hate that we have useless split in option sections, a single [options] section can do the job.

@xclaesse
Copy link
Member

xclaesse commented Jun 1, 2021

[host.built-in options.zlib]
default_library = 'static'

That section name is starting to be ridiculous, the same can be achieved by:

[host.options]
zlib:default_library = 'static'

@ntonnaett
Copy link

This looks great. The only thing I'm missing is the coherence between subprojects and differently obtained dependencies.
This should work for both:

[host.options]
zlib:default_library = 'static'

Or there should be the possibility to declare the same for dependencies:

[host.options]
zlib:default_library = 'static'
zlib_dep:static = true

Maybe it's better to leave them separated because the behavior can differ between pkgconfig and subprojects.

@annacrombie
Copy link
Contributor

Why not have the config file be in the meson dsl? You can already evalutate some meson expressions / assign and retrieve variables.

# like json, but with expressions, variables, no weird comma rules
{
  'a': 6,
  'b': [1, 2],
  'c': {
    'x': "foo"
  }
}

# or, define with functions

set_options(
  for: 'host',
  machine: machine_options(
    system: 'linux'
    endian: 'little'
    cpu_family: 'aarch64'
    cpu: 'aarch64'
  ),
  built_in: {
    'foo': 'bar',
    'zlib:default_library': 'static',
  },
)

# or anything in between!

Pros:

  • no extra dependency to parse
  • familiar syntax by default
  • it is clear that expressions / variables can be used
  • flexible ways to compose / express hierarchies

Cons:

  • too flexible?
  • difficult / impossible for non-meson programs to parse (is this a problem?)

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.

None yet

6 participants