Skip to content

Conversation

@notwithering
Copy link
Contributor

this will use pkexec for privilege escalation over sudo, it will still fall back to sudo if pkexec fails (like it not being installed). this should provide a better experience for the user if running through a application runner gui

image

@brkzlr
Copy link
Collaborator

brkzlr commented May 13, 2025

You haven't changed anything for the AppImage which is arguably the only place we actually need this change for as the dev builds are intended for contributors only (and you would use the command line anyway for logging purposes).

You will need to build a correct AppImage runner in the ci/package.sh file so it will also prefer pkexec before sudo.

Another thing to note is the reason we didn't implement this trivial change yet is because we'd like to implement a logger at runtime because if we do a GUI authentication route, users will no longer have a command line open so when they encounter issues, they will have to keep reproducing the issue in a terminal instance to give us the relevant logs.

This can be iffy if it's a low repro bug so we'd want to keep this QoL for us devs by moving the usual terminal output to a logging file if we're to go with polkit.

@notwithering
Copy link
Contributor Author

You haven't changed anything for the AppImage which is arguably the only place we actually need this change for as the dev builds are intended for contributors only (and you would use the command line anyway for logging purposes).

You will need to build a correct AppImage runner in the ci/package.sh file so it will also prefer pkexec before sudo.

oh alright, the aur package that i use uses PINCE.sh so i didnt know, ill see what i can change

Another thing to note is the reason we didn't implement this trivial change yet is because we'd like to implement a logger at runtime because if we do a GUI authentication route, users will no longer have a command line open so when they encounter issues, they will have to keep reproducing the issue in a terminal instance to give us the relevant logs.

This can be iffy if it's a low repro bug so we'd want to keep this QoL for us devs by moving the usual terminal output to a logging file if we're to go with polkit.

yeah that was another thing i was thinking after i made the pr, a proper logging system would be good so the "scary terminal!" wouldnt have to be visible to the user

@brkzlr
Copy link
Collaborator

brkzlr commented May 13, 2025

AFAIK the AUR package modifies PINCE.sh in its own way so it can run. I'm not entirely sure as we don't touch nor maintain the AUR package, they're on their own, but it's worth checking out yourself as the changes you made might be useless without a change in the AUR package.

@korcankaraokcu Shall we require a logging system before we approve this or go ahead and approve once the requested changes are done but deal with some missing logs until a quick dirty logging system can be implemented?

@notwithering
Copy link
Contributor Author

also im unsure what to change in the appimage, should i change echo "Please run this AppImage using 'sudo -E'!" to auto pkexec or sudo itself?

@brkzlr
Copy link
Collaborator

brkzlr commented May 13, 2025

I don't know how pkexec is working as I haven't checked it out yet, but what you want to do is check if the user is running in a privileged state already, if not then you should try to use pkexec to run the PINCE AppImage in an elevated state.
If it fails, then the usual error message should appear.

IIRC I didn't embed sudo in the AppImage due to some issues which is why I opted for the error message to tell users to run the AppImage itself as sudo. I don't know if the same issues will happen with pkexec so you will have to try yourself.

EDIT: Just to confirm, you only have to edit the AppRun.sh creation code, at line 178. You don't need to touch anything else in that package.sh file.

@notwithering
Copy link
Contributor Author

AFAIK the AUR package modifies PINCE.sh in its own way so it can run. I'm not entirely sure as we don't touch nor maintain the AUR package, they're on their own, but it's worth checking out yourself as the changes you made might be useless without a change in the AUR package.

just checked the PKGBUILD and it uses the PINCE.sh from the repo but removes the .venv/PINCE check so it should work just fine

prepare() {
	# Remove ".venv/PINCE" exist check
	sed -e '/^if \[ ! -d .*.venv.* \]; /,/venv.*activate$/ s/^/# /' \
		-e 's|[^ ]*python3|python3|' \
		-i "./$pkgname/PINCE.sh"
}

I don't know how pkexec is working as I haven't checked it out yet, but what you want to do is check if the user is running in a privileged state already, if not then you should try to use pkexec to run the PINCE AppImage in an elevated state. If it fails, then the usual error message should appear.

IIRC I didn't embed sudo in the AppImage due to some issues which is why I opted for the error message to tell users to run the AppImage itself as sudo. I don't know if the same issues will happen with pkexec so you will have to try yourself.

alright thanks, ill take a look

@notwithering
Copy link
Contributor Author

@brkzlr having major issues trying to use the package.sh, errors here there and everywhere. i dont think im going to be able to properly test it so what i wanted to try was sudo -E "$0" "$@" then pkexec {ENV_VARS stuff} "$0" "$@" if you wish to try. ill continue fighting this thing to see if i can get it to work

@brkzlr
Copy link
Collaborator

brkzlr commented May 14, 2025

If you're in Arch, you have to add export NO_STRIP=1 right before deploytool is called otherwise it will fail during packaging. Add it above line 195.

@notwithering
Copy link
Contributor Author

notwithering commented May 14, 2025

If you're in Arch, you have to add export NO_STRIP=1 right before deploytool is called otherwise it will fail during packaging. Add it above line 195.

no, using a linux mint virtual machine with apt. didnt really want to change my install much, getting file permission errors with lrelease and something about GMP and MPFR (missing deps?)

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.

2 participants