-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
[MNT]: create build-requirements.txt and update dev-requirements.txt #28091
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Thanks for opening this! 😁 |
hey @abhi-jha just wanna check in on this, do you have bandwith to finish up? |
@story645 Need some clarifications above. |
For anyone who is having the issue "No module named setuptools_scm" after doing "pip install pybind11 meson-python cmake ninja numpy", the fix is "pip install setuptools-scm". |
882728a
to
8de76ce
Compare
19a76c7
to
7302c6d
Compare
2455837
to
2b82d53
Compare
2b82d53
to
bc21e9e
Compare
bc21e9e
to
ef7097a
Compare
I updated the title to reflect what the PR is now doing, feel free to change if inaccurate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the failed build logs are complaining about setuptools-scm and that's missing from the build-requirements file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing course & going for the requirements file!
Not sure if this needs second review, let me know if you want us to squash merge or if you'll rebase to one commit.
https://matplotlib.org/devdocs/devel/development_workflow.html#rewrite-commit-history
requirements/doc/doc-requirements.txt * Update the CI build files to refer to the requirments file rather than installing dependencies manually
2ed77cf
to
0d4f2f7
Compare
Rebased. |
@story645 Looks good to be merged. Not sure how the review process goes. Let me know if there's something that needs to be done from my end. |
It's just that this to me feels on the bubble of our does it need 1 or 2 reviews to merge guidelines (https://matplotlib.org/devdocs/devel/pr_guide.html#merging) - it's a minor infrastructure change to avoid a doc change, but it's not a necessary infrastructure change to get stuff working again. |
pybind11 | ||
meson-python | ||
cmake | ||
ninja |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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😮
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
Try also removing cmake (to rely on system cmake on CI). If the user has cmake and ninja avaialable, we should not force a reinstallation of them on top from pypi. They should be treated/documented the same way we treat gcc? |
Windows CI tests have failed. Is there more to it? |
All the CI pipelines apart from the test coverage seems to have passed. How do I get the coverage to pass? |
@abhi-jha we are not strict about CodeCov passing https://matplotlib.org/devdocs/devel/development_workflow.html#automated-tests This looks like it should be ready to merge as it has two approvals. @story645 @timhoffm is there anything more to do here? |
Thanks @abhi-jha! |
PR summary
Adding explicit docs to make sure the project installs in editable mode
Ref : https://matrix.to/#/!BXmyZMTnRjWJldDRLV:gitter.im/$iAbowa1uqTBT5Kvdqhvn_0hj9w1g0s0E7BOduGgv57g?via=gitter.im&via=matrix.org&via=cadair.com
PR checklist