-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Reloader fixes #531
Reloader fixes #531
Conversation
8377106
to
1842d71
Compare
@@ -679,6 +693,13 @@ def run_simple(hostname, port, application, use_reloader=False, | |||
automatically create one, or `None` to disable SSL | |||
(which is the default). | |||
""" | |||
if isinstance(application, string_types): | |||
if use_reloader: |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
a95fa8c
to
9b3f7f0
Compare
Replaced the empty |
9b3f7f0
to
329b26f
Compare
Rebased to work with the latest reloader changes. |
I generally like that but i would like to switch the cli over to click as well. This is optional behavior and i see now reason why it could not use it. |
Hmm, now I wonder how likely it is that some idiotic code puts non-strings into the syspath and makes the json serialization crash. I don't think we should care for that case, but it is something to consider. |
That's a good point @untitaker for a related reason: json uses unicode. File system paths can be either unicode or bytes. So json dumping is probably going to have absolutely terrible effects. |
There are alternative approaches to this issue mentioned in the outdated diffs. I still think reconstructing the exact command has potential, but using loader for this is definetly too unstable. |
Generally also we need to consider that Flask must have a related problem unless i already worked around it there. |
Maybe we should just pickle the path. The serialized format is only stored for a fraction of a second. |
#461 is a very concrete issue from a end user. |
only pickle the path if we absolutize the items. |
What do you mean by that? |
@untitaker regarding "I still think reconstructing the exact command has potential", keep in mind on Python 2 the |
I only need it when the interpreter was started with
Isn't that always the case? |
Sure, but we are back to undocumented behavior. Are we sure this isn't ever going to change?
Yes, if you look at the value of
Here is what happens when you run it directly vs. with -m:
But I still think this is too obscure to use. |
AFAICT the |
The loader API is so fundamentally broken and changes all the time, i would stay away from it. By absolitize i mean |
16edc59
to
afc2c67
Compare
Bump, @miguelgrinberg could you serialize the path using pickle and apply |
Sorry, I dropped the ball on this. I'll change to pickle, that's not a problem. @mitsuhiko did not explain why it is necessary to make the |
329b26f
to
d44aa06
Compare
d44aa06
to
102c3ab
Compare
@untitaker the pickling appears to work fine, but I'm seeing a strange bug in Python 3.3 and 3.4, so there is still some work before this can be used. As we discussed, when starting the app with Need to think about how to handle this, it seems we need to restore |
Logged this issue as #593, since it is unrelated to this one. |
I played around with this a bit and i can't see a way how we could solve this problem by copying sys.path at all. Even if we manage to replace the whole search path before any imports are processed (which i tried by setting |
f9d0f4e
to
cbec4d1
Compare
There are two somewhat related reloader fixes in this PR:
Let me know what you think.
Miguel