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

Add experimental riscv64 builds #1922

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Add experimental riscv64 builds #1922

merged 1 commit into from
Jun 11, 2024

Conversation

kxxt
Copy link
Contributor

@kxxt kxxt commented Jun 10, 2024

With a performance issue(riscv-forks/electron#1) fixed, I think the riscv64 port of electron is mostly ready for VSCodium. There are still some corner cases that lead to segfault but it's already suitable for usage on real riscv64 hardware.

This PR add riscv64 builds to vscodium.

Official electron doesn't support riscv64 so I used a fork maintained by myself.

BTW I noticed some of the patches modifies the compiled javascript files. I think it's cleaner to directly modify the typescript source file and compile ts files to javascrpt.

I haven't implemented riscv64 support in sysroot scripts so this PR only covers a basic riscv64 build, without DEB and RPM targets.

Here's a screenshot:

VSCodium 1.90 running on 64 core SG2042.

The window on the right shows using open-remote-ssh plugin to connect to x86_64 host from riscv64.

Screenshot_20_10_1

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 10, 2024

Ups... I think we were working on more or less the same...

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 10, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

@daiyam
Copy link
Member

daiyam commented Jun 10, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

We should split the ppc64le-and-riscv64-support.patch.

build.sh Outdated Show resolved Hide resolved
patches/remove-mangle.patch Outdated Show resolved Hide resolved
@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 10, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

We should split the ppc64le-and-riscv64-support.patch.

Roger that!

patches/ext-from-gh.patch Outdated Show resolved Hide resolved
@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

We should split the ppc64le-and-riscv64-support.patch.

I think it might be cleaner to keep one patch. If we split the patch, the ppc64 and riscv64 patches both modify the same line in a file, which will lead to a conflict. WDYT?

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

Does the PPC64LE has an electron fork that publishes electron binaries? If so I could add it in this PR?

@kxxt kxxt marked this pull request as draft June 11, 2024 00:22
@kxxt kxxt force-pushed the master branch 2 times, most recently from 9332fa0 to 4398d41 Compare June 11, 2024 00:24
@kxxt kxxt marked this pull request as ready for review June 11, 2024 00:55
@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

Does the PPC64LE has an electron fork that publishes electron binaries? If so I could add it in this PR?

We don't publish the binaries, but we use the ELECTRON_MIRROR variable to set the location for it. The PR that I opened only makes the changes needed to enable the build to happen. Is up to the user to specify where that electron binary comes from.

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

Should we coordinate and have this be a single PR for both RISCV64 and PPC64LE?

Does the PPC64LE has an electron fork that publishes electron binaries? If so I could add it in this PR?

We don't publish the binaries, but we use the ELECTRON_MIRROR variable to set the location for it. The PR that I opened only makes the changes needed to enable the build to happen. Is up to the user to specify where that electron binary comes from.

Oh sorry, I didn't see your PR. I thought you haven't opened it. About the patches, do you think it's more convenient to split riscv64 and ppc64 or keep one patch? I think splitting the patch doesn't actually solve the conflict but makes it more complex because we need to stack a patch on top of another.

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

I think it might be cleaner to keep one patch. If we split the patch, the ppc64 and riscv64 patches both modify the same line in a file, which will lead to a conflict. WDYT?

I kinda agree, I tried splitting the patch and the only way I got it to work was if I based one on top of the other. Also, in the other PR there are some changes to how to get the vscode-sysroot that make it so that we don't need to check for a specific arch.

While developing it I was trying to make it so that porting to RISCV64 was more or less easy, but never tried to build it.

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

think splitting the patch doesn't actually solve the conflict but makes it more complex because we need to stack a patch on top of another.

Agree, took me a couple of hours just to split the changes and always diffing everything to make sure I didn't mess anything up. I kinda feel like this could be a single PR or at least we should both coordinate on how to get them both in.

I do recommend you take the patch to install-sysroot.js, that makes it easier if the tag changes or we want to use another repo.

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

I do recommend you take the patch to install-sysroot.js, that makes it easier if the tag changes or we want to use another repo.

I agree with you. There's no need to maintain two methods of using a custom electron repo.

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

Another way is to decommonize the intermediate vscode artifact, that is, building an intermediate vscode artifact for each architecture. This way we can apply different patches to different architectures. But I don't think it worth the effort for just two architectures that need patching. This method might make more sense if we have patches for four or more architectures that need patching.

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

This method might make more sense if we have patches for four or more architectures that need patching.

As long as no one asks me to build for s390x I'm fine with that.

Let's get started then! I'm in GMT-5, so this is probably the best time to contact me.

I see that you replace the electron.txt checksums. That would collide with my needs, since that checksums file wouldn't have the sha256 for our binary.

package_linux_bin.sh Outdated Show resolved Hide resolved
@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

This method might make more sense if we have patches for four or more architectures that need patching.

As long as no one asks me to build for s390x I'm fine with that.

Let's get started then! I'm in GMT-5, so this is probably the best time to contact me.

I see that you replace the electron.txt checksums. That would collide with my needs, since that checksums file wouldn't have the sha256 for our binary.

It wouldn't collide with your needs. The riscv64 and ppc64le pipelines are ran separately so replacing it in riscv64 shouldn't affect it's usage in ppc64le.

@kxxt kxxt marked this pull request as draft June 11, 2024 14:02
@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

It wouldn't collide with your needs. The riscv64 and ppc64le pipelines are ran separately so replacing it in riscv64 shouldn't affect it's usage in ppc64le.

Is it ok with you if the binaries for the electron binary and it's sha256 change without VSCodium knowing? I kinda feel like that might be a bit risky.

@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

It wouldn't collide with your needs. The riscv64 and ppc64le pipelines are ran separately so replacing it in riscv64 shouldn't affect it's usage in ppc64le.

It will be better if you could just patch the electron.txt

@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

It wouldn't collide with your needs. The riscv64 and ppc64le pipelines are ran separately so replacing it in riscv64 shouldn't affect it's usage in ppc64le.

Is it ok with you if the binaries for the electron binary and it's sha256 change without VSCodium knowing? I kinda feel like that might be a bit risky.

I agree

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

It wouldn't collide with your needs. The riscv64 and ppc64le pipelines are ran separately so replacing it in riscv64 shouldn't affect it's usage in ppc64le.

Is it ok with you if the binaries for the electron binary and it's sha256 change without VSCodium knowing? I kinda feel like that might be a bit risky.

It's indeed a bit risky but I don't have other ways to keep the electron riscv releases compatible with upstream releases, that is, to make tools like app-builder and electron-builder happy. I do publish releases in riscv electron that won't be clobbered to tags like v29.4.2.riscv2 but that's unfortunately incompatible with some electron build tools. Anyway, let's use immutable releases tags for VSCodium.

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

I see that you replace the electron.txt checksums. That would collide with my needs, since that checksums file wouldn't have the sha256 for our binary.

It wouldn't collide with your needs. The riscv64 and ppc64le pipelines are ran separately so replacing it in riscv64 shouldn't affect it's usage in ppc64le.

It will be better if you could just patch the electron.txt

I actually don't replace the checksums but append to it. @daiyam Does it look good to you? I consider it as a method of "patch".

@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

I actually don't replace the checksums but append to it. @daiyam Does it look good to you? I consider it as a method of "patch".

No, I prefer to have version and checksum in clear text in the repo. There are several benefit to do this:

  • every body can look at the code and know which version has been used
  • reproducible binaries
  • the changes of version are tracked

We do the same for VSCode (stable.json)

@kxxt kxxt force-pushed the master branch 2 times, most recently from 957ccec to 284ac93 Compare June 11, 2024 15:11
@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

I don't know if it helps, but I do remember seeing an environment variable to change the electron version, I just don't remember where. I'll look for it.
With that I think you could use ELECTRON_MIRROR and whatever the other variable is and all that would be needed is the patch for electron.txt.

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

I actually don't replace the checksums but append to it. @daiyam Does it look good to you? I consider it as a method of "patch".

No, I prefer to have version and checksum in clear text in the repo. There are several benefit to do this:

* every body can look at the code and know which version has been used

* reproducible binaries

* the changes of version are tracked

We do the same for VSCode (stable.json)

Thanks for clarifying. I have squashed the commits and rebased my PR. Will mark ready for review once the CI passes here: https://github.com/kxxt/vscodium/actions/runs/9468203671

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

Found it! Can even specify a custom file name! Does that help?

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

I don't know if it helps, but I do remember seeing an environment variable to change the electron version, I just don't remember where. I'll look for it. With that I think you could use ELECTRON_MIRROR and whatever the other variable is and all that would be needed is the patch for electron.txt.

Nice finding! But I probably won't have time to try it tonight. I am going to bed soon.

@daiyam I write the clear text electron checksum in package_linux_bin.sh. Do you think I need to create a patch file for it or it's fine to maintain the checksum in package_linux_bin.sh?

@kxxt kxxt marked this pull request as ready for review June 11, 2024 15:32
@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

@daiyam I write the clear text electron checksum in package_linux_bin.sh. Do you think I need to create a patch file for it or it's fine to maintain the checksum in package_linux_bin.sh?

You don't need a patch

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

@daiyam I write the clear text electron checksum in package_linux_bin.sh. Do you think I need to create a patch file for it or it's fine to maintain the checksum in package_linux_bin.sh?

You don't need a patch

Then I think the PR is ready for review now. The CI is green here(for riscv64 and ppc64le): https://github.com/kxxt/vscodium/actions/runs/9468203671/job/26084811222

@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

Found it! Can even specify a custom file name! Does that help?

@lex-ibm VSCode seems to use the package @vscode/gulp-electron to download electron (https://github.com/microsoft/vscode/blob/5adc4dcb025aac567c55100c4f2777614644de89/build/lib/electron.ts#L208)

Edit: After reviewing the code, VSCode do use @vscode/gulp-electron to download electron. And @kxxt is modifying the right properties.

package_linux_bin.sh Outdated Show resolved Hide resolved
@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

LGTM

@kxxt Is it ok for you?

@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

@lex-ibm Do you have any feedback?

@kxxt
Copy link
Contributor Author

kxxt commented Jun 11, 2024

LGTM

@kxxt Is it ok for you?

Yes

@lex-ibm
Copy link
Contributor

lex-ibm commented Jun 11, 2024

LGTM! Awesome job!

@daiyam daiyam merged commit 8c9acad into VSCodium:master Jun 11, 2024
21 checks passed
@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

@kxxt Thanks!

@kxxt
Copy link
Contributor Author

kxxt commented Jun 12, 2024

Thanks both of you for helping me to get this PR merged!

About automatically updating the riscv64 electron version and checksum. I think we can implement it in a script and run it in the *-spearhead workflows.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants