Closed zhubonan closed 3 years ago
thanks @zhubonan for the report!
I would propose:
try_sudo
should become a parameter of the constructor
https://github.com/aiidateam/pgsu/blob/c75a1f4f2eaa2297ea47106b55a3cf8f97ab37fd/pgsu/__init__.py#L67
DEFAULT
(if there is a way to identify WSL from this?)Sounds good?
I'd be happy to review and accept a PR on this - otherwise it might take me a bit longer.
It would be even better if there was a way of testing WSL on github actions, but perhaps that's not easily possible... can you check?
For detecting that we're on WSL, checking for "microsoft" (any capitalization) in the release
attribute of uname
(and, probably, also checking that we're on Linux) seems like a reasonable (although not bulletproof) way:
In [6]: from platform import uname
In [7]: uname()
Out[7]: uname_result(system='Linux', node='A-DOGRES-082219', release='4.19.128-microsoft-standard', version='#1 SMP Tue Jun 23 12:58:10 UTC 2020', machine='x86_64', processor='x86_64')
Some discussions: https://github.com/microsoft/WSL/issues/423 https://github.com/microsoft/WSL/issues/4071 Found via this page: https://www.scivision.dev/python-detect-wsl/
If it's just about detecting Ubuntu, platform.dist()
seems to work also on WSL:
In [7]: platform.dist()
Out[7]: ('Ubuntu', '18.04', 'bionic')
What's the reason for trying sudo only on Ubuntu, by the way? How's the behavior different in other Linuxes?
Thanks a lot for the helpful info @greschd !
What's the reason for trying sudo only on Ubuntu, by the way? How's the behavior different in other Linuxes?
No good reason other than that I did not know the default postgres setup on other linux distros and did not want to scare users with trying sudo
unnecessarily.
Happy to add any other distros, where the setup is known to be the same (postgres
system user, with paswordless psql
into becoming postgres superuser), or different (let me know the details).
Ah yeah, that makes sense -- the default user etc. can definitely vary between distros.
My naive guess would be that most Ubuntu-based (or maybe even Debian-based) distros would work the same, but it probably makes sense to test before adding them.
In that case, probably just replacing 'Ubuntu' in platform.version()
with 'Ubuntu' in platform.dist()[0]
should do the trick.
Thanks for looking into this, for me the platform.dist()
give this:
('debian', 'buster/sid', '')
so no mentioning of Ubuntu
. The same goes for both WSL1 and WSL2. I am on 18.04 though, it may very different versions? @greschd
The other complication is WSL is not always Ubuntu, other Linux distributions can be run as well like openSUSU....
Perhaps the check can be simply whether this is a sudo
in the path and if the system is Linux?
>>> from subprocess import check_output
>>> check_output(['which', 'sudo'])
b'/usr/bin/sudo\n'
>>>
platform.dist()
Maybe it depends on python versions, mine on 3.7 says the function is deprecated since 3.5 and it is gone in 3.8.
>>> platform.dist()
__main__:1: DeprecationWarning: dist() and linux_distribution() functions are deprecated in Python 3.5
('debian', 'buster/sid', '')
@zhubonan hmm, I get the same behavior when using the conda
Python. Seems like it depends on how Python was compiled.. maybe the command was deprecated because it ended up being unreliable.
There seem to be a few different ways of determining the distro, not sure which is the most commonly available: https://linuxhandbook.com/check-linux-version/
So, in general, I think this is less of a WSL problem and more of a Linux distro problem. As you mentioned, different distros or even non-Linux OSes can run on WSL; our problem here comes mostly from that. I think we should overall rely more on how the system is set up, and less on hard-coding "known good" distro names.
I think we should overall rely more on how the system is set up, and less on hard-coding "known good" distro names.
Open to suggestions on how to do this
@greschd Ahh I see, yes I am using python from anaconda!
@ltalirz How about we test if there is a sudo
in the $PATH
, and if so we use it? I don't see any drawback of trying to run sudo
when other ways fail, as long as the user is given clear explanation that AiiDA is asking the password for setting up the postgres database.
How about we test if there is a
sudo
in the$PATH
This is already the case https://github.com/aiidateam/pgsu/blob/f453385faccc708c8c257f8f5599aa346708a978/pgsu/__init__.py#L153
I don't see any drawback of trying to run
sudo
when other ways fail, as long as the user is given clear explanation that AiiDA is asking the password for setting up the postgres database.
Some users get (understandably) rather suspicious if any script asks for a sudo password... on the other hand, we're already doing this for ubuntu, so it's not really something new.
Perhaps the best heuristic to decide whether the sudo
approach should be tried is whether the postgres
system user exists:
import pwd
try:
pwd.getpwnam('postgres')
except KeyError:
print('User postgres does not exist.')
My question then is:
what is the postgresql setting in your WSL setup?
Do you have a postgres
system user with connection setting peer
for local
?
Can you paste the postgresql server configuration?
Is there a way for us to test it on CI?
What was the error message when quicksetup
didn't work?
Can it be improved?
There will always be cases for which autotedection of the postgres setup doesn't work, but one can still use verdi quicksetup
(you can specify just the variables you need to provide). We should make sure that this is clearly spelled out.
Thanks for looking into this. I think it makes sense to use postgres
user as a heuristic. As long as the purposed of asking for sudo
password is communicated to the user I think it would be fine.
what is the postgresql setting in your WSL setup?
I am not quite sure what you refers to - it should be identical to the Ubuntu setup. I just install it with apt.
Do you have a postgres system user with connection setting peer for local?
Yes, there is a postgres users added when the package is installed by apt.
Can you paste the postgresql server configuration?
Please see the attached acharive - I just zipped the whole /etc/postgresql/10/main/
folder.
postgresql.tar.gz
Is there a way for us to test it on CI?
I am not sure, but any update-to-date Windows 10 should be able to run WSL1. WSL2 requires a HyperV so I am not sure if that is possible.
What was the error message when quicksetup didn't work? Can it be improved?
Please provide PostgreSQL connection info:
postgres host []: localhost
postgres port [5432]:
postgres super user [postgres]:
database [template1]:
postgres password of postgres []:
Unable to autodetect postgres setup.
Critical: failed to determine the PostgreSQL setup
with pgsu == 0.1.0
and aiida-core==1.5.2
, there is no password for postgres
users so I left it blank.
Thanks @zhubonan !
For the record, the postgresql configuration on WSL is indeed identical to Ubuntu, i.e. there is a postgresql
system user with peer
authentication. Excerpt from pg_hba.conf
:
# Database administrative login by Unix domain socket
local all postgres peer
# TYPE DATABASE USER ADDRESS METHOD
# "local" is for Unix domain socket connections only
local all all peer
# IPv4 local connections:
host all all 127.0.0.1/32 md5
# IPv6 local connections:
host all all ::1/128 md5
Note to self: While peer
authentication is, in principle, available for all unix users, a corresponding postgresql database user is only created for the postgres
system user - i.e. when you try psql
as a random system user, you get
ubuntu@qmobile:~$ psql
psql: FATAL: role "ubuntu" does not exist
Since the available method for host-based login is md5
(password), I think that indeed the only way to connect as the postgresql superuser is to become the postgres
system user.
Related to https://github.com/aiidateam/aiida-core/issues/4748. The the sudo is not only needed for
quicksetup
but also other things likeverdi profile delete
etc.Because WSL does not have version keys containing
Ubuntu
, the use ofsudo
can not be set.Perhaps there the package can use an environmental variable to force the use of
sudo
a workaround.platform.version
gives