-
-
Notifications
You must be signed in to change notification settings - Fork 515
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
Add Python 3.11 migration #3573
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
primary_key: python | ||
ordering: | ||
python: | ||
- 3.6.* *_cpython |
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 we need to list 3.6 and 3.7 here?
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 was wondering the same thing. Saw this in our PyPy migrator
conda-forge-pinning-feedstock/recipe/migrations/pypy38.yaml
Lines 8 to 9 in 28885e3
- 3.6.* *_cpython | |
- 3.7.* *_cpython |
Also we had this in the Python 3.10 migrator ( #2008 ) ( even after we dropped Python 3.6, #1955 )
So thinking this is needed for other reasons. Hopefully someone can clarify. Also happy to drop if I've misunderstood
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.
Can you link a PR where this migration has been applied locally? If that works, all is fine.
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.
Conda-smithy keeps trying to delete the migrator altogether (as it not merged in conda-forge-pinning). Forgetting the option to tell conda-smithy to override this behavior
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.
It shouldn't hurt to have superfluous entries. Pretty sure we can remove 3.6.
To remove 3.7 we need to #2623 (comment) :
[...] drop 3.7 when 3.11 comes out and we start the migration in October. [...]
As per the comment above, this PR should remove 3.7 from the current pinnings already (because AFAIR we currently don't have code to do add+remove in a single migration).
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've made a PR in conda-forge/pytest-feedstock#152
(The option is --exclusive-config-file ../conda-forge-pinning-feedstock/recipe/conda_build_config.yaml
)
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.
As per the comment above, this PR should remove 3.7 from the current pinnings already (because AFAIR we currently don't have code to do add+remove in a single migration).
(NB: If there surfaced new/other (good/important) reasons to drop it after the migration, that's fine by me. I was just reciting the comment/core decision from a few months back.)
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.
Ah ok was looking for a way to drop Python 3.7 in the migration, but didn't see it. Went ahead and deleted from the pinning file directly 👍
Thanks Chris! 🙏 Yeah had tried that option, but it was still deleting it for me. Probably missing something. Maybe it matters what the path to the pinning file is? In any event, looks like the PR is doing the right thing, which is encouraging 😄
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.
It needs to be able to find the correct migrations/
folder next to conda_build_config.yaml
so perhaps that was your issue?
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.
Oh I see. Thanks for clarifying! 🙏
Yeah probably. Was trying to point it to conda_build_config.yaml
installed in base
as opposed to a feedstock, which would not be adjacent to migrations/
😅
Co-authored-by: Uwe L. Korn <xhochy@users.noreply.github.com>
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 guess this can be unmarked as draft?
Actually we probably need to fix crossenv before merging this to avoid lots of broken PRs: conda-forge/crossenv-feedstock#34 |
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've cross-compiled numpy
with this now so I think this can be merged
cc @conda-forge/core |
Thanks Chris! Also thanks everyone for the reviews 🙏 |
Are python 3.11 packages supposed to be getting built now? I'm trying to build new packages in conda-forge/openmm-feedstock#95, but it still isn't building for 3.11. |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)