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

Check for glibc version instead of distro version #31

Merged
merged 58 commits into from
Nov 4, 2024

Conversation

wknapik
Copy link
Collaborator

@wknapik wknapik commented Oct 30, 2024

Resolves #5 (enforce system requirements)
Resolves #24 (various improvements)
Resolves #9 (support for macOS)
Adds support for Manjaro

@wknapik wknapik self-assigned this Oct 30, 2024
Makefile Outdated Show resolved Hide resolved
@wknapik wknapik marked this pull request as ready for review October 30, 2024 21:36
@wknapik wknapik requested a review from fmarier as a code owner October 30, 2024 21:36
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
fmarier
fmarier previously approved these changes Oct 31, 2024
Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

This looks good to me, but before merging:

  • Let's have @stoletheminerals review as well.
  • Let's confirm that installing using cask on macOS will not interfere with auto-updates.

install.sh Show resolved Hide resolved
fmarier
fmarier previously approved these changes Nov 1, 2024
install.sh Outdated Show resolved Hide resolved
install.sh Show resolved Hide resolved
@wknapik
Copy link
Collaborator Author

wknapik commented Nov 1, 2024

@fmarier I assume we're still waiting for approval from @stoletheminerals?

@fmarier
Copy link
Member

fmarier commented Nov 1, 2024

@wknapik Yes, are you done making all of the changes you wanted to make?

I'd like Artem to review since he's assigned to the sec review and we're not planning any other changes to the MVP script after this.

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 2, 2024

Yes, are you done making all of the changes you wanted to make?

Yeah, I think so

We'll ask people internally to test before going public and maybe we'll make some adjustments then, in a new PR

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 2, 2024

Actually, pushed one last change to the README

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 2, 2024

And a few more ;]

Copy link

@stoletheminerals stoletheminerals left a comment

Choose a reason for hiding this comment

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

lgtm! since it supports macos installation with brew, do we plan to add it to https://brave.com/download/? It seems macos doesn't have a dedicated landing page

install.sh Show resolved Hide resolved
@wknapik
Copy link
Collaborator Author

wknapik commented Nov 4, 2024

lgtm! since it supports macos installation with brew, do we plan to add it to https://brave.com/download/? It seems macos doesn't have a dedicated landing page

Do we want to promote that method of installation?

With the auto-updater on macOS, just downloading and double-clicking on a .dmg seems simple enough. No need to have users run commands in a terminal. But maybe I'm not seeing some upside?

For someone who has to do it frequently (like members our QA team), the script might end up being more convenient, but for the average user, I'm guessing not so much.

Let's see what feedback we get once this goes live.

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 4, 2024

We could also extend support to windows via WSL, I think

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 4, 2024

@fmarier please approve as code owner and we'll merge

@wknapik wknapik enabled auto-merge (squash) November 4, 2024 15:58
@wknapik wknapik merged commit e350e6d into main Nov 4, 2024
2 checks passed
@wknapik wknapik deleted the wknapik-check-for-glibc-instead-of-distro branch November 4, 2024 18:02
@fmarier
Copy link
Member

fmarier commented Nov 4, 2024

We could also extend support to windows via WSL, I think

It already works on WSL according to @bsclifton who tested an earlier version. WSL is basically just Ubuntu.

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 4, 2024

It already works on WSL according to @bsclifton who tested an earlier version. WSL is basically just Ubuntu.

So that installs the Linux browser, which then runs under WSL, right?
I was thinking of installing the Windows version of the browser via WSL.
I haven't used WSL, so I don't know if that makes sense, but if it does, we could do that.

@fmarier
Copy link
Member

fmarier commented Nov 4, 2024

I see. Yes, right now it would install the Linux version (.deb).

Good idea. Worthy of a new-distro issue in this repo. Could be a nice opportunity for someone in the community to volunteer a PR.

@wknapik
Copy link
Collaborator Author

wknapik commented Nov 4, 2024

Opened #36

@bsclifton
Copy link
Member

It already works on WSL according to @bsclifton who tested an earlier version. WSL is basically just Ubuntu.

So that installs the Linux browser, which then runs under WSL, right? I was thinking of installing the Windows version of the browser via WSL. I haven't used WSL, so I don't know if that makes sense, but if it does, we could do that.

Yup- this installs the Linux version. When you open it, it'll open the app in a new window but it's the Linux version.

I'm not sure how you can interface with Windows from WSL; they have different file systems. There might be a compatibility layer, but I doubt you can just do an install (to Windows from WSL). I'd love to be wrong though

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.

Potential improvements deviating from upstream Add support for macOS Enforce our system requirements
5 participants