-
Notifications
You must be signed in to change notification settings - Fork 33
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
Implement path to make mesop work in Windows #415
Conversation
Codecov ReportAttention: Patch coverage is
|
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.
Let me check something first
Fix mesop in windows refactor
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.
Looks good to me.
fastagency/ui/mesop/timer.py
Outdated
|
||
|
||
@contextmanager | ||
def patch_os_and_str() -> Iterator[None]: |
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 have to worry if 2 threads attempt to execute this at the same time? In that case the one that finishes first would possibly remove patch for the second one at too early stage.
But I am not sure if we could have 2 threads hitting this code at the same time.
Description
MesopUI
is not working inWindows
with weird error. Flask was complaining thatwakeup_component.js
. After a short conversation with @davorinrusevljan, I have learnt about the patching we do for mesop. This patching is not working inWindows
.I have investigated further and discovered a bug in
mesop.labs.web_component
- https://github.com/google/mesop/blob/main/mesop/labs/web_component.py#L34-L35. Here a/
is prepended to a path but this piece of code forgets the fact that the windows uses\
instead of/
.I have fixed it by introducing a patched function and using that patched function. Ideally this bug fix should be made at mesop repo itself but I don't want to wait till they make the fix as we have some potential Windows users.
Fixes #413
Type of change
Checklist