ENH: use system ninja if adequate #175

henryiii · 2022-10-15T03:45:52Z

This uses the same solution as patchelf - if the system has it (and it's a new enough version, ~~1.7+ according to the release notes~~, 1.8.2 according to the meson error if missing), then use that. This fixes the build on systems that have a new enough ninja present but are not supportable by binary wheels.

See scikit-build/ninja-python-distributions#127 for discussion.

rgommers · 2022-10-15T11:47:46Z

Thanks @henryiii! I agree that checking if ninja is already available is the right thing to do.

The tests seem to be unhappy still:

ERROR: Could not detect Ninja v1.8.2 or newer

henryiii · 2022-10-15T13:25:31Z

Ahh, I should uninstall Ninja before running the tests locally. I also might’ve forgotten to run the test after my last change. I’m hoping it’s just that some unit tests are not activating the isolated environment. but I’ll check.

I’ll be traveling for the next couple of days so I might not be able to finish this until Monday.

mesonpy/__init__.py

henryiii · 2022-10-17T14:57:32Z

It looks like it hits the non-PEP 621 standard metadata generation:

Using Meson to generate the project metadata (no `project` section in pyproject.toml)

Then it breaks because it's trying to run Meson outside building sdists/wheels. Is it possible to generate the metadata without ninja being present? Or could this step be delayed until inside the isolated environment via the hooks (even prepare_metadata_for_build_wheel runs inside the isolated environment)?

mesonpy/__init__.py

tests/conftest.py

henryiii · 2022-10-20T12:36:07Z

This is working for SDists:

* Creating venv isolated environment...
* Installing packages in isolated environment... (meson-python @ git+https://github.com/henryiii/meson-python@henryiii/fix/ninja, oldest-supported-numpy, setuptools_scm)
* Getting dependencies for sdist...
* Installing packages in isolated environment... (ninja >= 1.8.2)
* Building sdist...

But it's not working for Wheels, it's triggering meson too soon:

* Building wheel from sdist
* Creating venv isolated environment...
* Installing packages in isolated environment... (meson-python @ git+https://github.com/henryiii/meson-python@henryiii/fix/ninja, oldest-supported-numpy, setuptools_scm)
* Getting dependencies for wheel...
+ meson setup --native-file=/private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-via-sdist-ghy0drx8/fsps-0.4.3.dev1+gd689181.d20221020/.mesonpy-native-file.ini -Ddebug=false -Doptimization=2 --prefix=/usr/local/opt/python@3.10/Frameworks/Python.framework/Versions/3.10 /private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-via-sdist-ghy0drx8/fsps-0.4.3.dev1+gd689181.d20221020 /private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/build-via-sdist-ghy0drx8/fsps-0.4.3.dev1+gd689181.d20221020/.mesonpy-dpq6izjw/build
The Meson build system

henryiii · 2022-10-20T12:40:01Z

Ahh, this is "it's running a context manager that sets up a build directory". Okay, so it's running Meson to see if it it is pure or not. Is there a reason to use python-meson if it's pure? I think it probably should add patchelf regardless - it won't use it in the pure case, and it's would only be a downside if not present on the system on a linux system without patchelf binaries and for a pure Python build. This is faster too.

henryiii · 2022-10-20T12:46:31Z

This works now for me locally after uninstalling ninja:

Successfully built fsps-0.4.3.dev1+gd689181.d20221020.tar.gz and fsps-0.4.3.dev1+gd689181.d20221020-cp310-cp310-macosx_12_0_x86_64.whl

Changes:

diff --git a/pyproject.toml b/pyproject.toml
index 8d3e71d..153a282 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -28,7 +28,7 @@ test = ["pytest", "pytest-xdist"]

 [build-system]
 build-backend = 'mesonpy'
-requires = ["meson-python>=0.8.1", "wheel", "oldest-supported-numpy", "setuptools_scm"]
+requires = ["meson-python @ git+https://github.com/henryiii/meson-python@henryiii/fix/ninja", "oldest-supported-numpy", "setuptools_scm"]

 [tool.setuptools_scm]
 write_to = "src/fsps/fsps_version.py"

henryiii · 2022-10-20T18:27:27Z

I think I fixed the Windows and Linux tests, macOS was passing - you can tell what I'm developing on. :)

henryiii · 2022-10-20T20:11:42Z

This is working except on pypy & Windows, and those are failing due to the virtual_env fixture not working on those platforms, so it can't do the isolated test. I'm using pytest-virtualenv elsewhere, which works on both platforms, so I've pushed a commit that uses that instead. (I wonder if this is related to the skips on the other tests using virtual_env)?

henryiii · 2022-10-21T20:47:35Z

Could you try triggering the CI again? I think I fixed the platform incompatibilities by changing virtualenv implementations.

eli-schwartz · 2022-10-30T15:15:11Z

NotImplementedError: Unknown Python implementation: mesonpy. Please report this to https://github.com/FFY00/mesonpy/issues and include information about the Python distribution you are using.

What a fascinating error. Why is this even able to happen?

pyproject.toml

henryiii · 2022-10-31T03:27:49Z

Looks like Win Python 3.7 might be broken by this, though I'm not sure how. Will try to look into it in the next day or two. Something is going really wrong with the ABI tags, seems to be related to the stable ABI. Seems mesonpy is reporting itself as the Python implementation on Windows 3.7 only. 🙄

FFY00 · 2022-10-31T04:15:08Z

Looks like some of the tag parsing code is broken for windows tags. Windows does not have standardized tags so this all is just looking at what happens in practice and writing code around it 😑. Interesting how this only triggers on Python 3.7.

FFY00 · 2022-10-31T04:16:11Z

What a fascinating error. Why is this even able to happen?

It parsing code for a non-standardized format, so it's a bit fragile.

dnicolodi · 2022-11-02T11:20:52Z

The parsing code that generates the funny error is removed in #191. I'll be happy to provide another review for this PR. Can you please squash the commits into meaningful patches?

henryiii · 2022-11-03T04:01:24Z

squash the commits into meaningful patches

Should I wait till after #191?

henryiii · 2022-11-03T04:03:29Z

Ahh, so this can happen without my changes: #189 - okay, can clean up.

henryiii · 2022-11-03T04:10:46Z

Everything that was not related was PR'd separately already, so it's just one commit, the ninja change + related tests.

henryiii · 2022-11-16T18:51:08Z

I'm getting meson dist --allow-dirty --no-tests --formats gztar breaking in the new test in Sage, but I can't see any info as to why it breaks.

mkoeppe · 2022-11-16T18:53:38Z

I see Dist currently only works with Git or Mercurial repos in https://github.com/mesonbuild/meson-python/actions/runs/3481218227/jobs/5822146022#step:14:326

henryiii · 2022-11-16T18:56:38Z

Hmm, interesting! Any idea what that means, though? This is triggering build isolation, so it should be in a virtual environment, but it should still be pointing at the original source. Unless it's in the sdist -> wheel step. Then it's getting and SDist and making a wheel.

Here's the relevant bit of the logs (stderr only has the traceback):

* Building sdist...
The Meson build system
Version: 0.64.0
Source dir: /sage/local/var/lib/sage/venv-python3.9/var/tmp/sage/build/meson_python-git/src/tests/packages/scipy-like
Build dir: /sage/local/var/lib/sage/venv-python3.9/var/tmp/sage/build/meson_python-git/src/tests/packages/scipy-like/.mesonpy-3qyyq2m3/build
Build type: native build
Project name: scipy-like
Project version: 2.3.4
C compiler for the host machine: gcc (gcc 8.5.0 "gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-17)")
C linker for the host machine: gcc ld.bfd 2.30-119
Cython compiler for the host machine: cython (cython 0.29.32)
Host machine cpu family: x86_64
Host machine cpu: x86_64
Program python found: YES (/tmp/build-env-l0l6fkhy/bin/python)
Found pkg-config: /usr/bin/pkg-config (1.4.2)
Build targets in project: 3

scipy-like 2.3.4

  User defined options
    Native files: /sage/local/var/lib/sage/venv-python3.9/var/tmp/sage/build/meson_python-git/src/tests/packages/scipy-like/.mesonpy-native-file.ini
    debug       : false
    optimization: 2
    prefix      : /usr

Found ninja-1.11.0 at /sage/local/bin/ninja
Dist currently only works with Git or Mercurial repos
+ meson setup --prefix=/usr /sage/local/var/lib/sage/venv-python3.9/var/tmp/sage/build/meson_python-git/src/tests/packages/scipy-like /sage/local/var/lib/sage/venv-python3.9/var/tmp/sage/build/meson_python-git/src/tests/packages/scipy-like/.mesonpy-3qyyq2m3/build --native-file=/sage/local/var/lib/sage/venv-python3.9/var/tmp/sage/build/meson_python-git/src/tests/packages/scipy-like/.mesonpy-native-file.ini -Ddebug=false -Doptimization=2
+ meson dist --allow-dirty --no-tests --formats gztar

ERROR Backend subprocess exited when trying to invoke build_sdist

mkoeppe · 2022-11-16T18:58:19Z

All tests in the Sage CI operate from an sdist, not from the meson-python clone.

FFY00 · 2022-11-17T07:32:12Z

It seems that Meson isn't being able to produce a dist when it doesn't have VCS, which is reasonable, but we'll need to deal with that.

rgommers · 2022-11-17T10:20:25Z

It seems that Meson isn't being able to produce a dist when it doesn't have VCS, which is reasonable, but we'll need to deal with that.

Erroring out seems like the right default at least. If we do need a way to create an sdist outside of a VCS, can that be a new issue with its own rationale? Seems low-prio to me, and should not be blocking for dealing with system ninja.

henryiii · 2022-11-17T14:06:23Z

Passing. :)

dnicolodi

Nice. Maybe a couple of wrong merge resolutions during the rebase?

pyproject.toml

dnicolodi · 2022-11-17T15:32:13Z

pyproject.toml

  'auditwheel',
  'Cython',
  'pyproject-metadata>=0.6.1',
  'wheel',
+  'importlib_metadata; python_version<"3.8"',


Same for this one?

dnicolodi · 2022-11-17T16:24:20Z

tests/test_pep518.py

@@ -15,7 +15,6 @@
 @pytest.mark.parametrize(
    'build_arg', ['', '--wheel'], ids=['sdist_to_wheel', 'wheel_directly']
 )
-@pytest.mark.xfail(sys.platform.startswith('cygwin'), reason='ninja pip package not available on cygwin', strict=True)
 def test_pep518(pep518, package, build_arg, tmp_path):


It would be cool to extend this test to be an integration test for the two cases: ninja from a package and ninja from the system. However I'm not so sure about what strategy we could use to realize that. If we want to keep the test not dependent on a specific test environment setup, the only thing I can think about is to provide a wheelhouse that does not have ninja, mask a possibly existing ninja by adding a script that returns an error or a wrong version number to a directory we prepend to $PATH, and verify that the build fails. Add ninja to the wheelhouse and verify that the build succeeds.

dnicolodi · 2022-11-17T16:32:22Z

I just merged #212 Can you please rebase again? We should see the pep518 test fail on platforms that do not have a system ninja.

dnicolodi · 2022-11-17T16:37:46Z

If you rebase and add ninja to the wheelhouse setup, I'll work on the pep518 tests.

dnicolodi · 2022-11-17T17:11:46Z

Seems that all the GitHub Actions runners have ninja installed...

dnicolodi · 2022-11-17T18:28:31Z

Never mind, ninja is there because we install it as part of the test dependencies. I think we can merge this and work on extended tests later.

FFY00

I just have one thing that needs to be fixed before merging. Otherwise, it looks good to me! Thank you @henryiii 😊

mesonpy/__init__.py

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

eli-schwartz · 2022-11-18T03:03:44Z

Erroring out seems like the right default at least. If we do need a way to create an sdist outside of a VCS, can that be a new issue with its own rationale? Seems low-prio to me, and should not be blocking for dealing with system ninja.

Maybe a better way to describe this would be "if we do need a way to create an sdist from an sdist"? I'm not sure I understand the general desirability of this, although apparently some people do want for academic reasons.

For test mocking specifically, there should probably be a setup fixture that runs git init && git add -A . && git commit -m "create test project repo" or thereabouts

henryiii · 2022-11-18T03:09:35Z

There is a context manager that does that and I found it and now use it (which is why the tests are passing now) when I realized the other tests had to be doing this too. It can't be a fixture since it needs the package directory as input or it needs to have the working directory already changed to the package directory.

(It could be a fixture that takes arguments via a mark, actually, but probably not worth it?)

eli-schwartz · 2022-11-18T03:15:49Z

Ah okay, never mind then. :D

Signed-off-by: Filipe Laíns <lains@riseup.net>

henryiii force-pushed the henryiii/fix/ninja branch from 44ee2b4 to 9eb127d Compare October 15, 2022 04:00

henryiii changed the title ~~fix: use system ninja if adequate~~ ENH: use system ninja if adequate Oct 15, 2022

henryiii mentioned this pull request Oct 15, 2022

Avoid downloading ninja source scikit-build/ninja-python-distributions#127

Open

rgommers added the enhancement New feature or request label Oct 15, 2022

eli-schwartz suggested changes Oct 16, 2022

View reviewed changes

mesonpy/__init__.py Outdated Show resolved Hide resolved

mesonpy/__init__.py Outdated Show resolved Hide resolved

henryiii marked this pull request as draft October 17, 2022 15:39

eli-schwartz reviewed Oct 19, 2022

View reviewed changes

mesonpy/__init__.py Outdated Show resolved Hide resolved

eli-schwartz reviewed Oct 20, 2022

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

henryiii marked this pull request as ready for review October 26, 2022 11:52

eli-schwartz mentioned this pull request Oct 30, 2022

pip check fails due to dependency on ninja #60

Closed

FFY00 force-pushed the henryiii/fix/ninja branch from 10b8a78 to 2bc8635 Compare October 31, 2022 00:58

FFY00 reviewed Oct 31, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

henryiii force-pushed the henryiii/fix/ninja branch from 924d689 to 2f3dce2 Compare November 3, 2022 04:09

henryiii force-pushed the henryiii/fix/ninja branch 2 times, most recently from 081396b to 57a0579 Compare November 16, 2022 16:30

henryiii mentioned this pull request Nov 16, 2022

TST: adding isolated build test #209

Merged

dnicolodi mentioned this pull request Nov 17, 2022

TST: simplify pep518 test setup #212

Merged

henryiii force-pushed the henryiii/fix/ninja branch from 57a0579 to 21a5afb Compare November 17, 2022 13:23

dnicolodi reviewed Nov 17, 2022

View reviewed changes

henryiii force-pushed the henryiii/fix/ninja branch from a20c78c to fbbfbf9 Compare November 17, 2022 17:05

FFY00 approved these changes Nov 17, 2022

View reviewed changes

mesonpy/__init__.py Outdated Show resolved Hide resolved

henryiii added 2 commits November 17, 2022 19:54

ENH: use system ninja if adequate

7df0f13

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

fix: address feedback

47f3ba6

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii force-pushed the henryiii/fix/ninja branch from fbbfbf9 to 47f3ba6 Compare November 18, 2022 01:15

don't ask for patchelf if ninja is available and the project is pure

9d47743

Signed-off-by: Filipe Laíns <lains@riseup.net>

FFY00 force-pushed the henryiii/fix/ninja branch from 3f17afe to 9d47743 Compare November 18, 2022 19:22

FFY00 merged commit 8824bc2 into mesonbuild:main Nov 19, 2022

maxbachmann mentioned this pull request Nov 30, 2022

RFE: please allow use system installed ninja binary scikit-build/ninja-python-distributions#156

Closed

ENH: use system ninja if adequate #175

ENH: use system ninja if adequate #175

Conversation

henryiii commented Oct 15, 2022 • edited Loading

rgommers commented Oct 15, 2022

henryiii commented Oct 15, 2022

henryiii commented Oct 17, 2022 • edited Loading

henryiii commented Oct 20, 2022

henryiii commented Oct 20, 2022 • edited Loading

henryiii commented Oct 20, 2022 • edited Loading

henryiii commented Oct 20, 2022

henryiii commented Oct 20, 2022

henryiii commented Oct 21, 2022

eli-schwartz commented Oct 30, 2022

henryiii commented Oct 31, 2022

FFY00 commented Oct 31, 2022

FFY00 commented Oct 31, 2022

dnicolodi commented Nov 2, 2022

henryiii commented Nov 3, 2022

henryiii commented Nov 3, 2022

henryiii commented Nov 3, 2022

henryiii commented Nov 16, 2022

mkoeppe commented Nov 16, 2022

henryiii commented Nov 16, 2022 • edited Loading

mkoeppe commented Nov 16, 2022

FFY00 commented Nov 17, 2022

rgommers commented Nov 17, 2022

henryiii commented Nov 17, 2022

dnicolodi left a comment

Choose a reason for hiding this comment

dnicolodi Nov 17, 2022

Choose a reason for hiding this comment

dnicolodi Nov 17, 2022

Choose a reason for hiding this comment

dnicolodi commented Nov 17, 2022

dnicolodi commented Nov 17, 2022

dnicolodi commented Nov 17, 2022

dnicolodi commented Nov 17, 2022

FFY00 left a comment

Choose a reason for hiding this comment

eli-schwartz commented Nov 18, 2022

henryiii commented Nov 18, 2022

eli-schwartz commented Nov 18, 2022

henryiii commented Oct 15, 2022 •

edited

Loading

henryiii commented Oct 17, 2022 •

edited

Loading

henryiii commented Oct 20, 2022 •

edited

Loading

henryiii commented Oct 20, 2022 •

edited

Loading

henryiii commented Nov 16, 2022 •

edited

Loading