Skip to content

Fix Openssl3 + py37 on windows failure - #517

Merged
github-actions[bot] merged 4 commits into
conda-forge:3.7from
akrherz:openssl3_py37
Oct 13, 2021
Merged

Fix Openssl3 + py37 on windows failure#517
github-actions[bot] merged 4 commits into
conda-forge:3.7from
akrherz:openssl3_py37

Conversation

@akrherz

@akrherz akrherz commented Oct 13, 2021

Copy link
Copy Markdown

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Adds a test showing the openssl3 + python3.7 failure. Will iterate on a patch to then fix this.

Fixes #516

@conda-forge-linter

Copy link
Copy Markdown

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.

@akrherz
akrherz marked this pull request as draft October 13, 2021 10:49
@akrherz

akrherz commented Oct 13, 2021

Copy link
Copy Markdown
Author

Great, the failure reproduced only on windows + openssl3

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "D:\bld\python_1634123290171\_test_env\lib\ssl.py", line 589, in create_default_context
    context.load_default_certs(purpose)
  File "D:\bld\python_1634123290171\_test_env\lib\ssl.py", line 490, in load_default_certs
    self._load_windows_store_certs(storename, purpose)
  File "D:\bld\python_1634123290171\_test_env\lib\ssl.py", line 482, in _load_windows_store_certs
    self.load_verify_locations(cadata=certs)
ssl.SSLError: unknown error (_ssl.c:4034)

Patch iteration coming in a bit

@akrherz

akrherz commented Oct 13, 2021

Copy link
Copy Markdown
Author

My blissfully ignorant plan here is to iterate over cypthon 3.8 branch patches in this space and see when the failing test hopefully starts to pass. There are some larger patches that I am hoping to avoid. This first one may improve the logged error message to provide a better clue to the issue.

@akrherz

akrherz commented Oct 13, 2021

Copy link
Copy Markdown
Author

Whoa, win_64_openssl3 is green! Will bump build number and un-draft this once the other builds are green.

@akrherz
akrherz marked this pull request as ready for review October 13, 2021 14:10
@akrherz

akrherz commented Oct 13, 2021

Copy link
Copy Markdown
Author

OK, this should be ready to go. My understanding of why this patch fixes the issue is that the ASN1 reader was incorrectly erroring when it found an unexpected EOF on the bytearray, but it should have been OK. This code path is unique to windows.

@dopplershift

Copy link
Copy Markdown
Member

Wow, nice work @akrherz !

@akrherz

akrherz commented Oct 13, 2021

Copy link
Copy Markdown
Author

Greeeeeeeeeen!!!

fixes #516

@jakirkham

Copy link
Copy Markdown
Member

Woot! 🎉 Thanks @akrherz! 😄

Would it make sense to add the urlopen example from issue ( #516 ) as a test as well?

@akrherz

akrherz commented Oct 13, 2021

Copy link
Copy Markdown
Author

Would it make sense to add the urlopen example from issue ( #516 ) as a test as well?

I don't believe so as that would introduce a network availability requirement for the test to succeed. I added a test that exercises the same failing codepath.

@jakirkham

Copy link
Copy Markdown
Member

@conda-forge/python, could someone please take a second look at this?

@jakirkham

Copy link
Copy Markdown
Member

cc @chenghlee @h-vetinari (for awareness)

@isuruf isuruf added the automerge Merge the PR when CI passes label Oct 13, 2021
@jakirkham

Copy link
Copy Markdown
Member

Thanks Isuru! 😄

@github-actions
github-actions Bot merged commit 3c6f0f7 into conda-forge:3.7 Oct 13, 2021
@github-actions

Copy link
Copy Markdown
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • drone: passed
  • travis: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@xylar

xylar commented Oct 14, 2021

Copy link
Copy Markdown

@akrherz, thanks very much for fixing this!

@h-vetinari

h-vetinari commented Oct 14, 2021

Copy link
Copy Markdown
Member

Thanks a lot for tackling this (so far I only responded with emojis...)! :)

Unfortunately, this now leads to a hang in the cryptography test suite. I'm in the process of opening a new issue, just thought I'd let people in the thread know.

Edit: #518

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Merge the PR when CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants