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

Verify user arguments #330

Closed
FFY00 opened this issue Mar 1, 2023 · 5 comments · Fixed by #384
Closed

Verify user arguments #330

FFY00 opened this issue Mar 1, 2023 · 5 comments · Fixed by #384
Labels
enhancement New feature or request
Milestone

Comments

@FFY00
Copy link
Member

FFY00 commented Mar 1, 2023

In #167, we added options to allow users to pass arguments to Meson, but that implementation allows them to pass options that may conflict with ours, which may mess things up (eg. overriding --prefix).

While I think we should try to detect conflicting options and either error out or issue a warning, which is tricky to implement, most issues I think can be mitigated by disallowing arguments that don't start with -, I checked and there's no positional argument we'd want users to be able to specify, and putting our options last in the call, other than just defaults that we want the user to be able to override. I think this was a big overlook from my part when we merged that PR, probably because I was meaning to do the argument validation.

@FFY00 FFY00 added the enhancement New feature or request label Mar 1, 2023
@dnicolodi
Copy link
Member

dnicolodi commented Mar 1, 2023

If we move away from having to run meson install (see #279 and #280) we don't need to pass the --prefix option (or the --python.purelibdir and --python.platlibdir options) to meson setup because we don't care anymore about the real installation paths, only about the installation paths relative to the placeholder directories in intro-install_plan.json.

The remaining command line arguments are: $source_dir, $build_dir, --native-file=$native_file, -Ddebug=false, -Doptimization=2, $user_args. I think this ordering is the right one because it allows the user to override the default -Ddebug and -Doptimization (and in the near future -Db_ndebug) options. I would need to check what happens when more than one --native-file argument are present. It would be cool if Meson could merge the content of the two. If he does not, things are most likely not going to work very well if the user adds their own --native-file argument.

meson setup errors out if more than two positional command line arguments (arguments not starting with a -) are specified:

$ meson setup . _build foo
usage: meson [-h] {setup,configure,dist,install,introspect,init,test,wrap,subprojects,rewrite,compile,devenv,env2mfile,help} ...
meson: error: unrecognized arguments: foo

Therefore, the command line validation is already performed and I don't think meson-python needs to adds its own validation on top.

The only open question is about the --native-file argument. If Meson does not try to merge the two native files, the easy fix would be disallow passing this option via meson-python. The more elaborate fix would be to detect it and merge the user provided native file with the meson-python generated one.

@dnicolodi
Copy link
Member

I've verified that Meson merges multiple native files. The only thing to decide is whether we want the user --native-file option to take precedence over the one generated by meson-python or vice versa.

@FFY00
Copy link
Member Author

FFY00 commented Mar 1, 2023

If we move away from having to run meson install (see #279 and #280)

We run meson install because binaries in the build directory have a RPATH set, so that you can use them from there.

$ ls builddir/
build.ninja  compile_commands.json  example.cpython-310-x86_64-linux-gnu.so*  example.cpython-310-x86_64-linux-gnu.so.p/  libexample.so*  libexample.so.p/  meson-info/  meson-logs/  meson-private/  mesonpy-wheel-libs/
$ patchelf --print-rpath builddir/example.cpython-310-x86_64-linux-gnu.so
$ORIGIN/

While this is okay for editable wheels, we must keep meson install when building wheels.

we don't need to pass the --prefix option (or the --python.purelibdir and --python.platlibdir options) to meson setup because we don't care anymore about the real installation paths, only about the installation paths relative to the placeholder directories in intro-install_plan.json.

That is needed by the heuristics, not because we run meson install. Once we drop the heuristics, we can drop those.

I've verified that Meson merges multiple native files. The only thing to decide is whether we want the user --native-file option to take precedence over the one generated by meson-python or vice versa.

We don't, ours should take precedence, as it is only used to set the Python interpreter, and the PEP 517 hooks are required to build packages for the current interpreter.

@dnicolodi
Copy link
Member

We run meson install because binaries in the build directory have a RPATH set, so that you can use them from there.

Don't we adjust the RPATH anyway when building the wheel because the included libraries are relocated?

We don't, ours should take precedence, as it is only used to set the Python interpreter, and the PEP 517 hooks are required to build packages for the current interpreter.

Ok, we just need to order the command line arguments in the right way.

dnicolodi added a commit to dnicolodi/meson-python that referenced this issue Apr 9, 2023
This ensure that the python interpreter path specified in the
meson-python native file overrides any other provided in user supplied
native files.

Fixes mesonbuild#330.
@dnicolodi
Copy link
Member

With #381 merged, there aren't arguments that the user may pass to Meson that conflict with the ones passed by meson-python in a way that may prevent meson-python to work properly. Even passing --prefix does not have any visible effect. The only way I know to interfere with the meson-python assumptions is to pass a --native-file specifying a different path for the python interpreter. This is taken care of in #384.

dnicolodi added a commit to dnicolodi/meson-python that referenced this issue Apr 9, 2023
This ensure that the python interpreter path specified in the
meson-python native file overrides any other provided in user supplied
native files.

Fixes mesonbuild#330.
rgommers pushed a commit that referenced this issue Apr 10, 2023
This ensure that the python interpreter path specified in the
meson-python native file overrides any other provided in user supplied
native files.

Fixes #330.
@rgommers rgommers added this to the v0.13.0 milestone Apr 10, 2023
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 a pull request may close this issue.

3 participants