-
Notifications
You must be signed in to change notification settings - Fork 78
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 sudo #704
base: master
Are you sure you want to change the base?
Fix sudo #704
Conversation
Since the prompt is added to stdout, remove the one-space-prompt that currently corrupts stdout when a command is ran with sudo.
Check that no element in the chain adds any unwanted content to stdout or stderr when running a command. This is especially important as PAM modules can just write arbitrary messages to stdout when using sudo, such as password expiry notification. There unfortunately seems to be no way of silencing it, but we can at least catch it before it triggers errors down the line.
It seems that empty prompt used to not work at the time of Ubuntu 18.04: I'll try to find what version is the cutoff date |
It works on Ubuntu 20.04 at least, so the cutoff date is earlier than that. EDIT: tested on docker with Ubuntu 14.04 and it works as well. So either there was a bug in a specific version of sudo, or the issue manifested itself in another distro EDIT 2: The original @marcbonnici Do you have access to a WSL install to see if that is still a problem nowadays ? There seems to be a WSL 2 environment that uses a real linux kernel so hopefully it would just ship a regular sudo somehow |
Just checked and I can confirm on my WSL 1 environment the bug is still present, however it does appear to be fixed when switching to WSL2. Just to try and understand better, why was the additional space added causing a breakage here? Could the check to determine whether there are additional messages printed not ignore the single additional space? However worst case, if we wanted to try and maintain compatibility with WS1 maybe we could default to the no prompt option and if this fails, try the spaced version before bailing? |
Thanks for checking, that's partly good news. Apparently WSL 1 is not dead though ...
I discovered this when modifying
Honestly I don't think it's a good idea considering the above. The only other alternative I see is to postprocess the output to remove the prompt in case sudo was used (and precisely sudo, not just as_root=True), but if no prompt works that's even better. |
Thanks for the information.
I see, I'm not sure if there are still any requirements to use WSL1 over WSL2, I do vaguely recall there being some issues with WSL2 but these could have been fixed by now.
Makes sense, I can't think of another option to maintain the compatibility other than post processing either. The only other options would be to require the use of WSL2 but that also wouldn't be ideal if WSL1 is being kept around. |
Do we actually care about WSL at all ? If not, it could make sense to just use |
That's a fair comment, I guess the real world scenarios where the target platform is required to be WSL(1) is likely very limited. Just for the background, the issue was originally found when running the devlib test suite from WSL (which creates a localtarget) as opposed to an actual use case. As you say, perhaps just documenting this issue might be the cleaner option. |
@marcbonnici Is there a specific place you'd see to document that ? I don't think there is any list of officially supported targets in devlib ATM |
True... Looks like we don't really have a good place for this in the current documentation. Perhaps we might need to make do with some comments in the code next to the default prompt and a summary of this discussion in the commit message so at least if we need to revisit this in the future there is a simple way to determine a potential issue and what a solution could be? |
Fixes #703
@marcbonnici Can you remember why we used a one-space prompt with
sudo -p ' '
instead of a completely empty prompt ? This prompt seems to be picked up in stdout or stderr when using Target.execute(), modifying the return value as a result.