Skip to content
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 remote support in packaged apps #13584

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Fix remote support in packaged apps #13584

merged 2 commits into from
Apr 17, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Apr 10, 2024

What it does

Closes #13140

Currently. the ApplicationPackage service is bound this way:

bind(ApplicationPackage).toDynamicValue(({ container }) => {
  const { projectPath } = container.get(BackendApplicationCliContribution);
  return new ApplicationPackage({ projectPath });
}).inSingletonScope();

However, the projectPath remains undefined, until the CLI app lifecycle has completed. This means that the ApplicationPackage may already be injected into other services with its projectPath undefined. This seems to be the case in eclipse-theia/theia-ide#350 and in #13140.

This change changes the way the projectPath is set. Instead of waiting until the CLI lifecycle completes, we set the BackendApplicationPath as soon as possible (at js load time). It didn't really make a lot of sense for the projectPath to be a CLI setting anyway, given that we use it to find our own code.

How to test

  1. Use the SSH remote feature and assert that it works as expected
  2. Open a webview from a plugin and assert that it loads and works as expected

Review checklist

Reminder for reviewers

@msujew msujew added the remote issues related to the remote functionality label Apr 10, 2024
@msujew
Copy link
Member Author

msujew commented Apr 10, 2024

cc @jfaltermeier This should fix eclipse-theia/theia-ide#350. Can you test it?

@jfaltermeier
Copy link
Contributor

@msujew Yes I can test it tomorrow.

@jfaltermeier
Copy link
Contributor

jfaltermeier commented Apr 11, 2024

Hi, so I've built a local next version based on your branch and consumed it in Blueprint.
I then ran into an error message "Server error while downloading native dependency from", because there is no github release matching my next version, from where it would get the native parts.
I then more or less hardcoded this URL to the last Github Release and built a new local next version.

With this I ran into the same issue as before (eclipse-theia/theia-ide#350 (comment))

But I don't know how the remote package works exactly, so maybe with above "fix"/hack it is not expected to work.
Do I have to rebuild those native parts as well? Do the versions have to match for some copying to work? (e.g. I can see that the .theia-ide-electron-app-1.48.300-remote directory exists in the container including the native parts. This has the version number of the last release and not my local next version, so maybe this is an issue?)

@msujew
Copy link
Member Author

msujew commented Apr 11, 2024

@jfaltermeier Thanks for testing. Looking closer at the error, I believe that I misunderstood the source of the issue. Sorry for that :/

I'll try to debug this myself later to see what goes wrong.

@msujew
Copy link
Member Author

msujew commented Apr 11, 2024

@jfaltermeier Alright, found the issue. When the application is packaged, the path.relative call returned bogus data for some reason. In fact, the mapping using path.relative wasn't necessary at all, since glob already returns all file paths in relative form. See latest commit on this PR. I've applied the same fix to the theia-blueprint repo and was successfully able to run the dev-container and remote SSH feature.

Note that it still required the other change for me, since otherwise the projectPath wasn't correctly computed.

Copy link
Contributor

@jfaltermeier jfaltermeier left a 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, thank you!
I was also able to get a dev-container running in Theia Blueprint as well:
ksnip_20240417-142232

@msujew msujew changed the title Fix application project path injection Fix remote support in packaged apps Apr 17, 2024
@msujew msujew merged commit a87c46a into master Apr 17, 2024
14 checks passed
@msujew msujew deleted the msujew/fix-app-path branch April 17, 2024 13:31
@github-actions github-actions bot added this to the 1.49.0 milestone Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
remote issues related to the remote functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remote SSH not working
2 participants