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

ENH: pass through config_settings coming from build frontends #122

Closed
wants to merge 1 commit into from

Conversation

rgommers
Copy link
Contributor

@rgommers rgommers commented Jul 31, 2022

Closes gh-54

This came up when Spack tried to package SciPy 1.9.0 (see spack/spack#31810). It turned out that most of the infra is already there, config_settings simply needed plumbing through. I have tested that this works for SciPy, when building a wheel against a non-default BLAS/LAPACK.

@FFY00 in gh-54 you said "I am struggling to decide on the design", but as far as I can tell there is no UI design aspect here? This is up to build front-ends, all a backend has to do is pass on the config_settings dict that is passed into build_sdist and build_wheel?

I haven't added tests yet, I thought I'd first put this up for feedback.

EDIT: config_settings is part of PEP 517: https://peps.python.org/pep-0517/#config-settings

@rgommers rgommers added the enhancement New feature or request label Jul 31, 2022
@rgommers rgommers requested a review from FFY00 July 31, 2022 11:20
@FFY00
Copy link
Member

FFY00 commented Aug 10, 2022

Thanks for opening this. Unfortunately, it is not that simple 😅. Blindly passing the options will cause breakage if you mess with any of the options we rely on. That said, the -D option would not be an issue, so we can allow that option for now and then work on the other ones.

@FFY00 in gh-54 you said "I am struggling to decide on the design", but as far as I can tell there is no UI design aspect here? This is up to build front-ends, all a backend has to do is pass on the config_settings dict that is passed into build_sdist and build_wheel?

Well, we do multiple meson calls! Currently, we do setup, compile, and install. So, there are indeed some design decisions there. There is also specifying the extra Meson arguments in the config and then adding/overriding them on the call.

@eli-schwartz
Copy link
Member

Blindly passing the options will cause breakage if you mess with any of the options we rely on. That said, the -D option would not be an issue, so we can allow that option for now and then work on the other ones.

I think the only option you rely on is -Dprefix={sys.base_prefix} (yes, that can be a -D option)?

Well, we do multiple meson calls! Currently, we do setup, compile, and install.

Only the meson setup program is relevant here, I guess. :) The other two programs merely happen to share a multi-call program launcher.

@FFY00
Copy link
Member

FFY00 commented Aug 10, 2022

I think the only option you rely on is -Dprefix={sys.base_prefix} (yes, that can be a -D option)?

IIRC changing the prefix should only be an issue in the heuristics code paths, but yeah, we do set that. We also use --native-file, so we cannot let users override that, at least blindly. Similarly, cross builds will need some extra handling.

Only the meson setup program is relevant here, I guess. :) The other two programs merely happen to share a multi-call program launcher.

Yes, but I would want to allow users to pass --{ninja,vs,xcode}-args to meson compile if they need, or tweak --jobs and --load-average.

In meson install, some users might want --skip-subprojects, but I am still not entirely sure how to handle that. See #35 for eg.

@eli-schwartz
Copy link
Member

We also use --native-file, so we cannot let users override that, at least blindly.

These will simply stack, so passing your native file as the very last option would work and override any settings the user specified.

TBH I'd want to configure the python installation being compiled for 🤷‍♂️ but that might be challenging for build requirements.

@FFY00
Copy link
Member

FFY00 commented Aug 10, 2022

These will simply stack, so passing your native file as the very last option would work and override any settings the user specified.

Yep, though the UX of quietly overriding it is not great, so we'd probably want to try detecting those situations and either throw an error or a warning. Is there anything fancy in the syntax that would make parsing not trivial?

TBH I'd want to configure the python installation being compiled for man_shrugging but that might be challenging for build requirements.

Well, that goes against PEP 517 😅. For cross builds, I have thrown around the idea of defining a new hook specifically for that, so that build backends may support that use case correctly. That needs a new PEP though, so I am not sure when I'll get around to do it.

@eli-schwartz
Copy link
Member

Yep, though the UX of quietly overriding it is not great, so we'd probably want to try detecting those situations and either throw an error or a warning. Is there anything fancy in the syntax that would make parsing not trivial?

The file must be a valid configparser file, but the RHS of a binaries entry can have a limited amount of meson.build syntax. mesonbuild.coredata -> class MachineFileParser(configparser.ConfigParser) is the reference implementation. Off the top of my head, the things which might actually be expected to see in a binaries entry are:

  • [] arrays (such as cpp = ['ccache', 'g++'])
  • + concatenation
  • / path joining
  • variable references to a constants entry, e.g. a target triplet

Since you don't actually care about the contents, only whether the key exists, you can simply load it as a configparser file, and check the names of binaries being set in cfg = ConfigParser(); cfg.read(...); cfg['binaries'].keys().

@rgommers
Copy link
Contributor Author

It's indeed the case that the vast majority of options need to go to setup. This PR as it stands does not break anything that is possible today, and just adds that capability. Filtering config_settings to pass on known supported options to compile / install steps should not be hard to add, your suggestions in https://github.com/FFY00/meson-python/pull/122#issuecomment-1211366152 seem like the right ones to me.

The optimization and debug levels we use can be overridden, that requires custom handling.

Yep, though the UX of quietly overriding it is not great, so we'd probably want to try detecting those situations and either throw an error or a warning.

I'd throw an error for --native-file for now, and leave any parsing till the time someone needs it (let's open a new enhancement issue to track).

That said, the -D option would not be an issue, so we can allow that option for now and then work on the other ones.

This isn't a great approach. Each time someone needs to pass something else to setup, they're going to run into a problem, come here to open an issue, and then wait for a new release. Unless there's a good reason to not pass everything on to setup, we should do that instead.

@rgommers
Copy link
Contributor Author

@FFY00 and I had a chat about this PR on Friday. One issue @FFY00 pointed out was that if any new options are added to the build or install stages in Meson, those are not supported through the UX proposed in this PR until they are explicitly handled. That shouldn't happen often, but it is a small downside.

I now understand better the alternative he had in mind: explicitly letting the user define which options go to setup, which to build and to install. That is more comprehensive and requires less special handling of options to redirect them.

The flip side of that coin is that the UX is going to be more verbose. It's not clear exactly what that UX should be yet. The pip and build --config-settings docs say it must be key=value pairs as the argument:

So likely something like:

-config-settings --setup-opt-key=value
-config-settings --build-opt-key=value
-config-settings --install-opt-key=value
# or shorter (can be with or without a space after -C):
-C--setup-opt-key=value
-C--build-opt-key=value
-C--install-opt-key=value

# With concrete options:
-C --setup-opt--Dblas=blis
-C --build-opt--n=8
-C --install-opt---strip

There's a couple of different ways of spelling this (e.g. --setup-key instead of --setup-opt-key, and one could automatically prepend one dash to key so it'd be --setup-Dblas=blis rather than --setup--Dblas-blis). The principle is clear though.

Opinions pro or con this idea? If separate setup/build/install stages, preferred way of spelling it?

@eli-schwartz
Copy link
Member

if any new options are added to the build or install stages in Meson

The install stage does have some fine-tuning you can do to it, e.g. you can define a subset of tags to be installed, or skip subprojects. I'm not sure how useful either one is for wheels though. There's also --strip, but that duplicates the setup option of the same name.

In general, it seems plausible that further options might be added.

In comparison, the build stage is just "run ninja". It is both more and less interesting -- there are no options to fine-tune the resulting wheel, but several options to control the resources used (parallelism, etc.)

But, the build stage is also probably best controlled by e.g. $SAMUFLAGS which is already a supported interface for the c89 Unix-only version of ninja, assuming that leaving out build stage support would allow reducing the complexity of config_settings (it might not).

@rgommers
Copy link
Contributor Author

rgommers commented Sep 4, 2022

It turns out mesonpep517 already has this: https://thiblahute.gitlab.io/mesonpep517/pyproject.html#mesonoptions. It has

--setup-args
--dist-args
--install-args

There's one missing, --compile-args. The naming scheme is logical though: --<meson-subcommand-name>-args.

Would make sense to adopt that I think.

@tristan957
Copy link

tristan957 commented Oct 4, 2022

--compile-args doesn't exist because mesonpep517 calls out to Ninja directly. I am the one that implemented options there.

@rgommers
Copy link
Contributor Author

rgommers commented Oct 4, 2022

Ah, good to know, thanks. Are you still happy with how that interface works in mesonpep517 now @tristan957?

@tristan957
Copy link

tristan957 commented Oct 4, 2022

I think the interface makes sense given the constraints of the PEP517 interface, although renaming them to --meson-xxx-args might not be a bad idea. Note that I haven't used mesonpep517 in a while. It had quite a few issues and neither me or the maintainer had a lot of time to devote to it. I do think adding a compile args one makes sense if you are meson compile here

@tristan957
Copy link

Having a way to specify default options in the pyproject.toml would also be nice. Not sure if that has been taken into account yet. Passing options on the command line would override the defaults in a perfect world I think

@rgommers
Copy link
Contributor Author

rgommers commented Oct 4, 2022

renaming them to --meson-xxx-args might not be a bad idea.

That may be helpful for clarity. It makes very long lines even longer, but overall that is still a good tradeoff perhaps.

Having a way to specify default options in the pyproject.toml would also be nice.

That is gh-108. It seems reasonable, and the issue submitter there also explains why it must be in pyproject.toml rather than in meson.build or meson-options.txt.

@tristan957
Copy link

I wasn't referring to -Dxxx options. I was referring to --skip-subprojects for instance.

@FFY00
Copy link
Member

FFY00 commented Oct 31, 2022

Closing in favor of #167.

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

Successfully merging this pull request may close these issues.

Passing through project-specific config options
4 participants