[SSSD] [sssd PR#5636][opened] Improve assertion when verifying paths for Python modules

[sergiodj]

URL: https://github.com/SSSD/sssd/pull/5636
Author: sergiodj
Title: #5636: Improve assertion when verifying paths for Python modules
Action: opened

PR body:
"""
In Ubuntu we're facing a problem where the 3 Python tests under
src/tests/*-test.py are failing due to cosmetical differences between
what the '.__file__' method returns and what 'MODPATH' ends up being.

I have not been able to pinpoint exactly what is causing this issue;
it only happens when SSSD is built inside a chroot environment (with
sbuild, for example). The logs look like this:

```python
F
======================================================================
FAIL: testImport (__main__.PyHbacImport)
Import the module and assert it comes from tree
----------------------------------------------------------------------
Traceback (most recent call last):
File "/<<PKGBUILDDIR>>/src/tests/pyhbac-test.py", line 91, in testImport
self.assertEqual(pyhbac.__file__, MODPATH + "/pyhbac.so")
AssertionError: '/<<PKGBUILDDIR>>/build/./tp_pyhbac_xw2omut2/pyhbac.so' != './tp_pyhbac_xw2omut2/pyhbac.so'
- /<<PKGBUILDDIR>>/build/./tp_pyhbac_xw2omut2/pyhbac.so
+ ./tp_pyhbac_xw2omut2/pyhbac.so
```

Given that the intention of the test is to verify that the two paths
are equal, I suggest that we do this slight improvement and call
'os.path.realpath' before comparing both paths. This way we guarantee
that they're both properly canonicalized.

I have verified that the tests still pass with this change.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5636/head:pr5636
git checkout pr5636

[sergiodj]

URL: https://github.com/SSSD/sssd/pull/5636
Author: sergiodj
Title: #5636: Improve assertion when verifying paths for Python modules
Action: edited

Changed field: body
Original value:
"""
In Ubuntu we're facing a problem where the 3 Python tests under
src/tests/*-test.py are failing due to cosmetical differences between
what the '.__file__' method returns and what 'MODPATH' ends up being.

I have not been able to pinpoint exactly what is causing this issue;
it only happens when SSSD is built inside a chroot environment (with
sbuild, for example). The logs look like this:

```python
F
======================================================================
FAIL: testImport (__main__.PyHbacImport)
Import the module and assert it comes from tree
----------------------------------------------------------------------
Traceback (most recent call last):
File "/<<PKGBUILDDIR>>/src/tests/pyhbac-test.py", line 91, in testImport
self.assertEqual(pyhbac.__file__, MODPATH + "/pyhbac.so")
AssertionError: '/<<PKGBUILDDIR>>/build/./tp_pyhbac_xw2omut2/pyhbac.so' != './tp_pyhbac_xw2omut2/pyhbac.so'
- /<<PKGBUILDDIR>>/build/./tp_pyhbac_xw2omut2/pyhbac.so
+ ./tp_pyhbac_xw2omut2/pyhbac.so
```

Given that the intention of the test is to verify that the two paths
are equal, I suggest that we do this slight improvement and call
'os.path.realpath' before comparing both paths. This way we guarantee
that they're both properly canonicalized.

I have verified that the tests still pass with this change.
"""

[elkoniu]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

elkoniu commented:
"""
Thank you for this PR. If there is a chance you can run the test again and show how `os.path.realpath(pyhbac.__file__)` and `os.path.realpath(MODPATH + "/pyhbac.so")` are evaluated on your chroot environment? Simple `print` will be good enough.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-843584540

[sergiodj]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

sergiodj commented:
"""
Thanks for the reply, @elkoniu.

Here's how the paths are evaluated:

```
realpath(pyhbac.__file__) = /tmp/sssd/build/.libs/_py3hbac.so
realpath(MODPATH + /pyhbac.so) = /tmp/sssd/build/.libs/_py3hbac.so
```

You can also check that, without the patch, the paths are evaluated as:

```
'/<<PKGBUILDDIR>>/build/./tp_pyhbac_xw2omut2/pyhbac.so'
'./tp_pyhbac_xw2omut2/pyhbac.so'
```

Where `<<PKGBUILDDIR>>` is just a mnemonic for some temporary path that `sbuild` uses. They are the same path, but the second one is relative.

Thanks.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-843595627

[elkoniu]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

elkoniu commented:
"""
If I understand this test cases correctly the steps are (based on `pyhbac` usecase):
1) Create temporary `MODPATH` subdirectory under `TEST_DIR`
2) Depending on python version make symbolic link to correct `pyhbac.so` version in the `MODPATH` directory
3) Import `pyhbac`
4) Confirm that imported `pyhbac` module path is the same as created `pyhbac.so` symlink path.

What `chroot` breaks is injection of prefix `/<<PKGBUILDDIR>>/build/` into loaded module path.
By using `realpath()` you forcing following symbolic links for both: chroot path and the link we created in steep (2).
I think functionally it is correct. What I am wondering is, if we should detect and thread chroot environment special here.
For example instead of calling `realpath()` - subtract "chroot" piece from `module.__file__`.

Can you check if any module loaded into chroot environment will have this chroot-specific prefix added to `module.__file__`? Maybe this should be addressed in Python directly.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-843620673

[sergiodj]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

sergiodj commented:
"""
Heh, I had written a whole wall of text replying to your last comment, but then I investigated a bit more and ended up finding what's happening. In a nutshell:

* Ubuntu Impish (the development version) is using Python 3.9.5.
* Debian sid is using Python 3.9.2.

When I looked at the Python 3.9.5 changelog, I found this bug:

https://bugs.python.org/issue43105

And voilà: everything makes sense. Python 3.9.5+ resolves relative paths in imported modules, which breaks the current test because, unless `SSS_TEST_DIR` is set (which it is not), the path will always be relative. IMHO, and if I understand the purpose of the test, this means that the proposed change is actually the correct way to address this problem.

For what it's worth, and because I had written so much before:

* I don't think there is a way to determine the "chroot" part from `module.__file__`, because from what I gathered the path change happens even when you're building sssd outside of a chroot (inside a VM, a container or even natively, for example). Moreover, it's not really possible to determine that we're inside a chroot just by looking at this path. For example, for `sbuild` the `<<PKGBUILDDIR>>` part is actually something like `build/sssd-GhFpxp/sssd-2.4.1`, which is a regular path like any other.

Thanks!
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-843637837

[elkoniu]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

elkoniu commented:
"""
Thank you for this Python investigation:) So far this PR LGTM but I would like a second pair of eyes to take a look at it too. On the morning I will try to ping some Python specialist from the team for final ACK.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-843641286

[sergiodj]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

sergiodj commented:
"""
Fair enough, thank you!
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-843646012

[elkoniu]

URL: https://github.com/SSSD/sssd/pull/5636
Title: #5636: Improve assertion when verifying paths for Python modules

elkoniu commented:
"""
Small question - if usage of `os.path.abspatch` also solves the issue? Can you print how the paths are evaluated then?
"""

See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-844892228