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

feat: support arm64 architecture #18

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sannidhyaroy
Copy link
Contributor

Changes

  • Verify system architecture before installation

@sannidhyaroy
Copy link
Contributor Author

@eduardoleolim Could you check if this script properly installs the aarch64 version of Zen on a ARM machine?

bash <(curl -s https://raw.githubusercontent.com/sannidhyaroy/updates-server/refs/heads/feat/arch/install.sh)

The PR should be ready, if it's working as expected...

@eduardoleolim
Copy link

eduardoleolim commented Feb 17, 2025

It works

Captura desde 2025-02-17 08-20-27

Captura desde 2025-02-17 08-20-52

@sannidhyaroy sannidhyaroy marked this pull request as ready for review February 18, 2025 13:07
@eduardoleolim
Copy link

eduardoleolim commented Feb 18, 2025

@sannidhyaroy , I noticed an error. If there is a case where the output of uname -m is arm64, so the variable os_arch must be updated with the value aarch64 to build the download url correctly.

I think the download url must be built after switch statement

@sannidhyaroy
Copy link
Contributor Author

@sannidhyaroy , I noticed an error. If there is a case where the output of uname -m is arm64, so the variable os_arch must be updated with the value aarch64 to build the download url correctly.

I think the download url must be built after switch statement

Right, forgot about that completely. Thanks, I'll push a fix soon!

@sannidhyaroy
Copy link
Contributor Author

sannidhyaroy commented Feb 19, 2025

@sannidhyaroy , I noticed an error. If there is a case where the output of uname -m is arm64

That should work now.

@eduardoleolim
Copy link

eduardoleolim commented Feb 19, 2025

It works but I think it could be better

set -euo pipefail

os_arch=$(uname -m)
... Variables initialized here
# Delete initialization of official_package_location in line 10
... Other variables initialized here

...

case "$os_arch" in
    x86_64) echo "64-bit (Intel/AMD) architecture identified!" ;;
    aarch64|arm64)
		echo "64-bit ARM architecture identified!"
		os_arch="aarch64" # Update os_arch to ensure valid value for download url. Another option could be extract arm64 to another case of switch statement
    *)
		echo "Zen doesn't support this architecture: $os_arch"
		exit 1 ;;
esac

# Make the initialization of official_package_location after define the architecture correctly
official_package_location="https://github.com/zen-browser/desktop/releases/latest/download/zen.linux-$os_arch.tar.xz"

echo "Downloading the latest package"
curl -L --progress-bar -o $tar_location $official_package_location

...

Three changes was made:

  1. Delete initialization of official_package_location in line 10
  2. Update os_arch in switch statement
  3. Initialize official_package_location after switch statement

I think this make the script a little bit smaller and better. I add the full script.

zen.txt

@sannidhyaroy
Copy link
Contributor Author

That's what I originally did. But then scraped it off, thinking that all variables being at the top makes it easier for others to make changes, if required, since you wouldn't need to check through the entire script to find the variable declaration.

I can change the code, but what do you think about this?

@eduardoleolim
Copy link

eduardoleolim commented Feb 19, 2025

I made another alternative:

  1. I added an if statement next to os_arch initialization to update it from arm64 to aarch64. I consider the if statement as part of variables initialization.
  2. I removed the case arm64 from switch statement.
  3. The initialization of official_package_location keeps in line 10 (now 13).

zen.txt

Copy link
Contributor Author

@sannidhyaroy sannidhyaroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the pros of the above mentioned reasons that you specified, while handling the cons that I mentioned to some extent.

#!/bin/bash

set -euo pipefail

# Variables
...
official_package_location="" # Placeholder for download URL, to be set later
...

# Handle architecture detection and set the correct download URL
case "$os_arch" in
    x86_64) 
        echo "64-bit (Intel/AMD) architecture identified!"
        ;;
    aarch64|arm64) 
        echo "64-bit ARM architecture identified!"
        os_arch="aarch64"
        ;;
    *)
        echo "Zen does not support this architecture: $os_arch"
        exit 1
        ;;
esac

# Set the official package download URL after architecture is set
official_package_location="https://github.com/zen-browser/desktop/releases/latest/download/zen.linux-$os_arch.tar.xz"

# Rest of the script
...

I think this is the midway between both the solutions. What do you think?

@eduardoleolim
Copy link

I like it

@sannidhyaroy
Copy link
Contributor Author

I like it

Great! We'll proceed with that approach then...

The script will fail on non-linux systems to avoid accidental execution on unsupported platforms
@eduardoleolim
Copy link

@mauro-balades do you see any bug?

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

Successfully merging this pull request may close these issues.

3 participants