Allow overriding programs from the command line #12623
Conversation
|
cc @anarazel. This could help us to remove the options in the Postgres build. |
|
I plan on adding a test in the future if people seem to think that this is a good approach. @jpakkane had previously said he was open to such a feature, but people have different opinions as time change. |
|
Hmm not sure how to fix the CI at the moment. I must be missing something with Coredata. Any help is appreciated! |
|
@tristan957 what about instead writing a web page in the site which do a comparison of usages between meson and autotools ? I don't think it's a good idea to add several ways to do the same things I've been using the autotools during more than 10 years. When I've heard about meson, i've used it quite easily. There are examples, community is responsive. It should be sufficient |
But we already have several ways to do everything else. Every command-line Furthermore, it's a frequent pain point of users, to have to create a whole new file just to pass one tiny little tweak to meson. |
I've tried advocating for machine files in Postgres for this purpose, but it was said it was a worse solution than this, so here we are, me proposing a solution which makes the transition from autotools to meson a little bit easier. |
| @@ -59,6 +61,14 @@ def add_arguments(parser: argparse.ArgumentParser) -> None: | |||
| 'newer version of meson.') | |||
| parser.add_argument('--clearcache', action='store_true', default=False, | |||
| help='Clear cached state (e.g. found dependencies). Since 1.3.0.') | |||
| parser.add_argument('--native-program', | |||
There was a problem hiding this comment.
I not tied to these argument names
mesonbuild/coredata.py
Outdated
| for a in aliases: | ||
| parts = a.split('=', 1) | ||
| if len(parts) == 1: | ||
| pass | ||
|
|
||
| alias_map[parts[0]] = parts[1] |
There was a problem hiding this comment.
What about
| for a in aliases: | |
| parts = a.split('=', 1) | |
| if len(parts) == 1: | |
| pass | |
| alias_map[parts[0]] = parts[1] | |
| alias_map.update(p.split('=', maxsplit=1) for p in aliases if '=' in p) |
There was a problem hiding this comment.
Wouldn't this call update needlessly?
There was a problem hiding this comment.
Huh? What do you mean? It iterates through aliases, test for the presence of =, split it, and add it to the dict, just like your for loop...
There was a problem hiding this comment.
The difference is that when aliases is empty, a for loop doesn't enter the block body, while alias_map.update(generator_expression) calls the .update() method with no values.
That being said, it's a good point to do:
for a in aliases:
if '=' in a:
parts = a.split('=', 1)
alias_map[parts[0]] = parts[1]
This was changed in 751f03c, but the type annotation was not correct when it was added.
|
The codeql error is rather interesting. |
tristan957
force-pushed
the
program
branch
4 times, most recently
from
57c9908
to
5c25216
Compare
mesonbuild/msetup.py
Outdated
| self.options.cross_program = [f'{k[0]}={v[1]}' for k, v in env.coredata.get_program_overrides(MachineChoice.HOST).items()] | ||
| self.options.native_program = [f'{k[0]}={v[1]}' for k, v in env.coredata.get_program_overrides(MachineChoice.BUILD).items()] |
There was a problem hiding this comment.
In terms of readability this feels somehow inscrutable.
It also unfortunately doesn't actually get dumped to cmd_line.txt at all.
There was a problem hiding this comment.
There was a bug. These shouldn't have been indexed. Let me know what you think
| native-program | ||
| cross-program |
There was a problem hiding this comment.
@eli-schwartz do we allow passing machine files in mconfigure? Seems like not. Should these options match what the other options do?
People coming from autotools are used to having easy access for overriding programs. This should ease that transition.
|
I have conflicting feelings about this MR, since we already have machine files for exactly this use case. On one hand I can see how it might be nice under some circumtances but OTOH having more than one way of doing something has a tendency to come back and bite you in the ass. |
|
Well that is unfortunate since you initially expressed interest in it. All I can tell you is that machine files are not satisfactory for the powers that be in Postgres. |
There was a problem hiding this comment.
I'm with @jpakkane that I'm torn on this, since now we have three ways to override programs, env vars, machine files, and now command line.
I'd really like to see the order of env, machine file, and command line specified (with the same precedence we have for CFLAGS, -Dc_args and c_args in the machine file, along with tests for that.
| def __parse_program_overrides(options: SharedCMDOptions, for_machine: MachineChoice) -> OrderedDict[str, str]: | ||
| overrides_map: OrderedDict[str, str] = OrderedDict() | ||
| overrides: T.List[str] | ||
| if for_machine == MachineChoice.BUILD: |
There was a problem hiding this comment.
| if for_machine == MachineChoice.BUILD: | |
| if for_machine is MachineChoice.BUILD: |
People coming from autotools are used to having easy access for overriding programs. This should ease that transition.