8000 don't require ninja to come from PyPI by eli-schwartz · Pull Request #260 · contourpy/contourpy · GitHub
[go: up one dir, main page]

Skip to content

don't require ninja to come from PyPI #260

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

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

eli-schwartz
Copy link
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
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
Contributor Author

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

Good point. pandas-dev/pandas#54244

@eli-schwartz
Copy link
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
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
0