-
Notifications
You must be signed in to change notification settings - Fork 141
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
fix: default to localhost on WSL #180
Conversation
Hi, thanks for the PR! I'm working in #179 on the reload code where I take a slightly different approach (the directory we use there is the directory of the app. |
@maartenbreddels Ok, should we close this PR then? |
Yeah, I think we can close this, but we could integrate your HOST workaround in that PR as well, I'll cherry-pick it |
Actually, why is your HOST env var set? is it also due to conda? |
This is done by WSL I believe... I first thought I couldn't run I use |
could we change it to also ignore specifically a hostname ending on w11 on windows platforms? (e.g. if sys.platform == ''win32'' and DEFAULT_HOST.endswith("-w11"). |
I found online that the safest way to do this was through |
ok, so it's |
Is my current "automatic WSL" setup. This is true even outside a |
But I don't want to disable HOST completely, since it is a common way to configure the domain/ip to bind the socket to. |
Sure, I simply recommend defaulting to either:
Or
Personally I haven't seen a tool previously default into |
I'm not sure I understand you. FLASK does it, and the nodejs servers I used, like the one that comes with nuxt. I often find out, because also my HOST variable is set to something invalid :) |
Okey! I understand where you're coming from. I've mainly seen changing host as a code or config option. But if it's default in flask I guess it could make sense. With that said, I was quite confused when starting solara and couldn't get it to work. All because it defaults into I think a compromise could be to default with |
Now that #167 is in, could you rebash this (or force push) with a fix to ignore HOST with |
@maartenbreddels what's the default here, a squash and merge? If so the commit-history doesn't matter. Please review changes. |
Depends, in any case master should be clean. So if a pr is a single change, it should become 1 commit. If you don’t force push I’ll squash merge. If you keep it clean and force push, also good. But 1 pr with multiple features (I’m ok with that is the build on each other) just force push and rebase. We try to stay close to conventional commits. |
Let's squash and merge, giving the new commit the title from the PR. That's usually how I do it. |
Co-authored-by: Maarten Breddels <[email protected]>
Awesome work @Lundez, thanks a lot! |
Make sure WSL works with defaultsolara run
(currently the$HOST
is populated in WSL and we cannot open that browserpath from Windows...Make sure we only reload the modules from CWD pathQ: why don't we always simply default intolocalhost
? To me it makes sense to uselocalhost
all the time unless specified in--host
.localhost
if hostname is same as on WSL (.*?-w1\d
)