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

Fall back on requests module when urllib fails to load a URL #8667

Merged
merged 2 commits into from
May 24, 2019

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented May 23, 2019

This can happen on systems where OpenSSL is too old.

This can happen on systems where OpenSSL is too old.
@dschuff dschuff requested a review from kripken May 23, 2019 22:13
Copy link
Member

@quantum5 quantum5 left a comment

Choose a reason for hiding this comment

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

Why not just use requests to start with? It's the recommended way to do HTTP requests in python.

If it is a dependency issue, maybe fall back to urllib instead of the other way around?

data = f.read()
except URLError as e:
try:
import requests
Copy link
Member

Choose a reason for hiding this comment

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

may be worth a comment that requests is used as a workaround for an older python?

@dschuff
Copy link
Member Author

dschuff commented May 23, 2019

@quantum5 Is requests in py3k by default? If so, then that seems like a good idea. Otherwise I disagree that it should be the 'recommended' way to do things :)

@quantum5
Copy link
Member

Unfortunately, requests is not part of the standard library. It is a rather popular library though and much more pleasant to use than urllib. I would personally use it first and use urllib as a fallback, but either way works.

@dschuff dschuff merged commit 924a97d into incoming May 24, 2019
@sbc100 sbc100 deleted the requests branch May 24, 2019 09:59
@sbc100
Copy link
Collaborator

sbc100 commented May 24, 2019

The problem is that we don't have any way today to use modules not part the standard library because we don't yet have any way to run any "install" or "post check-out" step before emscripten is expected to be functionally. For the same reason we don't have any git sub-modules yes. Once we improve the emsdk emscripten packaging story I think we can introduce some kind of "post-sync" step that git developers are expected to run, and then have this performes as part of the emsdk packaging (perhaps included in the emscripten release tar balls).

@quantum5
Copy link
Member

Maybe an alternative to consider is to use setup.py, so then you can pip install -e . to install emscripten locally, as well as have it install dependencies. It would also put all the commands into PATH. Perhaps one day the end user can do pip install emscripten.

@sbc100
Copy link
Collaborator

sbc100 commented May 24, 2019

Sure. From my POV, it not so much about the specific command that needs to run, more about how to enforce that developers run that command to stay up-to-date. Emscripten is a strange mix since we are have both python and node module dependencies in the same tree. The npm people want to be able to to do npm install emscripten too :) See #5774.

Node developers are used to running npm install after a git pull, python uses might run pip install requirements.txt or setup.py.. and everyone needs to run git submodule-update. I guess we would want a single script to do any/all of this and some way to warn developers who forget to run it?

quantum5 pushed a commit to quantum5/emscripten that referenced this pull request May 24, 2019
@sbc100
Copy link
Collaborator

sbc100 commented May 29, 2020

I'm a little worried that this means we need to run out tests on both with and without the requests module.

A recent bug that does use the requests module: #11283

I this still needed? I don't see the original bug that triggered this change? I wonder if we can revert it now. Having all those different methods seems like a bad idea.

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
sbc100 added a commit that referenced this pull request Apr 12, 2021
It turns out that the www.ijg.org refused requests from the urllib
library for some reason.

This issues was being masked by the fact that system_libs.py prefers to
use `requests` when it is installed.  This change undoes that.  This
feature was originally added in #8667 to deal with older python
installations but these days we supply our own python as part of emsdk
so we should be able to depend on a recent/correct version.  Having the
fallback here just makes testing harder.

Fixes: #13869
sbc100 added a commit that referenced this pull request Apr 12, 2021
It turns out that the www.ijg.org refused requests from the urllib
library for some reason.

This issues was being masked by the fact that system_libs.py prefers to
use `requests` when it is installed.  This change undoes that.  This
feature was originally added in #8667 to deal with older python
installations but these days we supply our own python as part of emsdk
so we should be able to depend on a recent/correct version.  Having the
fallback here just makes testing harder.

Fixes: #13869
sbc100 added a commit that referenced this pull request Apr 13, 2021
It turns out that the www.ijg.org refused requests from the urllib
library for some reason.

This issues was being masked by the fact that system_libs.py prefers to
use `requests` when it is installed.  This change undoes that.  This
feature was originally added in #8667 to deal with older python
installations but these days we supply our own python as part of emsdk
so we should be able to depend on a recent/correct version.  Having the
fallback here just makes testing harder.

Fixes: #13869
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.

4 participants