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

Run a binary server #316

Merged
merged 1 commit into from
Jan 28, 2021
Merged

Conversation

datho7561
Copy link
Contributor

@datho7561 datho7561 commented Aug 27, 2020

If the user has no Java installed, run a binary version of LemMinX. There is a setting xml.server.preferBinary, which, when set to true, causes the binary LemMinX to be used even if Java is installed. If LemMinX extensions (lemminx-maven, etc.) are present, Java is present, and the xml.server.preferBinary preference is enabled, the Java version will be run anyways. If LemMinX extensions are present and no Java is detected, a warning is displayed saying that the extensions can't be used.

The binary server is downloaded when it is needed by the extension, instead of being packaged into the extension. This reduces the size of the extension and total download size for the user. The extension packages the SHA256 hash of the server binaries in order to verify their integrity. The extension falls back to the Java server if the binary server download or integrity check fail.

Additionally, this PR contains modifications to the existing Jenkins pipeline so that the correct download link for the binary server is packaged into the package.json, and the hashes for the binary server are packaged into ./server/*.sha256. There is also a new pipeline file that can be used to build the LemMinX binary under Windows, macOS, and RHEL 8 for the x64_86 architecture.

This PR is a rebase of PR 256 with additional features.

@datho7561
Copy link
Contributor Author

Notes on testing this PR:

  • Set the setting xml.server.binary.enabled to true
  • Build the LemMinX binary using Generate native binary eclipse-lemminx/lemminx#860
  • Move the binary to ./server/lemminx-(darwin/linux/win32) (depending on your os)
  • Create a sha256 hash for the binary and put it under ./server/lemminx-(darwin/linux/win32)-hash
    • (e.g. sha256sum lemminx-linux > ./server/lemminx-linux-hash)

@fbricon
Copy link
Collaborator

fbricon commented Sep 1, 2020

needs to handle download errors. I had the wrong name on the server and absolutely nothing was reported in vscode.

http server reported:
[2020-09-01T16:26:10.073Z] "GET /lemminx-binary/lemminx-darwin" "undefined"
[2020-09-01T16:26:10.080Z] "GET /lemminx-binary/lemminx-darwin" Error (404): "Not found"

the extension created an empty binary under [vscode-xml]/server, so then it didn't retry to download the proper binary the next time

src/javaServerStarter.ts Outdated Show resolved Hide resolved
src/javaServerStarter.ts Outdated Show resolved Hide resolved
src/javaServerStarter.ts Outdated Show resolved Hide resolved
@datho7561 datho7561 force-pushed the native-binary branch 2 times, most recently from 9c1cc6e to 10667a1 Compare September 2, 2020 20:28
@fbricon fbricon mentioned this pull request Sep 8, 2020
@datho7561 datho7561 force-pushed the native-binary branch 2 times, most recently from 8eff7af to 2bd0840 Compare September 11, 2020 20:29
@fbricon
Copy link
Collaborator

fbricon commented Sep 30, 2020

I can't rebase it cleanly against master

src/javaServerStarter.ts Outdated Show resolved Hide resolved
src/javaServerStarter.ts Outdated Show resolved Hide resolved
src/javaServerStarter.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@fbricon
Copy link
Collaborator

fbricon commented Sep 30, 2020

Testing with the server downloaded from https://github.com/datho7561/lemminx/actions/runs/279271890, OSX is not happy about it and will repeatedly show the same error message 5 times in less than 3 min, if you press cancel every time.

Screen Shot 2020-09-30 at 5 17 02 PM

New challenge: how to notarize the app so that macOS shuts the fuck up?

@datho7561 datho7561 force-pushed the native-binary branch 2 times, most recently from da3d998 to 66227fe Compare September 30, 2020 18:15
@datho7561
Copy link
Contributor Author

That's odd. I rebased, but for some reason it still says its in conflict here.

@datho7561 datho7561 force-pushed the native-binary branch 3 times, most recently from 8f8af65 to 9b0c127 Compare October 14, 2020 20:20
Copy link
Contributor Author

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored some of the code I wrote because it was a mess. It should now work without Java. Some other changes I made:

  • It will try to use the binary if no Java is installed
  • It will give a warning if there are LemMinX extensions but no Java installed
  • It will always use the Java server if it there are LemMinX extensions and Java is set up

src/requirements.ts Outdated Show resolved Hide resolved
src/requirements.ts Outdated Show resolved Hide resolved
src/serverStarter.ts Show resolved Hide resolved
src/serverStarter.ts Show resolved Hide resolved
src/binaryServerStarter.ts Outdated Show resolved Hide resolved
src/binaryServerStarter.ts Outdated Show resolved Hide resolved
@datho7561 datho7561 force-pushed the native-binary branch 6 times, most recently from f7f3cc5 to 20a509e Compare January 14, 2021 21:10
@datho7561
Copy link
Contributor Author

I personally don't think we should add a setting to silence this warning. Maybe we could add an entry to the internal documentation that explains why the binary is not trusted, and how to generate the hash file for the binary if its trusted. Then we could link from the warning to the documentation with a button.

@datho7561 datho7561 force-pushed the native-binary branch 7 times, most recently from 6a533ef to 5a6c534 Compare January 15, 2021 18:40
@fbricon
Copy link
Collaborator

fbricon commented Jan 15, 2021

I personally don't think we should add a setting to silence this warning. Maybe we could add an entry to the internal documentation that explains why the binary is not trusted, and how to generate the hash file for the binary if its trusted. Then we could link from the warning to the documentation with a button.

I believe things should be as easy/streamlined as possible, and consistent across Red Hat extensions. vscode-java does store trusted checksums, to avoid nagging users: https://github.com/redhat-developer/vscode-java/blob/15dae347c8b0f3badf0273f3bfc4bb4b1c22935d/src/settings.ts#L209-L240

@datho7561
Copy link
Contributor Author

Okay. I will implement this.

@fbricon
Copy link
Collaborator

fbricon commented Jan 15, 2021

one important part of the vscode-java implem, is the setting is stored at the global level, to avoid malicious actors allowing shas of malicious binaries in the workspace settings.json

@datho7561
Copy link
Contributor Author

Should be added now.

NativeImage.jenkins Outdated Show resolved Hide resolved
NativeImage.jenkins Show resolved Hide resolved
NativeImage.jenkins Outdated Show resolved Hide resolved
NativeImage.jenkins Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@datho7561 datho7561 marked this pull request as ready for review January 18, 2021 15:57
@datho7561
Copy link
Contributor Author

The pipeline will not work until the new parameter GRAALVM_VERSION is set up in the Jenkins job configuration

src/binaryServerStarter.ts Outdated Show resolved Hide resolved
Download a binary lemminx, check its integrity, then run it.

Includes:
 * Setting to specify a binary, `xml.server.binary.path`
 * Setting to specify args for the binary, `xml.server.binary.args`

Defaults to java server in cases such as:
 * The binary can't be downloaded
 * The file containing the expected hash of the binary is missing
 * The hash of the binary doesn't match the expected hash
 * Binary specified in setting can't be located and the above three fail

Signed-off-by: David Thompson <[email protected]>
@fbricon fbricon merged commit 5495a0e into redhat-developer:master Jan 28, 2021
@datho7561 datho7561 deleted the native-binary branch January 28, 2021 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants