VSCodium / vscodium

binary releases of VS Code without MS branding/telemetry/licensing
https://vscodium.com
MIT License
24.27k stars 1.02k forks source link

Add experimental riscv64 builds #1922

Closed kxxt closed 3 weeks ago

kxxt commented 3 weeks ago

With a performance issue(https://github.com/riscv-forks/electron/issues/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 commented 3 weeks ago

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

lex-ibm commented 3 weeks ago

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

daiyam commented 3 weeks ago

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

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

lex-ibm commented 3 weeks ago

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!

kxxt commented 3 weeks ago

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 commented 3 weeks ago

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?

lex-ibm commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.

kxxt commented 3 weeks ago

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.

lex-ibm commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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:

We do the same for VSCode (stable.json)

lex-ibm commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

kxxt commented 3 weeks ago

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?

daiyam commented 3 weeks ago

@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 commented 3 weeks ago

@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 commented 3 weeks ago

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.

daiyam commented 3 weeks ago

LGTM

@kxxt Is it ok for you?

daiyam commented 3 weeks ago

@lex-ibm Do you have any feedback?

kxxt commented 3 weeks ago

LGTM

@kxxt Is it ok for you?

Yes

lex-ibm commented 3 weeks ago

LGTM! Awesome job!

daiyam commented 3 weeks ago

@kxxt Thanks!

kxxt commented 3 weeks ago

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.