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
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. """
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
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
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
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
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
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
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
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
alexey-tikhonov commented: """ Sorry it fell through the cracks. LGTM.
Small question - if usage of `os.path.abspatch` also solves the issue? Can you print how the paths are evaluated then?
I think `realpath` is better, `abspatch` doesn't resolve sym links.
"""
See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-932161779
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
sergiodj commented: """ Thanks, @alexey-tikhonov. """
See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-932376116
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
pbrezina commented: """ Pushed PR: https://github.com/SSSD/sssd/pull/5636
* `master` * efd155f0abebd2aae3f1a910fced1326bf3fa6a6 - Improve assertion when verifying paths for Python modules
"""
See the full comment at https://github.com/SSSD/sssd/pull/5636#issuecomment-934228853
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5636 Title: #5636: Improve assertion when verifying paths for Python modules
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/5636 Author: sergiodj Title: #5636: Improve assertion when verifying paths for Python modules Action: closed
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
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
elkoniu
-
pbrezina
-
sergiodj