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

Fix incorrect argon2 target in arm builds #6453

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Fix incorrect argon2 target in arm builds #6453

merged 2 commits into from
Sep 28, 2023

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Sep 27, 2023

I removed building from source because the comment said it was due to CentOS 7, which we no longer use.

However, it seems that the real issue is that argon2 is pulling the wrong binary for cross-builds? So we need to force building from source instead. This should fix #6446 and fix #6448.

Unfortunately building from source is causing the new Kerberos module to fail on arm64. To fix that we will need to install the arm64 version of the library.

Additionally, building from source is causing keytar to fail (a longstanding issue that previously was accidentally ignored when it failed in CI). It seems unique to GitHub's VM; I cannot reproduce it in a Docker container. So, switch to a Docker container.

@code-asher code-asher requested a review from a team as a code owner September 27, 2023 16:35
@code-asher code-asher force-pushed the arm-again branch 15 times, most recently from 979a9f6 to b327b2d Compare September 28, 2023 00:14
@code-asher code-asher changed the title Add arm64 source in addition to armhf Fix incorrect argon2 target in arm builds Sep 28, 2023
Not building from source causes argon2 to pull the wrong arch, so we
have to build from source.

But building from source is causing the new Kerberos module to fail on
arm64 and keytar to fail on both.

The latter has been very difficult to debug because the GitHub image
provides a different result to containers based on Ubuntu 20.04.
Because of this, use a container instead.

Use debian:buster as the container because it is easier to set up the
architecture sources (no need to modify the sources) and because it
seems to come with glibc 2.28 rather than 2.31.

Also use the exact version of Node (18.15.0) for reproducibility.
Otherwise you get IDs that can cause (benign) errors while extracting,
which might be confusing.  At the very least, I did not see these errors
from previous tars (although they seem to use 1001).

There is no guarantee what IDs might exist so 0 seems the most
reasonable.
@code-asher code-asher enabled auto-merge (squash) September 28, 2023 00:35
@code-asher code-asher disabled auto-merge September 28, 2023 03:17
@code-asher code-asher merged commit 6275520 into main Sep 28, 2023
11 checks passed
@code-asher code-asher deleted the arm-again branch September 28, 2023 03:17
@Lan-Hekary
Copy link
Contributor

Hello @code-asher ,
can you make a fix build with this commit ?

@code-asher
Copy link
Member Author

I just published an RC with this commit. If you get a chance to try it please let me know if it fixes the issue!

@Lan-Hekary
Copy link
Contributor

I updated it via curl -fsSL https://code-server.dev/install.sh | sh -s -- --version 4.17.1-rc.1
and restarted the service, It works now as intended.
Thank you

@code-asher
Copy link
Member Author

Awesome thank you for the confirmation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants