Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[MNT]: create build-requirements.txt and update dev-requirements.txt #28091

Merged
merged 3 commits into from
May 19, 2024
Merged

[MNT]: create build-requirements.txt and update dev-requirements.txt #28091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d4f2f7
MNT: Add meson-python, numpy, pybind11 and setuptools-scm in
abhi-jha Apr 16, 2024
0804fda
Remove ninja dependency
abhi-jha May 15, 2024
924e3f1
Remove cmake dependency
abhi-jha May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ commands:
- run:
name: Install Python dependencies
command: |
python -m pip install --user meson-python numpy pybind11 setuptools-scm
python -m pip install --user -r requirements/dev/build-requirements.txt
python -m pip install --user \
numpy<< parameters.numpy_version >> \
-r requirements/doc/doc-requirements.txt
Expand Down
2 changes: 1 addition & 1 deletion azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ stages:

- bash: |
python -m pip install --upgrade pip
python -m pip install --upgrade meson-python numpy pybind11 setuptools-scm
python -m pip install --upgrade -r requirements/dev/build-requirements.txt
python -m pip install -r requirements/testing/all.txt -r requirements/testing/extra.txt
displayName: 'Install dependencies with pip'

Expand Down
6 changes: 6 additions & 0 deletions requirements/dev/build-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pybind11
abhi-jha marked this conversation as resolved.
Show resolved Hide resolved
meson-python
cmake
ninja
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we need ninja for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly I got this list in the chat on gitter while triaging the first time. We can remove it if it is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah! Removing ninja has failed the CI build😮

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's on our build-dependencies list https://matplotlib.org/devdocs/install/dependencies.html#setup-dependencies b/c meson uses it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected that meson-python declaration jinja as a dependency. But if not, let’s include it explicitly- with a comment that it‘s for meson.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected that meson-python declaration ninja as a dependency.

It effectively does, if you are using build isolation, but not if you aren't. See contourpy/contourpy#260 for discussion with a meson maintainer and follow-up in contourpy/contourpy#270.

There is no good answer to explicitly including ninja in the build requirements file. If you don't include it and are not using build isolation then you may well need to explicitly install it, particularly on macos and windows. But if you do include it there are situations such as on cygwin where there are no wheels available and you will end up trying to build it from source, which may not be a pleasant experience, rather than using the prebuilt cygwin package (e.g. #26543).

I have a slight preference for not including it, so that build-requirements.txt is consistent with the [build-system] section of pyproject.toml. This does mean that some CI runs may require it to be explicitly installed using pip, sudo apt, or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that we need to put ninja dependency manually in the Ci scripts, separete from build-requirements.txt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to, but shouldn't do it by default. We should only consider doing so in specific CI runs that fail at the build stage due to lack of ninja.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed both ninja and cmake. I see a test coverage pipeline failing. Others have passed.

numpy
setuptools-scm
1 change: 1 addition & 0 deletions requirements/dev/dev-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
-r build-requirements.txt
-r ../doc/doc-requirements.txt
-r ../testing/all.txt
-r ../testing/extra.txt
Expand Down