-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 self-hosting on Windows #6316
Conversation
type: LogType.Info | ||
}); | ||
return Promise.resolve(); | ||
} | ||
|
||
protected unregisterWatchScript(path: string): void { | ||
this.watchCompilationRegistry.delete(path); | ||
protected unregisterWatchScript(pluginRootPath: string): void { |
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.
Hello, why do we need to rename path to pluginRootPath, it's adding extra changes ?
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.
I think it improves the code, as it better describes the purpose of the variable and puts it more in line with the rest of the module.
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.
@westbury Was there a compilation warning/error because of a shadowed module variable path
?
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.
@akosyakov The rename was not technically required, as I could have used a different name for the path import. I thought it was a good idea to make the code a little bit clearer by using the same name for paths that are specifically a path to the plugin root.
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.
Tested on Linux only (Fedora 30) - works for me.
@kittaakos Do you have time to test this on Windows? This PR contains the fixes for the problems both you and I found when trying to validate #5849 on Windows. @akosyakov I seem to have lost my permissions to assign reviewers, add tags etc, probably as a result of the move to eclipse-theia. I have no unaccepted invites that I can find. |
@westbury please check there should be an invitation email since we moved a repo to a new org cc @marcdumais-work |
@westbury I also could not find it at first. Here's the email title to search-for: |
@marcdumais-work I have not been able to find the invitation anywhere. I would expect to find it at this URL irrespective of any missing e-mail: |
Hi @westbury , Just a sanity check: have you checked with your email address registered at the Foundation? It could be that the email ended-up in the spam folder and was automatically purged after a little while. If still unresolved, I would suggest opening a bugzilla about his: start here and select component: GitHub. In the description mention that you did not receive the invite to join the |
Yes 👍 |
I cannot build it on my Windows image anymore. It used to work two weeks ago:
Were there any native dependency udpates recently? |
I can build the from the |
Signed-off-by: Nigel Westbury <nigelipse@miegel.org>
f0f72f0
to
0090cd2
Compare
Done, though I see nothing in the intervening commits that would have caused or fixed the build failure.
Not since the previous base of this commit. For me I have never found the build environment as reliable on Windows as it is on Linux. It may be specifically my machine rather than Windows in general but I always just re-clone when I see something like this on Windows. |
Thanks!
The environment is (quite) reliable on my Windows image. For instance, I had no issues building your other PR. I do not know 😊 Anyways, thank you! |
I could build it this time, but it does not work for me. Or maybe I am missing a step or two. I did the followings:
|
I was using 0090cd2. |
@marcdumais-work Thanks for the advice. That's sorted it out. Apparently it was 'a recently discovered edge case in the sync tool'. |
You're using Electron :-) I had only tested the browser version. I see the following when I try it in Electron (and nothing happens in the UI): root ERROR Error when loading drives. Error: Request 'getDrives' failed |
This problem is fixed by rebuilding drivelist natives (this may have been an issue on my machine only). However there is a larger problem in starting the Electron instance on Windows. This may take some time (for me at least) to determine the cause. So we can either 1) reduce the scope of this PR to the browser only and merge if it works on the browser, or 2) leave it hanging around until someone figures out the electron issue. |
The fixes here are required to use self-hosting on Windows. So that browser users are not blocked, I have opened #6846 for the separate electron issue and am merging these fixes. |
What it does
While validating #5849 on Windows I had to fix a couple of unrelated bugs. These fixes are required to get self hosting working on Windows.
There are two fixes here.
How to test
On Windows, run Hosted mode: Debug instance (use, for example, https://github.com/tom-shan/theia-plugin-hello-world). With the fixes you should see the self-hosting successfully start.
Review checklist
Reminder for reviewers