Skip to content

Conversation

@moreshiny
Copy link
Contributor

@moreshiny moreshiny commented Dec 8, 2022

Checklist

@conda-forge-linter
Copy link

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 (recipe) and found it was in an excellent condition.

@moreshiny
Copy link
Contributor Author

moreshiny commented Dec 8, 2022

@conda-forge-admin, please rerender

@moreshiny moreshiny force-pushed the 3.7 branch 2 times, most recently from e5a4470 to 7c971b8 Compare December 9, 2022 01:53
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GutHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/python-feedstock/actions/runs/3653774329.

@conda-forge-webservices
Copy link
Contributor

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 (recipe) and found it was in an excellent condition.

@moreshiny
Copy link
Contributor Author

Would anyone have chance to look this over?

md5: df3b8ad6e2ab8f198d65ecf68816bf42
patches:
# https://bugs.python.org/issue40263
- patches/0000-Fix-off-by-one-error-in-_winapi_WaitForMultipleObjec.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it really necessary to rename all these files?

@hmaarrfk
Copy link
Contributor

Generally speaking, we've stopped supporting python 3.7. So renaming all the patches is somewhat disruptive for the review process

@hmaarrfk
Copy link
Contributor

There seems to be a few patches that you've made a marked improvement on. It would be good if in the pull request, you left everything you could untouched. i think you can specify a starting number for the git format patch command. that should alleviate alot of the changes.

@moreshiny
Copy link
Contributor Author

@hmaarrfk Yeah the reordering and cleanup changes mostly came from me fighting with the patches but the most minimal set of changes is actually quite small and related to zlib/bzip2/openssl version changes.

I've reduced it to that. This means that the chunk ranges are not correct but the patches apply anyway. Let me know if I should bring those and/or the patch index changes back in.

- <opensslDir>$(ExternalsDir)openssl-1.1.1n\</opensslDir>
- <opensslOutDir>$(ExternalsDir)openssl-bin-1.1.1n\$(ArchName)\</opensslOutDir>
- <opensslIncludeDir>$(opensslOutDir)include</opensslIncludeDir>
+ <opensslDir>$(OPENSSL_DIR)\</opensslDir>
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain in a few words why this change is necessary now? it uses an environment variable which is new.

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 not changed line 91 with the OPENSSL_DIR variable, this has been in the patch file for a long time. It just looks that way because it's a diff of a patch file which Github doesn't display that well.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood. perfect. Thank you for pointing that out.

It seems that python changed the version of openssl they built against.

Our patch also previously replaced it with an environment variable.

@hmaarrfk
Copy link
Contributor

@conda-forge/python I want to bring to your attention that this PR would introduce the breaking str->int conversion change

I know we decided

to release these on the supported versions of python, but i'm not sure what we want to do here, given that we "stopped supporting" python 3.7.

I'm ok with releasing this, but I just wanted to bring it up.

https://docs.python.org/3.7/whatsnew/changelog.html#python-3-7-14-final

@isuruf
Copy link
Member

isuruf commented Jan 14, 2023

@hmaarrfk
Copy link
Contributor

Please use https://github.com/conda-forge/python-feedstock/blob/main/recipe/patches/README.md when updating the patches here.

Sorry for the noise on reformatting the patches. I defer to Isuru's expertise here.

@moreshiny
Copy link
Contributor Author

Please use https://github.com/conda-forge/python-feedstock/blob/main/recipe/patches/README.md when updating the patches here.

Sorry for the noise on reformatting the patches. I defer to Isuru's expertise here.

No problem at all.

@isuruf Just to be clear, I would:

Let me know if that's ok for you then I will regenerate all patches on this and the other version PRs (and add/update the patch Readme as needed).

Patches are now numbered and named as they are generated by
the standard procedure.
@jakirkham
Copy link
Member

@conda-forge-admin, please re-render

conda-forge-webservices[bot] added 4 commits March 24, 2023 07:21
@moreshiny moreshiny closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants