Skip to content

don't require ninja to come from PyPI - #260

Merged
ianthomas23 merged 1 commit into
contourpy:mainfrom
eli-schwartz:pyproject-ninja
Jul 24, 2023
Merged

ianthomas23 merged 1 commit into
contourpy:mainfrom
eli-schwartz:pyproject-ninja

Conversation

@eli-schwartz

Copy link
Copy Markdown
Contributor

In pyproject.toml, the baseline requirements for building are supposed to be listed.

Many end users have a recent enough system-provided ninja, and don't need to install a new copy into every contourpy isolated build virtual environment. This is handled inside meson-python itself, which dynamically adds an additional build requirement on ninja via the backend hook get_requires_for_build_sdist, but only if a suitable system version is not found.

This is especially helpful on systems where an OS ninja is installed, but the platform is obscure enough that there are no PyPI "ninja" wheels, thus forcing pip install contourpy to download an additional copy of the ninja source code and build it via cmake before beginning to install contourpy itself. This is not guaranteed to work...

The ninja entry in build_requirements.txt is left alone because it's opt-in and used for CI. In CI pipelines, it may make sense to be fairly specific about which providers you want to use for any given dependency, and that's a workflow preference encoded as a workflow build command. Since it doesn't prevent people from using the baseline install command, this commit makes no value judgment regarding it.

In pyproject.toml, the baseline requirements for building are supposed
to be listed.

Many end users have a recent enough system-provided ninja, and don't
need to install a new copy into every contourpy isolated build virtual
environment. This is handled inside meson-python itself, which
dynamically adds an additional build requirement on ninja via the
backend hook `get_requires_for_build_sdist`, but only if a suitable
system version is not found.

This is especially helpful on systems where an OS ninja is installed,
but the platform is obscure enough that there are no PyPI "ninja"
wheels, thus forcing `pip install contourpy` to download an additional
copy of the ninja source code and build it via cmake before beginning to
install contourpy itself. This is not guaranteed to work...

The ninja entry in build_requirements.txt is left alone because it's
opt-in and used for CI. In CI pipelines, it may make sense to be fairly
specific about which providers you want to use for any given dependency,
and that's a workflow preference encoded as a workflow build command.
Since it doesn't prevent people from using the baseline install command,
this commit makes no value judgment regarding it.
@ianthomas23

Copy link
Copy Markdown
Member

Thanks @eli-schwartz. I'll merge as-is and probably update build_requirements.txt in line with this after some consideration.

I note that I saw this pattern elsewhere, in pandas.

@ianthomas23
ianthomas23 merged commit 7132cf4 into contourpy:main Jul 24, 2023
@eli-schwartz
eli-schwartz deleted the pyproject-ninja branch July 24, 2023 18:04
@eli-schwartz

Copy link
Copy Markdown
Contributor Author

I note that I saw this pattern elsewhere, in pandas.

Good point. pandas-dev/pandas#54244

@eli-schwartz

Copy link
Copy Markdown
Contributor Author

I'll merge as-is and probably update build_requirements.txt in line with this after some consideration.

# Install requirements when not using build isolation.
python -m pip install -r build_requirements.txt

As observed on the pandas tracker, "I'd prefer not to rely on the Github runners having ninja installed." thus pandas will probably keep installing ninja explicitly in CI.

In particular, when building contourpy (or pandas of course) without build isolation, the need to preinstall requirements as explained in the contourpy comments means that you cannot rely on pyproject.toml build-requires, and meson-python's behavior of "automatically inject an additional build-requires for ninja, but only if it isn't installed as part of the OS" does not apply; for cases where you intend to install build_requirements.txt, you do need to either install ninja too, or run that meson-python logic yourself, check whether a good enough ninja is installed, and decide whether to install ninja.

The simple solution is of course to always install it; in other words, avoid updating build_requirements.txt.

@ianthomas23

Copy link
Copy Markdown
Member

After consideration, what I am going to do is remove it from build_requirements.txt which I view as equivalent to the build-system section of pyproject.toml for the use case not supported by pip of "I would like to pip install without build isolation but first preinstall the build-system requirements". There is clarity in having the two the same.

I don't have a section in the contourpy docs stating that a compiler toolchain is required. I didn't think about it but probably should have done, and I will add one which will include ninja as part of the toolchain.

Originally when I was working on the switch to meson I didn't have the explicit ninja requirement. I cannot remember all the details but there was some failed build in one of my CI experiments because ninja wasn't preinstalled and this was the workaround I tried. Evidently it is not needed now, and there could well be other cruft lying around that is no longer needed. It has been a time of great change (in contourpy, meson, meson-python, etc) so I think that some of this is inevitable but will be improved in the long run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants