-
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
DevContainers: Support THEIA_DEFAULT_PLUGINS #14530
Conversation
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
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.
@jonah-iden thanks for looking into this. Could you maybe explain a little bit more in detail on why you went with this approach?
As far as i can tell (but i am not too familiar with the code), we missed the plugin directories provided via THEIA_DEFAULT_PLUGINS before this change, right? Then this addition definately makes sense.
However, when an adoptor only provides the plugins via the cli and not via the env it will still not work, right? Because that was the original issue described in #14137.
@sgraband could i have misunderstood the issue? |
I actually thought that the Theia IDE sets the plugins via the cli param, and that the cli param did not work in a bundled environment. However i quickly checked and you are right. I will create a local build of the Theia IDE with your fix and check if it works 👍 Sorry for the confusion! |
I just tested it and i am afraid it still does not work in a packaged environment (local Theia IDE build with your changes). |
Thanks for testing. Do you know if in you test installation everything is bundled together into an asar archive? |
I made a full local build of the theia ide following this: https://github.com/eclipse-theia/theia-ide/blob/jf/local-theia-build/local-theia-build.md. I can try it out again later on, but i am pretty sure i did not make a mistake. Also note, that i am on ubuntu, maybe this makes a difference somehow? |
thanks i'll try to do a local build as well |
Sadly i can not get the local build to work even in wsl. Probably becuase docker is still running on windows and the dockerfile is build for unix. |
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 just retested it with packaging the example electron app and everything works as intended. I think this is good to go. Should any issues arise when updating the Theia IDE to the next version i would investigate again on why this happens. I will create a reminder for myself.
Thanks for the improvement 🎉
What it does
Fixes #14137
Adds support for copying and using plugins defined by local-dir through THEIA_DEFAULT_PLUGINS env variable
How to test
remove the "--plugins=local-dir:../../plugins" fro your start target,
set THEIA_DEFAULT_PLUGINS environment variable to "--plugins=local-dir:../../plugins".
reopen in any dev container. See that the plugins should be correctly applied
Follow-ups
Review checklist
Reminder for reviewers