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

Changes needed to enable PPC64LE UI build #1923

Closed lex-ibm closed 3 weeks ago

lex-ibm commented 3 weeks ago

This PR makes the necessary changes to enable VSCodium to build the UI for PPC64LE given a repository for the binaries of electron (using ELECTRON_MIRROR).

It also changes scripts to allow for the use of this repo in Github Enterprise.

It, however, does not enable the pipeline for the build of VSCodium UI in PPC64LE.

Environment Variables Introduced

lex-ibm commented 3 weeks ago

I'm also thinking it might be a good idea to add a way to specify a different checksums file when using a custom electron mirror or a custom vscode sysroot, to avoid having to patch the checksums files.

daiyam commented 3 weeks ago

First of, the GitHub Enterprise build support should be moved to another PR.

Can you explain why you need export SHOULD_BUILD_ZIP="${SHOULD_BUILD_ZIP:-no}"? If you have an external check, shouldn't check_tags.sh just be skipped? It might be also another PR.

I'm also thinking it might be a good idea to add a way to specify a different checksums file when using a custom electron mirror or a custom vscode sysroot, to avoid having to patch the checksums files.

Which checksums files?

Also, is it ppc64le or ppc64el?

lex-ibm commented 3 weeks ago

I thought the same on the Github Enterprise support, but I was working on both at the same time 😅 Will separate that into it's own PR.

Regarding export SHOULD_BUILD_ZIP="${SHOULD_BUILD_ZIP:-no}", I found a bug when trying to build the UI. The section where PPC64LE sets those flags to no would prevent other architectures from building (maybe this only happens on our build environment?). This makes it so that if any other architecture, or even the user, sets those flags, they are not overwritten.

This PR here doesn't include it, but it is also required to patch build/checksums/electron.txt with the sha256 of the electron binary, here is an example of the patch needed:

diff --git a/build/checksums/electron.txt b/build/checksums/electron.txt
index 86f78d0..417ed07 100644
--- a/build/checksums/electron.txt
+++ b/build/checksums/electron.txt
@@ -26,6 +26,7 @@ fb90b8c903407ae575f9c8f727376519c0b35ed6f01dec55b177285b5db864e3 *electron-v28.2
 773aa1f0bbe2b79765bf498958565f63957f8ec2e42327978a143dcbbc7f1bea *electron-v28.2.8-linux-x64-debug.zip
 f8cbc6f2b719cc2f623afcfde8cb1d42614708793621a7a97b328015366b9b8f *electron-v28.2.8-linux-x64-symbols.zip
 e7d17ee311299dfef3d2916987a513c4c1b66ad2e417c15fa5d29699602bd6cb *electron-v28.2.8-linux-x64.zip
+a6b411825ee6cf5eba183d45427cf8e12ec0c733ac2792c6660fca770810812c *electron-v28.2.8-linux-ppc64le.zip
 5f0179fd7bf3927381bde24c9fb372fe95328be0500918cd6ee7f9503fae1ef5 *electron-v28.2.8-mas-arm64-dsym-snapshot.zip
 e9810019f1d7b1b5a93fd1aee8adda5a872ebfb170de6d55cdd55162b923432d *electron-v28.2.8-mas-arm64-dsym.zip
 4781376244c7df89d119575e2788ad43fae4387d850ef672665688081b30997c *electron-v28.2.8-mas-arm64-symbols.zip

I'm thinking that file might change more often that the sha256 for our electron build, so removing the need to patch it and instead use the checksums from the remote repo or adding the option to disable that check might be worth investigating. The same goes for the vscode sysroot.

Also, is it ppc64le or ppc64el?

That depends, in Debian it is ppc64el, in Enterprise Linux it's ppc64le.

daiyam commented 3 weeks ago

You don't need to patch the dep-lists since they are ignored by

https://github.com/VSCodium/vscodium/blob/6a38a6e9db4afdb987f1bc688940965735432653/patches/fix-build-linux.patch#L28

daiyam commented 3 weeks ago

Regarding export SHOULD_BUILD_ZIP="${SHOULD_BUILD_ZIP:-no}", I found a bug when trying to build the UI. The section where PPC64LE sets those flags to no would prevent other architectures from building (maybe this only happens on our build environment?). This makes it so that if any other architecture, or even the user, sets those flags, they are not overwritten.

You don't do one architecture per run?

Also you will need to add the tests for;

So that the SHOULD_BUILD_DEB="no" SHOULD_BUILD_RPM="no" SHOULD_BUILD_TAR="no" can be removed.

daiyam commented 3 weeks ago

I'm thinking that file might change more often that the sha256 for our electron build, so removing the need to patch it and instead use the checksums from the remote repo or adding the option to disable that check might be worth investigating.

It's fine. It would allow me to catch when the versions become out of sync. No need to do extra stuff.

lex-ibm commented 3 weeks ago

You don't do one architecture per run?

I do not, I only ran the check stage once. I'll move those changes to a different PR.

daiyam commented 3 weeks ago

That depends, in Debian it is ppc64el, in Enterprise Linux it's ppc64le.

Won't there be any issue with the remote-ssh with a client ppc64el to a server in ppc64le?

lex-ibm commented 3 weeks ago

Won't there be any issue with the remote-ssh with a client ppc64el to a server in ppc64le?

Tbh, I had not thought of that... I assumed uname reported ppc64le.

daiyam commented 3 weeks ago

I do not, I only ran the check stage once. I'll move those changes to a different PR.

Ya, the script is not thought for that case.

lex-ibm commented 3 weeks ago

I've separated the patches for PPC64LE and the ones for RISCV64, but I had to base riscv64-support.patch on top of the ppc64le-support.patch to avoid conflicts.

lex-ibm commented 3 weeks ago

I just remembered! (internal CI build failed)

You don't need to patch the dep-lists since they are ignored by

We need the object to exist, otherwise it fails the comparison.

And about the electron binary. It took around 3-4hrs to build on a P9, if IBM published an electron binary, would VSCodium be fine with using it?

daiyam commented 3 weeks ago

You don't need to patch the dep-lists since they are ignored by

We need the object to exist, otherwise it fails the comparison.

OK

And about the electron binary. It took around 3-4hrs to build on a P9, if IBM published an electron binary, would VSCodium be fine with using it?

Yes

daiyam commented 3 weeks ago

@lex-ibm This PR seems to be almost only about the split between ppc64 and riscv. Is it done for you?

lex-ibm commented 3 weeks ago

@daiyam I don't really know how to feel about the split, it does make things harder to maintain.

daiyam commented 3 weeks ago

One way to avoid conflict is to reduce the number of extra lines in the patches (from 3 to 1) with git diff --staged -U1 (https://www.git-scm.com/docs/git-diff#Documentation/git-diff.txt--Ultngt)

daiyam commented 3 weeks ago

LGTM

I'm ok as it is. Just tell me the go and I will merge.

lex-ibm commented 3 weeks ago

One way to avoid conflict is to reduce the number of extra lines in the patches (from 3 to 1) with git diff --staged -U1 (https://www.git-scm.com/docs/git-diff#Documentation/git-diff.txt--Ultngt)

There are still cases where they both need to change the same line.

If required, I'll separate the patches, but for now I'll go with this. This I know builds.

lex-ibm commented 3 weeks ago

I'm not making any more changes to this PR then, just waiting for CI. Please merge whenever you are ready 😄 🚀

daiyam commented 3 weeks ago

Done!