-
Notifications
You must be signed in to change notification settings - Fork 239
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
Macos certif #447
Macos certif #447
Conversation
Another solution I've seen mentioned that might include Python 3.5 is: pip install certifi
CERT_PATH=$(python -m certifi)
export SSL_CERT_FILE=${CERT_PATH}
export REQUESTS_CA_BUNDLE=${CERT_PATH} But I think the problem is that Python 3.6+ have their own copy of OpenSSL, while 3.5 does not and uses the system copy, so does this even show up at all for 3.5? |
The system copy was so old on python 3.5 that it did not support TLS 1.2 which caused some issue. Its openssl version has been patched in #71 not to use the system one.
It shall be patched like the other versions. |
I just tried forcing the TLS protocol version in my branch for vispy and it worked! I made sure not to use the dev version of cibuildwheel and I don't think I was skipping the test or anything. I think this is the main cause. Can someone summarize all of the issues related to why cibuildwheel started doing that and why it works? |
@joerick, I can reproduce the issue: https://github.com/mayeut/cibuildwheel/runs/1233247122?check_suite_focus=true#step:6:158 |
my test run does not finish yet, but basic on @mayeut I expand the test set. But I think that we should test more ssl context cases. EDIT also got error https://dev.azure.com/PartSeg/Open%20Source%20contrib/_build/results?buildId=1764&view=results |
@mayeut I got this for python 3.5
Did you have any idea? Or omit python 3.5 as there is plan to drop its support? |
This reverts commit 0980d5d.
That's because the folder |
@mayeut When omit |
I just tested "manually" the patch on my machine (i.e., just following the logic):
patch logic:
after:
So it should be working. Now investigating why it's failing in CI. |
With an updated test case to print
|
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.
With mode
changed with octal 0o775
, tests are passing as expected. c.f. https://github.com/mayeut/cibuildwheel/runs/1235348121?check_suite_focus=true
Co-authored-by: Matthieu Darbois <mayeut@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.
import stat | ||
import subprocess | ||
import sys | ||
STAT_0o775 = ( stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR |
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.
New line, before?
test/test_1_ssl.py
Outdated
data = urlopen("https://www.nist.gov", context=context) | ||
context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2) | ||
data = urlopen("https://www.nist.gov", context=context) | ||
data = urlopen("https://raw.githubusercontent.com/joerick/cibuildwheel/master/CI.md", context=context) |
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 assert something for the received data? E.g. just that it's not empty, or so?
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.
maybe we should, but then we should have some file with some fixed content.
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.
Not per se. You could just test if something non-empty was downloaded? Or take away the data =
, maybe? It just feels weird, all these useless assignments.
change for python 3.5 now ok, letting @YannickJadoul follow-up with his review
This reverts commit aaa1109.
Before merge I have one question. |
The test with c.f. https://www.ssllabs.com/ssl-pulse/ for an idea of currently deployed protocols. |
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, @Czaki! Looks good to me, now!
Thanks @Czaki ! |
@joerick 1.6.3? |
This PR is for fix #443
Because try of using
/Applications/Python 2.7/Install Certificates.command
fail I decide to vendor in script base on one present on my machine.I'm not sure what to do with python 3.5 so I skip this step on this version. @mayeut any suggestion?
The test for this case could be hard. We could download something from github.com, but any break in this functionality may produce error long time after change happen.