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

Changes needed to enable PPC64LE UI build #1923

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

lex-ibm
Copy link
Contributor

@lex-ibm lex-ibm commented Jun 10, 2024

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

  • VSCODE_SYSROOT_REPO: Github repository where vscode's sysroot is located
  • VSCODE_SYSROOT_VERSION: Tag in that repo that contains the sysroot tar.gz
  • GH_HOST: Github host (default to public Github)
  • Variations of GITHUB_TOKEN

@lex-ibm
Copy link
Contributor Author

lex-ibm commented Jun 10, 2024

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
Copy link
Member

daiyam commented Jun 10, 2024

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
Copy link
Contributor Author

lex-ibm commented Jun 10, 2024

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.

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

daiyam commented Jun 10, 2024

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

+const FAIL_BUILD_FOR_NEW_DEPENDENCIES = false;

@daiyam
Copy link
Member

daiyam commented Jun 10, 2024

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;

  • "${APP_NAME_LC}-linux-ppc64le-${RELEASE_VERSION}.tar.gz"
  • "${APP_NAME_LC}-linux-ppc64le-${RELEASE_VERSION}.rpm"
  • "${APP_NAME_LC}-linux-ppc64le-${RELEASE_VERSION}.deb"

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

@daiyam
Copy link
Member

daiyam commented Jun 10, 2024

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
Copy link
Contributor Author

lex-ibm commented Jun 10, 2024

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
Copy link
Member

daiyam commented Jun 10, 2024

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
Copy link
Contributor Author

lex-ibm commented Jun 10, 2024

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
Copy link
Member

daiyam commented Jun 10, 2024

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
Copy link
Contributor Author

lex-ibm commented Jun 10, 2024

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
Copy link
Contributor Author

lex-ibm commented Jun 11, 2024

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
Copy link
Member

daiyam commented Jun 11, 2024

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
Copy link
Member

daiyam commented Jun 11, 2024

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

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

lex-ibm commented Jun 11, 2024

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

@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

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)

@lex-ibm lex-ibm marked this pull request as ready for review June 11, 2024 14:47
@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

LGTM

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

@lex-ibm
Copy link
Contributor Author

lex-ibm commented Jun 11, 2024

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
Copy link
Contributor Author

lex-ibm commented Jun 11, 2024

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

@daiyam daiyam merged commit ffd7e63 into VSCodium:master Jun 11, 2024
36 checks passed
@daiyam
Copy link
Member

daiyam commented Jun 11, 2024

Done!

@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.

2 participants