ENH: remove the need for a working directory & other cleanups #364

dnicolodi · 2023-03-26T20:10:06Z

This brings back the simplifications I proposed in #340 in a way compatible with having the wheel generated from files installed by meson install in a temporary destination directory. This covers a very minimal part of the work done in #348 too. It should be trivial to rebase #348 on top of this.

Fixes #355 and #330 as there are no arguments the user can supply that would interfere with meson-python internals. It also fixes #317 in the same way as #348 does.

dnicolodi · 2023-03-26T20:51:30Z

The macOS test failure are the same found in #348 and will have the same solution.

rgommers · 2023-03-28T08:26:02Z

Thanks @dnicolodi. Trying to think of a merge strategy here - I think the status is:

this seems to overlap with, and improve on, the first 3 commits of BUG: copy wheel files from the Meson install directory #348.
this is blocked on the macOS RPATH test failures right now, which are addressed by gh-348 (and that's the key tricky and unfinished thing in that PR). This PR could temporarily xfail them though?
gh-348 also has tests for subdir(..., exclude_files) that are useful and will pass in combination with this PR. But there's no issue in adding those later.

dnicolodi · 2023-03-28T08:54:02Z

I see this more like preparation work for landing #348 with the resulting code being in better shape (if we agree that the resulting code after merging this PR is "better").

From a "build things from the ground up" point of view, I would merge this and rebase the fixes in #348 on top. Or we can do the other way around and have this merged after #348 to cleanup the implementation. I think merging this first results in less (or easier to resolve) rebase conflicts and will make looking the commit history easier, but we can go either way. This PR being ready to be merged, while the proper solution for the rpath issues in #348 are still being discussed, is one reason to have this merged first. If you want to merge with all green tests, I can mark the rpath test as xfail.

I can take care of rebasing #348 on top of this.

The tests for exclude_files probably found a bug in Meson on Windows, thus these are also marked as xfail on that platform. I started investigating that on the Meson side, but I didn't reach a conclusion yet.

dnicolodi · 2023-03-29T09:10:53Z

@rgommers I added the xfail to the tests now failing on macOS

rgommers · 2023-03-29T13:01:06Z

I can take care of rebasing #348 on top of this.

I think you have a branch for this already, right? If so, would you mind sharing for some testing? If not, please don't worry about it now.

dnicolodi · 2023-03-29T13:11:06Z

I think you have a branch for this already, right?

No, I don't. If that is the desired way forward, it should not take more than few minutes to prepare it.

dnicolodi · 2023-03-29T13:47:01Z

PR #348 rebased on top of this is here https://github.com/dnicolodi/meson-python/tree/pr348

FFY00 · 2023-03-31T01:26:46Z

I removed the v0.13.0 milestone, as this getting in is not critical for it, and we should release 0.13.0 as soon as we are able to. If this is in shape to get in by the time we sort the release blockers, great, otherwise we can just merge it afterwards and have it in 0.14.0.

dnicolodi · 2023-03-31T05:56:04Z

This is ready to be merged since a long time. It just needs a review. I don't understand why you think otherwise.

FFY00 · 2023-03-31T06:22:44Z

It conflicts with #348, which needs to get in for the new release.

dnicolodi · 2023-03-31T06:47:32Z

I already posted the conflict resolution.

FFY00 · 2023-03-31T06:49:35Z

But the PR may get more changes before merging. If you have an updated PR by then, great, we can merge it, otherwise we just do it for the next release.

dnicolodi · 2023-03-31T06:52:31Z

What's the problem with merging this now, rebase #348 on top, and take it from there?

FFY00 · 2023-03-31T06:55:15Z

It can unnecessarily delay the release. Like I said, this is not critical, unlike the other PR, so I am not merging this now.

FFY00 · 2023-03-31T06:57:18Z

This PR hasn't even been reviewed btw, so it might still require changes, etc. I am prioritizing the release blockers.

Looking up keys in a default dict adds these keys to the dictionary with their default value when they are not present. This is an issue if the lookup happens while the dictionary is being iterated. Avoid it.

This will matter later when intall_path() will get an absolute path into the installation directory rather than a path into the source or build directory.

Currently a Project has an associated a temporary working directory containing the build directory (unless the user specifies another one via configuration options) and the install directory for the meson project. The code can be simplified assigning to the project a build directory and always using a temporary install directory. Note that currently all files are copied into the Python wheel from the source or build directory, thus running meson install into a destination directory is not that useful. This will be addressed in later commits.

dnicolodi · 2023-08-26T12:57:26Z

@rgommers I left out the commits with behavior changes from this PR. This is now just refactoring and a change in the structure of temporary directories. It would be nice to have this merged as a first step toward fixing the linked issues.

rgommers

Thanks @dnicolodi. Restructuring this to have no behavior changes was helpful. Sorry it took so long to review. I verified this as well as I could, and tested with SciPy - it all looks good. The code is easier to understand after this - and the simplifications in the tests are a good sign too. So in it goes.

rgommers · 2023-09-01T16:47:15Z

@dnicolodi WDYT about tagging a 0.14.0 release? The PR queue is pretty much empty, so now would be a good time. We can quickly do a 0.15.0 too in case the subsequent PR to this one arrives quickly.

dnicolodi · 2023-09-01T16:49:58Z

Thanks for the review @rgommers. I'll prepare a PR with the actual fixes

dnicolodi · 2023-09-01T17:05:09Z

WDYT about tagging a 0.14.0 release?

I don't know how critical it is to push a release with the fixes that already landed in main. There are three known bugs related to the fact that meson-python takes the files for the wheel from the source directory instead than from the build directory: (1) the handling of exclude_path and exclude_directory for install_subdir, and (2) removing the RPATH pointing to the build directory. In addition, there is a bug (3) in how sub direcrtories of direcotries installed with install_subdir are handled. I have patches for all three, thus getting these fixes in should be fast. However, investigating (2) I found a bug in Meson, thus having correct RPATH handling would require a new Meson release.

rgommers · 2023-09-01T17:10:53Z

It's not critical to do it right this minute, so let's wait for at least the fixes for the bugs that don't depend on a new Meson release?

I'm also going through the backlog now and see what can be closed or easily addressed. Some of the documentation issues are straightforward, I'll try to fix a couple after dinner.

dnicolodi force-pushed the install-into-wheel branch 4 times, most recently from 6083699 to c2ee830 Compare March 26, 2023 20:46

dnicolodi force-pushed the install-into-wheel branch 2 times, most recently from 8e71ace to 8b496b6 Compare March 26, 2023 21:32

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

rgommers mentioned this pull request Mar 28, 2023

BUG: copy wheel files from the Meson install directory #348

Closed

dnicolodi force-pushed the install-into-wheel branch 3 times, most recently from 32b372a to a580617 Compare March 28, 2023 21:08

rgommers mentioned this pull request Mar 29, 2023

Make it easier to build the docs #368

Closed

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

rgommers self-requested a review March 30, 2023 20:30

FFY00 removed this from the v0.13.0 milestone Mar 31, 2023

dnicolodi force-pushed the install-into-wheel branch from d93c2f6 to f64f914 Compare June 6, 2023 13:55

dnicolodi mentioned this pull request Jun 21, 2023

meson --skip-subprojects option ignored when building wheel #423

Closed

dnicolodi force-pushed the install-into-wheel branch 9 times, most recently from abaeb0c to b79c87b Compare July 15, 2023 16:10

dnicolodi removed this from the v0.14.0 milestone Aug 23, 2023

dnicolodi added 3 commits August 26, 2023 11:56

ENH: do not populate defaultdict on lookup

217017c

Looking up keys in a default dict adds these keys to the dictionary with their default value when they are not present. This is an issue if the lookup happens while the dictionary is being iterated. Avoid it.

MAINT: simplify _WheelBuilder.build()

c7f473d

MAINT: tiny simplification for _WheelBuilder._install_path()

53fbcdd

This will matter later when intall_path() will get an absolute path into the installation directory rather than a path into the source or build directory.

dnicolodi force-pushed the install-into-wheel branch 4 times, most recently from f97ff4d to ee3cf79 Compare August 26, 2023 10:06

dnicolodi force-pushed the install-into-wheel branch from ee3cf79 to cd2894c Compare August 26, 2023 10:30

rgommers added this to the v0.14.0 milestone Sep 1, 2023

rgommers approved these changes Sep 1, 2023

View reviewed changes

rgommers merged commit da1c2e0 into mesonbuild:main Sep 1, 2023

rgommers mentioned this pull request Sep 1, 2023

BUG: install_subdir() in meson.build does not honor exlude_files/directories #317

Closed

ENH: remove the need for a working directory & other cleanups #364

ENH: remove the need for a working directory & other cleanups #364

Uh oh!

Conversation

dnicolodi commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnicolodi commented Mar 26, 2023

Uh oh!

rgommers commented Mar 28, 2023

Uh oh!

dnicolodi commented Mar 28, 2023

Uh oh!

dnicolodi commented Mar 29, 2023

Uh oh!

rgommers commented Mar 29, 2023

Uh oh!

dnicolodi commented Mar 29, 2023

Uh oh!

dnicolodi commented Mar 29, 2023

Uh oh!

FFY00 commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnicolodi commented Mar 31, 2023

Uh oh!

FFY00 commented Mar 31, 2023

Uh oh!

dnicolodi commented Mar 31, 2023

Uh oh!

FFY00 commented Mar 31, 2023

Uh oh!

dnicolodi commented Mar 31, 2023

Uh oh!

FFY00 commented Mar 31, 2023

Uh oh!

FFY00 commented Mar 31, 2023

Uh oh!

dnicolodi commented Aug 26, 2023

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

rgommers commented Sep 1, 2023

Uh oh!

dnicolodi commented Sep 1, 2023

Uh oh!

dnicolodi commented Sep 1, 2023

Uh oh!

rgommers commented Sep 1, 2023

Uh oh!

Uh oh!

dnicolodi commented Mar 26, 2023 •

edited

Loading

FFY00 commented Mar 31, 2023 •

edited

Loading