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 for MSYS2 #3

Merged
merged 4 commits into from
Oct 4, 2021
Merged

Fix for MSYS2 #3

merged 4 commits into from
Oct 4, 2021

Conversation

rashil2000
Copy link
Contributor

MSYS2 uses pacman. This includes a tiny fix for it.

Also closes #2

Tested on Arch Linux and MSYS2

@@ -8,7 +8,7 @@ if [[ -z $PARUZ ]]; then
elif ! command -v pacman >/dev/null 2>&1; then
echo "Neither paru nor pacman found. Is this Arch?" >&2
exit 1
elif [[ $EUID -eq 0 ]]; then
elif [[ $EUID -eq 0 ]] || [[ -f /usr/bin/msys-2.0.dll ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to special case msys2. It's fixed by your next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no sudo on MSYS2. So we need to use just 'pacman'.

Copy link
Owner

Choose a reason for hiding this comment

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

Are you able to set environment variables? If so, try setting PARUZ=pacman

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm a bit confused. Isn't PARUZ being set to pacman here in the next line itself? I can set environment variables, but I'm unable to understand the need for it. (The Readme tells us that we explicitly set PARUZ when we want to use a different helper, but this is just pacman.)

Copy link
Owner

Choose a reason for hiding this comment

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

I'm trying to fix your problem without a special case for MSYS2. If you export PARUZ=pacman in your shell, then it won't do the root check or try sudo pacman.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm again trying too understand what's the harm in having a special case (genuine question). If it's an implementation problem, see my comment below.

Copy link
Owner

Choose a reason for hiding this comment

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

Does the environment variable work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does.
But I find having to define PARUZ as 'pacman' slightly cumbersome, more so because there's already a fallback to 'pacman' in the script itself.
There's already a condition running in the if block there. Would it be considered really bad if we just put another OR check there? (Sorry again, I'm just trying to understand. I get that PRVW_CMD was slightly invasive as a platform-specific fix, so I removed it as per your instructions. But isn't this one really insignificant in comparison?)

Copy link
Owner

Choose a reason for hiding this comment

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

Adding an environment variable shouldn't be cumbersome. I have like 100 environment variables in my shell config. This scenario is exactly why I added the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paruz was made to handle paru and pacman (as I understand from the script). I would have happily set the environment variable if I was using some other helper (as the Readme tells us).

I will remove this condition if this feels really unnecessary.

paruz Outdated Show resolved Hide resolved
@rashil2000 rashil2000 changed the title Fix for Arch Linux and MSYS2 Fix for MSYS2 Oct 4, 2021
@joehillen joehillen merged commit f3bf989 into joehillen:master Oct 4, 2021
@rashil2000 rashil2000 deleted the msys-fix branch October 5, 2021 02:33
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.

Doesn't work with pacman
2 participants