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.

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

Requirements dev file #2699

Closed
wants to merge 8 commits into from
Closed

Requirements dev file #2699

wants to merge 8 commits into from

Conversation

oddbookworm
Copy link
Member

Continuation of #2698, using a branch on this repo instead of my own

@oddbookworm oddbookworm requested a review from a team as a code owner January 29, 2024 01:56
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

I'm not sure if CI should be using this requirements dev file? It negatively affects build times, and in some builds this has an extreme effect.

black<=23.7.0
clang-format<=17.0.6
numpy<=1.26.3
opencv-python<=4.8.1.78
Copy link
Member

Choose a reason for hiding this comment

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

numpy and opencv aren't really dev dependencies, they're more like optional runtime dependencies

Copy link
Member

Choose a reason for hiding this comment

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

It also looks like numpy and opencv failed builds on pypy are the cause of most? all? of the current CI failures.

What I'm thinking right now is that opencv should be dropped from here and numpy should be hard pinned to the last version that ships pypy3.8 wheels (1.24.4)

We test in CI with numpy, we don't test with opencv.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't pin to 1.24.4 numpy because it didn't ship with 3.12 wheels

Copy link
Member

Choose a reason for hiding this comment

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

Probably if we are going to use numpy as a dependency we should synch to their 3.5 years/42 month community python version deprecation schedule as well or we will keep having these out of synch issues. Their schedule is defined in this NEP:

https://numpy.org/neps/nep-0029-deprecation_policy.html

3.8 was deprecated on April 14th 2023 and 3.9 will be deprecated this April.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep a bit more extended support, as we've been doing.

I guess we should not pin numpy in the requirements file.

Also, numpy 2 is coming out if y'all haven't heard.

Copy link
Member

Choose a reason for hiding this comment

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

What would be nice is to pass the equivalent of --prefer-binary specifically on the numpy dependency (and also maybe even opencv if we are adding that). Should fix our issues ig, idk

@oddbookworm
Copy link
Member Author

I'm not sure if CI should be using this requirements dev file? It negatively affects build times, and in some builds this has an extreme effect.

I think that's part of the point of having one. If we pin the dep versions, then when something changes that would affect us, the CI would use the new version and would break. Unless we manually pin the versions in the CI too, that wouldn't really fix anything.

requirements-dev.txt Outdated Show resolved Hide resolved
@narilee2006
Copy link
Contributor

Why not requirements.txt?

@Starbuck5
Copy link
Member

Requirements-dev is a common way to express development dependencies, I think. These aren't dependencies for pygame-ce's runtime, they are dependencies for building/testing pygame-ce.

@Starbuck5
Copy link
Member

Does this need to be updated for/because of #2744 ?

@oddbookworm
Copy link
Member Author

Yup, about to push a commit updating

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@yunline yunline added CI Issue with the Continuous Integration (CI), the actions/bots that test things build Compiling stuff labels Mar 20, 2024
@oddbookworm
Copy link
Member Author

I'll hold off on this until #2824 is resolved, needs updates from meson build too

@oddbookworm oddbookworm requested a review from ankith26 May 12, 2024 03:22
@@ -0,0 +1,5 @@
ruff<=0.4.4
clang-format<=18.1.1
Copy link
Member

Choose a reason for hiding this comment

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

In my understanding, these won't be dependencies, as pre-commit automatically takes care of it behind the scenes.

clang-format<=18.1.1
numpy
pylint<=3.1.0
Sphinx<=7.2.6
Copy link
Member

Choose a reason for hiding this comment

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

Also, if #2853 is merged this line won't be needed, as the Sphinx dependency will be tracked in pyproject.toml along with other build-time dependencies.

@oddbookworm oddbookworm force-pushed the requirements-dev-file branch from d8d6d41 to b7e9424 Compare May 20, 2024 20:47
@@ -0,0 +1,3 @@
numpy
pylint<=3.1.0
meson
Copy link
Member

Choose a reason for hiding this comment

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

meson is already covered due to specifying meson-python in [build-system] requires of pyproject.toml, so like the rest it need not be re-specified here.

However, you could specify mypy here instead, it's used by the stubcheck program we have

@@ -1,4 +1,3 @@
name: Windows
Copy link
Member

Choose a reason for hiding this comment

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

why was this line removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, must've been a wonky merge conflict fix

@ankith26 ankith26 marked this pull request as draft July 7, 2024 19:36
@ankith26
Copy link
Member

This PR has been superseded by dev.py and therefore I am closing this. But still, thanks for working on it as the work done here still inspired some design and behaviour of the current dev.py.

@ankith26 ankith26 closed this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compiling stuff CI Issue with the Continuous Integration (CI), the actions/bots that test things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants