Univention Bugzilla – Bug 52945
do not import all ucsschool.lib.model symbols from all submodules at package import time
Last modified: 2021-05-26 11:28:54 CEST
When trying to import a single module from the ucsschool.lib.model package, all symbols from all of its submodules are imported. This is an unnecessary waste of resources and makes testing more difficult. * Change imports in the whole UCS@school code base to use the fully dotted path to the objects that should be imported. * Do not remove the * imports from __init__.py in 4.4, but deprecate it for the 5.0 release. * Remove the * imports from __init__.py in the Kelvin branch now.
Fixed for 4.4 in branch dtroeder/52945_schoollib_no_star_imports: [081ada665] Bug #52945: use full path to model imports (4.4) Fixed for Kelvin in dtroeder/kelvin_pre-commit branch: [098380a2f] Bug #52945: use full path to model imports Please reopen for merge to 4.4. I would then send messages to all developers of customer code (ProfS) that I have seen to use imports from "ucsschool.lib.models.__init__".
QA so far :) Reopen, some things are missing. Thanks! This looks like a lot of valuable hard work! * Do not remove the * imports from __init__.py in 4.4, but deprecate it for the 5.0 release. → This is missing. ---- === Code Review === --- 4.4 feature branch --- Other than that the imports look fine: 80_move_users_into_another_ou.py from ucsschool.lib.models.group import SchoolClass, User, WorkGroup 207_import-users_school_change.py from ucsschool.lib.models.group import SchoolClass, User, WorkGroup as LibWorkGroup rgrep -e "from ucsschool.lib.models import" ucs-* → no results → OK check if there are more imports in 4.4 git diff oldCommit..newCommit in 4.4 branch there is one new problematic import: git diff 74a4e675b00fd4ef434dd0f19ca4c050176e166b..dfef255d6ce415d4badb0ef54cf04a0e4171db0c | egrep -C 15 "^\+\s*from ucsschool.lib.models import" → 01_test_school_creation_groups.py * Remove the * imports from __init__.py in the Kelvin branch now. → OK --- feature/kelvin branches --- code looks ok Most of the code has already been published with Bug 52817, only this is missing in feature/kelvin ucs-school-lib/modules/ucsschool/lib/tests/test_validation_users.py:from ucsschool.lib.models import validator as validator try: import ConfigParser # py2 except ImportError: from configparser import ConfigParser # py3 → not needed in kelvin, right? === Functionalities === Build all packages (4.4) folders=$(find . -name "ucs*" -type d -maxdepth 1 -printf '%f\n') for folder in $folders; do echo "$folder"; cd "$folder" && apt build-dep . -y && cd .. ; done for folder in $folders; do echo "$folder"; cd "$folder" && dpkg-buildpackage && cd .. ; done # (removed packages for not matching to role) dpkg -i *.deb → no problems on single-server → no problems on primary node → no problems on replication node === Todos === - fix missing imports - rebase branches - create merge request against 5.0 - if possible: run branch test
I made the following commits to other repositories: = components/apple-school-manager = [4.4 559e3e9] Bug #52945: use full path to ucsschool.lib.model imports = ucs = [4.4-8 06d99f28d9] Bug #52945: use full path to ucsschool.lib.model imports = ucsschool-hooks = [4.4 8a347be] Bug #52945: use full path to ucsschool.lib.model imports
Thanks for the QA. All issues were addressed: (In reply to Tobias Wenzel from comment #2) > * Do not remove the * imports from __init__.py in 4.4, but deprecate it for > the 5.0 release. > → This is missing. The 5.0 branch was updated: [3b4345ca5] Bug #52945: use full path to ucsschool.lib.model imports (5.0) > === Code Review === > > --- 4.4 feature branch --- > > Other than that the imports look fine: > > 80_move_users_into_another_ou.py > from ucsschool.lib.models.group import SchoolClass, User, WorkGroup > > 207_import-users_school_change.py > from ucsschool.lib.models.group import SchoolClass, User, WorkGroup as > LibWorkGroup .. > in 4.4 branch there is one new problematic import: > > → 01_test_school_creation_groups.py The branch dtroeder/52945_schoollib_no_star_imports was rebased on 4.4 and the imports fixed: [65f479d00] Bug #52945: use full path to model imports (4.4) [a4576ad4a] Bug #52945: fix imports > --- feature/kelvin branches --- > > code looks ok > > Most of the code has already been published with Bug 52817, only this is > missing in feature/kelvin > > ucs-school-lib/modules/ucsschool/lib/tests/test_validation_users.py:from > ucsschool.lib.models import validator as validator That import is OK. In Kelvin there is no star import in ucsschool.lib.models.__init__ anymore. The above import is possible, because "validator" is a module in the package "ucsschool.lib.models". > try: > import ConfigParser # py2 > except ImportError: > from configparser import ConfigParser # py3 > > → not needed in kelvin, right? Correct, but it should stay anyway. In the Kelvin branch this was commited 2019 as part of making the school.lib Python 3 compatible. In the 4.4 branch this is now for the same reason (for 5.0). The code should stay this way in feature/kelvin, because it makes maintaining the Kelvin fork easier if the code in 4.4, 5.0 and feature/kelvin are as similar as possible. > === Todos === > > - fix missing imports done > - rebase branches done > - create merge request against 5.0 Already commited and pushed. > - if possible: run branch test AFAIK those are broken, because italc doesn't build, but I started one anyway: https://jenkins.knut.univention.de:8181/job/UCS%20Branch%20Test/613/
QA → reopen Looks good, only one mistake. ucs code looks good → ok components/apple-school-manager code looks good → ok build → ok tests pass → ok ucsschool-hooks code looks good → ok execute without failure → ok The 5.0 branch was updated: rgrep -e "from ucsschool.lib.models import" ucs-* → no result, ok in 207_import-users_school_change.py from ucsschool.lib.models.group import SchoolClass, User, WorkGroup as LibWorkGroup → reopen The branch dtroeder/52945_schoollib_no_star_imports was rebased on 4.4 and the imports fixed build succeeds → OK > That import is OK. In Kelvin there is no star import in ucsschool.lib.models.__init__ anymore. The above import is possible, because "validator" is a module in the package "ucsschool.lib.models". Thanks for the explanation!
* in branch "5.0" 207_import-users_school_change.py was fixed: [5.0] 2365e251c fixup! Bug #52945: use full path to ucsschool.lib.model imports (5.0) * in feature/kelvin the imports in ucs-test-ucsschool were fixed, although they are never used, just to make sure there are no misunderstanding. Should the branch dtroeder/52945_schoollib_no_star_imports be squashed and merged into 4.4, build, yaml?
* in feature/kelvin the imports in ucs-test-ucsschool were fixed, although they are never used, just to make sure there are no misunderstanding. Sure. I just did not want to introduce new wrong code (even though it is never used). Should the branch dtroeder/52945_schoollib_no_star_imports be squashed and merged into 4.4, build, yaml? Yes, please → reopen for that
Code was merged and built: [4.4] c3b5fc4fa Bug #52945: use full path to model imports (4.4) [4.4] 5694e6902 Bug #52945: changelogs and advisories [4.4] 3657641a7 Bug #52945: advisory updates ucs-school-lib 12.2.25 ucs-school-import 17.0.57 ucs-school-metapackage 12.0.4-16 ucs-school-umc-computerroom 11.0.0-31 ucs-school-umc-diagnostic 1.0.0-20 ucs-school-umc-exam 9.0.1-52 ucs-school-umc-groups 9.0.0-12 ucs-school-umc-helpdesk 15.0.0-3 ucs-school-umc-installer 7.0.1 ucs-school-umc-internetrules 15.0.1 ucs-school-umc-printermoderation 16.0.1 ucs-school-umc-rooms 16.1.1 ucs-school-umc-users 15.0.1 ucs-school-umc-wizards 11.0.1 ucs-school-webproxy 15.0.1 ucs-test-ucsschool 6.0.223
QA: all ok → verify merge → no conflicts changelog → ok yamls → ok (version correct, src correct etc.)
REOPEN: no merge request for git:06d99f28d980d1e1e625f6aec530ab3d0c4fea4b for UCS 5.0-0 has been done.
https://git.knut.univention.de/univention/ucs/-/merge_requests/94
Verify → merge request for 5.0.0 has been done
Errata updates for UCS@school 4.4 v9 have been released. https://docs.software-univention.de/changelog-ucsschool-4.4v9-de.html If this error occurs again, please clone this bug.