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

Update askpass.py user interface #63

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

Conversation

wjk
Copy link

@wjk wjk commented May 19, 2021

This PR does two things:

  • Update all copies of askpass.py to the version from the version in the ISO repo, albeit with one change: a colon is now present after “Password”.
  • The package downloader stub scripts now set SUDO_ASKPASS_TEXT.

@wjk wjk force-pushed the update-askpass branch from 1ba5e10 to e7c4b3c Compare May 19, 2021 23:37
@probonopd
Copy link
Member

probonopd commented May 21, 2021

Thanks @wjk.

Since helloSystem now has /usr/local/bin/askpass, I was thinking of retiring all other copies.
What do you think?

Also, let's avoid untranslated English-only text wherever possible, pending #46 (comment).

Until we have found an easy way to manage translations for PyQt applications, I think "Password: _____" is better than "(Long English-only text): _____"-

@wjk
Copy link
Author

wjk commented May 21, 2021

I created multiple copies of àskpass.py because I was trying to follow the AppImage philosophy of bundling all implementation-detail dependencies. I’ll leave it to your judgment if you want me to remove them and use the system copy. As for the dialog text, please see #53 (comment); we decided that the calling process is responsible for localizing the SUDO_ASKPASS_TEXT string.

I would also like to invite your attention to the babel package. Among other useful tools, this package includes a pure-Python *.po file parser. It parses plain-text po files into the same type of data structure that would be output by loading a compiled *.mo binary file. I could wrap this library into an API resembling the gettext standard library module, and distribute the *.po files within the application bundles. Sound good to you?

@probonopd
Copy link
Member

I could wrap this library into an API resembling the gettext standard library module, and distribute the *.po files within the application bundles. Sound good to you?

If this works with halfway reasonable code size and runtime performance overhead, then this would rock 👍 So I would highly appreciate a proof-of-concept.

AppImage philosophy of bundling all implementation-detail dependencies

"... that are not part of the base system". So this one is a close call, and I am still a bit undecided myself. It depends on whether we think that anyone outside of helloSystem will want to use those applications.

@wjk
Copy link
Author

wjk commented May 21, 2021

that are not part of the base system

According to the “Note to distributors” page in your documentation, these apps are not intended to be used outside of helloSystem as-is. Therefore, the askpass script can indeed be assumed to be part of the base system. I will therefore remove the local copies. I will submit my gettext POC in another branch. Thanks!

@probonopd
Copy link
Member

probonopd commented May 22, 2021

Agree that we should treat askpass as being part of the base system. However I think we should not hardcode the path but rely on SUDO_ASKPASS being supplied by the system in this case. (I have an experimental helloDesktop-on-Linux system for research purposes where it points to a different absolute path, for example.)

@wjk
Copy link
Author

wjk commented May 22, 2021

@probonopd I can certainly inherit the value for SUDO_ASKPASS from the parent process’s environment, but then what do we do if it is unset? In that case, sudo would prompt for a password on its stdin and block forever since nothing is connected to it.

@probonopd
Copy link
Member

We could

  • Search for askpass on the $PATH if SUDO_ASKPASS is unset
  • Show an error dialog saying that $SUDO_ASKPASS must be set

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