ENH: remove fallback needed only with now unsupported Meson versions #280

dnicolodi · 2023-01-29T02:03:23Z

I think we can safely drop this now. The CI should agree.

mesonpy/__init__.py

rgommers · 2023-01-29T12:04:39Z

Thanks @dnicolodi. I'm a little nervous about this one, because we've had so many problems with this logic before. I think all Meson issues are fixed indeed. However, it's hard for me to verify that we definitely won't need this fallback code anymore. IIRC this was a problematic case that we may not have a test for:

custom_target('file_with_random_extension',
  input : ['codegen.c.in'],
  output : ['codegen.c'],
  command : [custom_script, '@INPUT@', '@OUTPUT@'],
  install : true,  # still needed because of meson#7372 - can't pass generated targets to py.install_sources
  install_dir : py.get_install_dir() / 'pkgname' / 'submod'  # this may work now, not sure if that covers everything

Or are we sure that by design there is no way to hit that heuristics code path anymore?

dnicolodi · 2023-01-29T13:19:15Z

I just verified that the case above works correctly.

rgommers · 2023-01-29T16:32:24Z

I tested this PR with SciPy, and all works as expected for a regular wheel build on Linux. So that looks good. I'll leave further review to @FFY00, because he's more familiar with this code.

FFY00 · 2023-01-31T19:19:05Z

Yeah, I am a bit uneasy about this. IMO we should wait until after the editable installs release to merge this, unless there's any dependence on this¹.

The Traversable approach would have a dependence on this, or at least, would greatly benefit from this because it needs to map the files. But I'd prefer to also wait to merge that change. ↩

dnicolodi · 2023-01-31T19:43:56Z

The reason to remove this is indeed not to have to worry about "stray" installation paths (installation paths that do not start with a placeholder) in the editable support code. If we want to err on the safe side, we can check that all installation paths start with a placeholder and error out if they do not. If we find any, it is a Meson bug.

FFY00 · 2023-01-31T19:48:54Z

Even though newer Meson versions have it fixed, several users will still be using older versions, shipped by their distros. I'd really prefer to do this change after we release editable installs, so that we don't have to deal with breakage coming from several places.

If we want to err on the safe side, we can check that all installation paths start with a placeholder and error out if they do not.

Yes, we should do that.

FFY00 · 2023-01-31T19:51:26Z

Please note that this change has the possibility of breaking normal installs. I just want to wait a little bit.

dnicolodi · 2023-01-31T19:53:52Z

No, they cannot use older Meson shipped by their distros, we depend on Meson >= 0.63.3:

meson-python/pyproject.toml

Line 31 in c9438b0

'meson>=0.63.3',

We test this version an all sebsequent minor versions in the CI.

We already emit a warning if we cannot map the file. We can turn it into an error if the path does not start with a placeholder.

dnicolodi · 2023-01-31T20:00:30Z

I'm not in an hurry to merge this, but for what should we wait exactly? To say that we need to wait without specifying what we need to wait for seem like postponing indefinitely.

What we should have done, was to add comments to this code specifying in which cases it was required, and ideally we should have added corresponding tests to the test suite. Now we would not be debating whether this work-around is still needed or not. But "del senno di poi son piene le fosse".

FFY00 · 2023-01-31T20:05:23Z

No, they cannot use older Meson shipped by their distros, we depend on Meson >= 0.63.3:

I forgot about that.

mesonpy/__init__.py

FFY00 · 2023-01-31T20:52:23Z

Cool. We broke GitHub interface with this comments. They do not show up in the main PR page...

@dnicolodi did you delete this? Because I actually do not see your comment at all 🤣. I just got the email notification.

dnicolodi · 2023-01-31T20:55:05Z

@dnicolodi did you delete this? Because I actually do not see your comment at all 🤣. I just got the email notification.

Yes, I did. The comments are actually there. Just in a weird order. Sometimes the GitHub interface confuses me.

rgommers · 2023-01-31T22:08:03Z

No, they cannot use older Meson shipped by their distros, we depend on Meson >= 0.63.3:

I forgot about that.

We're missing meson_version: '>= 0.63.3' in the project() call in our own meson.build file, so this is right now not quite correct for non-isolated builds. Can we please add that to ensure we don't use an older Meson version? Just add a comment that the minimum version is defined in both pyproject.toml and meson.build - that's unavoidable, and updating in two places is a very small effort.

rgommers · 2023-01-31T22:08:17Z

(sorry, wrong button)

dnicolodi · 2023-01-31T22:37:52Z

We're missing meson_version: '>= 0.63.3' in the project() call in our own meson.build file, so this is right now not quite correct for non-isolated builds. Can we please add that to ensure we don't use an older Meson version? Just add a comment that the minimum version is defined in both pyproject.toml and meson.build - that's unavoidable, and updating in two places is a very small effort.

Adding something to the meson-python's meson.build enforces the minimum version only when building meson-python.

However, this is a good point. It is not impossible to conceive a way to use the system meson while installing meson-python in a virtual environment. I think maybe we should add a run-time check for the meson version. I think it would cost very little. Going down this line we could even move meson to a build dependency of the project being built obtained via get_requires_for_build_wheel() as we did for ninja, only if the meson executable is not found or is too old.

rgommers · 2023-01-31T23:02:10Z

However, this is a good point. It is not impossible to conceive a way to use the system meson while installing meson-python in a virtual environment. I think maybe we should add a run-time check for the meson version.

You are right, good point. +1 for a check.

eli-schwartz · 2023-02-01T05:57:32Z

Just add a comment that the minimum version is defined in both pyproject.toml and meson.build - that's unavoidable, and updating in two places is a very small effort.

Going down this line we could even move meson to a build dependency of the project being built obtained via get_requires_for_build_wheel() as we did for ninja, only if the meson executable is not found or is too old.

It is theoretically possible to parse meson.build for the meson_version once you have some version of Meson installed, to fine-tune the required version number. Not sure how helpful that is in practice?

import json
import subprocess

stdout = subprocess.check_output(['meson', 'introspect', '--ast', 'meson.build'])
j = json.loads(stdout)

for kw in j['lines'][0]['args']['kwargs']:
    if kw['key']['value'] == 'meson_version':
        meson_version = kw['val']['value']
        break

dnicolodi · 2023-02-01T12:07:48Z

We don't install the meson-python's meson.build thus extracting the minimum required meson version from there at runtime is not that useful. Unfortunately, I think that having the minimum version both in pyproject.toml and somewhere in the code is the only way to do the check at run time (with reasonable complexity).

dnicolodi · 2023-02-04T19:59:11Z

Is there a way to make the output of pre-commit-check more informative? pre-commit-check is failing with this error

ruff.....................................................................Failed
- hook id: ruff
- files were modified by this hook

Found 1 error (1 fixed, 0 remaining).

which tells me exactly nothing about what is wrong.

FFY00 · 2023-03-07T22:52:49Z

mesonpy/__init__.py

+# Maps wheel installation paths to Meson installation path placeholders.
+_SCHEME_MAP = {
+    'scripts': ('{bindir}',),
+    'purelib': ('{py_purelib}',),
+    'platlib': ('{py_platlib}', '{moduledir_shared}'),
+    'headers': ('{includedir}',),
+    'data': ('{datadir}',),
+    # our custom location
+    'mesonpy-libs': ('{libdir}', '{libdir_shared}')
+}
+
+
+def _map_meson_destination(destination: str) -> Tuple[Optional[str], pathlib.Path]:
+    """Map Meson installation path placeholders to wheel installation directory."""
+    parts = pathlib.Path(destination).parts
+    for folder, placeholders in _SCHEME_MAP.items():
+        if parts[0] in placeholders:
+            return folder, pathlib.Path(*parts[1:])
+    return None, pathlib.Path(destination)
+
+
+def _map_to_wheel(sources: Dict[str, Dict[str, Any]]) -> DefaultDict[str, List[Tuple[pathlib.Path, str]]]:
+    """Map files to the wheel, organized by wheel installation directrory."""
+    wheel_files = collections.defaultdict(list)
+    for group in sources.values():
+        for src, target in group.items():
+            dest = target['destination']
+            directory, path = _map_meson_destination(dest)
+            if directory is not None:
+                wheel_files[directory].append((path, src))
+            else:
+                warnings.warn(f'Installation path {dest!r} for {src!r} could not be mapped to an equivalent wheel directory.')
+                if not re.match(r'^{\w+}$', path.parts[0]):
+                    raise RuntimeError('Meson installation path {dest!r} does not start with a placeholder. Meson bug!')
+    return wheel_files


Please keep these functions in _WheelBuilder.

Please keep the docstring from _map_from_scheme_map.

Installation paths can get quite big, so please keep the original "File could not be mapped to an equivalent wheel directory" warning. It puts the actual issue on the front, making sure you can easily read it if the path is big, and gives all similar warnings the same initial text, making it obvious that a bunch of warnings are the same issue.
The error could also benefit from a similar format, but it's less of a problem.

IMO it'd be cleaner to move the warnings to _map_meson_destination.

Please keep these functions in _WheelBuilder.

Why? They don't need access to any member of _WheelBuilder.

IMO it'd be cleaner to move the warnings to _map_meson_destination.

For doing that, either the src path needs to be passed to _map_meson_destination just for the purpose of maybe emitting the warning this information needs to be removed from the warning.

Why? They don't need access to any member of _WheelBuilder.

Because its whole purpose is to support code in _WheelBuilder, and this way it also can access internal state, for example the source paths you referred in the next comment.

For doing that, either the src path needs to be passed to _map_meson_destination just for the purpose of maybe emitting the warning this information needs to be removed from the warning.

Or you can have the function in _WheelBuilder and be able to access it directly 😅

Please have a look at how the code is written. Having it a method of _WheelBuilder does not help in any way.

mesonpy/__init__.py

mesonpy/_introspection.py

meson-python correct behavior depends on fixes for Meson bugs released with version 0.63.3. Implement a belt and suspenders approach for making sure the version of the meson command line tool matches our minimum requirement.

rgommers · 2023-03-12T07:46:41Z

I did some more testing with the merged version of this in SciPy's CI. We have a specific job for Debian there which used to be an issue. All good now it looks like. Glad to see this code gone!

This reverts commits: - 4a4b54b - ba2743e - 26ab3e4 - 8661c22 Signed-off-by: Filipe Laíns <[email protected]>

Switch from debug=false, optimization=2 to buildtype=release. Despite what the Meson documentation says, the former does not change the default buildtype=debug, which undesired effects on other settings derived from the value of the buildtype option. Whit the default b_vscrt=from_buildtype option, Meson instructs the MSVC compiler to use the debug version of the VS runtime library when buildtype=debug. This causes the linker look for the debug build of all the linked DLLs. The Python distribution for Windows does not contain a debug version of the python.dll and linking fails. To avoid this issue when the user explicitly asks for a debug build, also set b_vscrt=mt. The b_ndebug=if-release option set by meson-python also looks at the value of the buildtype and not to the value of the debug options. Setting buildtype=release has the desired effect of disabling assertions. The prefix, python.purelibdir, and python.platlibdir build options are no more necessary after the heuristic for determining the location where installed files need to be placer from their installation location has been removed in mesonbuild#280. Remove them. Fixes mesonbuild#381.

Switch from debug=false, optimization=2 to buildtype=release. Despite what the Meson documentation says, the former does not change the default buildtype=debug, which undesired effects on other settings derived from the value of the buildtype option. Whit the default b_vscrt=from_buildtype option, Meson instructs the MSVC compiler to use the debug version of the VS runtime library when buildtype=debug. This causes the linker look for the debug build of all the linked DLLs. The Python distribution for Windows does not contain a debug version of the python.dll and linking fails. To avoid this issue when the user explicitly asks for a debug build, also set b_vscrt=mt. The b_ndebug=if-release option set by meson-python also looks at the value of the buildtype and not to the value of the debug options. Setting buildtype=release has the desired effect of disabling assertions. The prefix, python.purelibdir, and python.platlibdir build options are no more necessary after the heuristic for determining the location where installed files need to be placer from their installation location has been removed in #280. Remove them. Fixes #381.

dnicolodi force-pushed the heuristic branch 2 times, most recently from 3577f43 to 4031d7c Compare January 29, 2023 02:50

rgommers reviewed Jan 29, 2023

View reviewed changes

mesonpy/__init__.py Outdated Show resolved Hide resolved

rgommers added the maintenance Regular code improvements that are not new features nor end-user-visible bugs label Jan 29, 2023

FFY00 suggested changes Jan 31, 2023

View reviewed changes

mesonpy/__init__.py Outdated Show resolved Hide resolved

mesonpy/__init__.py Outdated Show resolved Hide resolved

rgommers closed this Jan 31, 2023

rgommers reopened this Jan 31, 2023

dnicolodi force-pushed the heuristic branch 4 times, most recently from b570f3c to e107203 Compare February 4, 2023 19:54

dnicolodi added this to the v0.13.0 milestone Mar 3, 2023

dnicolodi force-pushed the heuristic branch from 00de2d5 to adf24d9 Compare March 7, 2023 22:49

dnicolodi requested review from rgommers and FFY00 March 7, 2023 22:51

FFY00 suggested changes Mar 7, 2023

View reviewed changes

dnicolodi added 4 commits March 8, 2023 00:58

MAINT: refactor version parsing into an utility function

201ba14

ENH: implement runtime check for the required Meson version

eadd75b

meson-python correct behavior depends on fixes for Meson bugs released with version 0.63.3. Implement a belt and suspenders approach for making sure the version of the meson command line tool matches our minimum requirement.

ENH: remove fallback needed only with now unsupported Meson versions

4a4b54b

MAINT: simplify _map_from_scheme_map()

ba2743e

dnicolodi force-pushed the heuristic branch 4 times, most recently from c5f07be to 8286b5c Compare March 8, 2023 00:42

dnicolodi added 3 commits March 8, 2023 01:45

MAINT: further simplify

26ab3e4

ENH: raise an error when a Meson installation path looks wrong

8661c22

ENH: align error and warning log format

274a39d

dnicolodi force-pushed the heuristic branch from 8286b5c to 274a39d Compare March 8, 2023 00:45

FFY00 mentioned this pull request Mar 10, 2023

Misc CI improvements #289

Closed

dnicolodi merged commit 8060e82 into mesonbuild:main Mar 11, 2023

FFY00 mentioned this pull request Mar 14, 2023

Unable to map files without a starting placeholder in the Meson metadata (regression) #341

Closed

FFY00 added a commit to FFY00/meson-python that referenced this pull request Mar 14, 2023

Partially revert mesonbuildGH-280

6c0832f

This reverts commits: - 4a4b54b - ba2743e - 26ab3e4 - 8661c22 Signed-off-by: Filipe Laíns <[email protected]>

FFY00 added a commit to FFY00/meson-python that referenced this pull request Mar 14, 2023

Partially revert mesonbuildGH-280

3e97986

This reverts commits: - 4a4b54b - ba2743e - 26ab3e4 - 8661c22 Signed-off-by: Filipe Laíns <[email protected]>

FFY00 added a commit to FFY00/meson-python that referenced this pull request Mar 14, 2023

BUG: partially revert mesonbuildGH-280

d874116

This reverts commits: - 4a4b54b - ba2743e - 26ab3e4 - 8661c22 Signed-off-by: Filipe Laíns <[email protected]>

FFY00 mentioned this pull request Apr 4, 2023

Build directory for editable installations #370

Closed

dnicolodi mentioned this pull request Apr 6, 2023

ENH: adjust default build options #383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: remove fallback needed only with now unsupported Meson versions #280

ENH: remove fallback needed only with now unsupported Meson versions #280

dnicolodi commented Jan 29, 2023 •

edited

Loading

rgommers commented Jan 29, 2023

dnicolodi commented Jan 29, 2023

rgommers commented Jan 29, 2023

FFY00 commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

FFY00 commented Jan 31, 2023

FFY00 commented Jan 31, 2023 •

edited

Loading

dnicolodi commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

FFY00 commented Jan 31, 2023

FFY00 commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

rgommers commented Jan 31, 2023 •

edited

Loading

rgommers commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

rgommers commented Jan 31, 2023

eli-schwartz commented Feb 1, 2023

dnicolodi commented Feb 1, 2023

dnicolodi commented Feb 4, 2023

FFY00 Mar 7, 2023 •

edited

Loading

dnicolodi Mar 7, 2023

FFY00 Mar 8, 2023 •

edited

Loading

dnicolodi Mar 11, 2023

rgommers commented Mar 12, 2023

ENH: remove fallback needed only with now unsupported Meson versions #280

ENH: remove fallback needed only with now unsupported Meson versions #280

Conversation

dnicolodi commented Jan 29, 2023 • edited Loading

rgommers commented Jan 29, 2023

dnicolodi commented Jan 29, 2023

rgommers commented Jan 29, 2023

FFY00 commented Jan 31, 2023

Footnotes

dnicolodi commented Jan 31, 2023

FFY00 commented Jan 31, 2023

FFY00 commented Jan 31, 2023 • edited Loading

dnicolodi commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

FFY00 commented Jan 31, 2023

FFY00 commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

rgommers commented Jan 31, 2023 • edited Loading

rgommers commented Jan 31, 2023

dnicolodi commented Jan 31, 2023

rgommers commented Jan 31, 2023

eli-schwartz commented Feb 1, 2023

dnicolodi commented Feb 1, 2023

dnicolodi commented Feb 4, 2023

FFY00 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

dnicolodi Mar 7, 2023

Choose a reason for hiding this comment

FFY00 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

dnicolodi Mar 11, 2023

Choose a reason for hiding this comment

rgommers commented Mar 12, 2023

dnicolodi commented Jan 29, 2023 •

edited

Loading

FFY00 commented Jan 31, 2023 •

edited

Loading

rgommers commented Jan 31, 2023 •

edited

Loading

FFY00 Mar 7, 2023 •

edited

Loading

FFY00 Mar 8, 2023 •

edited

Loading