Skip to content

Commit

Permalink
Remove the init_namespace() method in favor of using the notify_chang…
Browse files Browse the repository at this point in the history
…e() method (PR #3328)

# Description

While trying the find out why the `ActiveEnv.init_namespace()` method was required, I noticed it would be better to make this logic part of the `ActiveEnv.notify_change()` method. In the previous implementation `init_namespace()` has to be called manually from different locations and in a specific order. The new implementation hides all the complexity in the `ActiveEnv` class.

# Self Check:

- [ ] ~~Attached issue to pull request~~
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [ ] ~~End user documentation is included or an issue is created for end-user documentation~~

# Reviewer Checklist:

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
  • Loading branch information
arnaudsjs authored and inmantaci committed Oct 6, 2021
1 parent 2a84ee7 commit 4c81bd1
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 21 deletions.
4 changes: 4 additions & 0 deletions changelogs/unreleased/remove-init-namespace-method.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
description: Remove the init_namespace() method in favor of using the notify_change() method
change-type: patch
destination-branches: [master]
50 changes: 33 additions & 17 deletions src/inmanta/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import pkg_resources
from pkg_resources import DistInfoDistribution, Requirement

from inmanta import const
from packaging import version

try:
Expand Down Expand Up @@ -544,23 +545,6 @@ def get_module_file(cls, module: str) -> Optional[Tuple[Optional[str], Loader]]:
spec = None
return (spec.origin, spec.loader) if spec is not None else None

def init_namespace(self, namespace: str) -> None:
"""
Make sure importer will be able to find the namespace packages for this namespace that will get installed in the
process venv. This method needs to be called before the importer caches the search paths, so make sure to call it
before calling get_module_file for this namespace.
:param namespace: The namespace to initialize.
"""
path: str = os.path.join(self.site_packages_dir, namespace)
os.makedirs(path, exist_ok=True)
spec: Optional[ModuleSpec] = importlib.util.find_spec(namespace)
if spec is None or spec.submodule_search_locations is None or path not in spec.submodule_search_locations:
raise Exception(
"Invalid state: trying to init namespace after it has been loaded. Make sure to call this method before calling"
" get_module_file for this namespace."
)

def notify_change(self) -> None:
"""
This method must be called when a package is installed or removed from the environment in order for Python to detect
Expand All @@ -573,6 +557,38 @@ def notify_change(self) -> None:
site.addsitedir(self.site_packages_dir)
importlib.invalidate_caches()

if const.PLUGINS_PACKAGE in sys.modules:
mod = sys.modules[const.PLUGINS_PACKAGE]
if mod is not None:
# Make mypy happy
assert mod.__spec__.submodule_search_locations is not None
if self.site_packages_dir not in mod.__spec__.submodule_search_locations and os.path.exists(
os.path.join(self.site_packages_dir, const.PLUGINS_PACKAGE)
):
"""
A V2 module was installed in this virtual environment, but the inmanta_plugins package was already
loaded before this venv was activated. Reload the inmanta_plugins package to ensure that all V2 modules
installed in this virtual environment are discovered correctly.
This is required to cover the following scenario:
* Two venvs are stacked on top of each other. The parent venv contains the inmanta-core package and
the subvenv is empty.
* The inmanta_plugins package gets loaded before a V2 module is installed in the subvenv. This way,
the module object in sys.modules, doesn't have the site dir of the subvenv in its
submodule_search_locations. This field caches where the loader should look for the namespace packages
that are part of the inmanta_plugins namespace.
* When a V2 module gets installed in the subvenv now, the loader will not find the newly installed V2
module, because it's not considering the site dir of the subvenv.
The above-mentioned scenario can only be triggered by test cases, because:
1) The compiler venv was removed. As such, no new venv are activated on the fly by production code
paths.
2) The compiler service creates a new subvenv for each inmanta environment, but the inmanta commands
are executed in a subprocess.
"""
importlib.reload(mod)


process_env: ActiveEnv = ActiveEnv(python_path=sys.executable)
"""
Expand Down
1 change: 0 additions & 1 deletion src/inmanta/module.py
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,6 @@ def _get_module_name(self, module_spec: List[InmantaModuleRequirement]) -> str:
class ModuleV2Source(ModuleSource["ModuleV2"]):
def __init__(self, urls: List[str]) -> None:
self.urls: List[str] = [url if not os.path.exists(url) else os.path.abspath(url) for url in urls]
env.process_env.init_namespace(const.PLUGINS_PACKAGE)

@classmethod
def get_installed_version(cls, module_name: str) -> Optional[version.Version]:
Expand Down
3 changes: 0 additions & 3 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,6 @@ def _patch_process_env(self) -> None:
running process.
"""
env.process_env.__init__(env_path=self.env)
env.process_env.init_namespace(const.PLUGINS_PACKAGE)

def _install_v2_modules(self, install_v2_modules: Optional[List[LocalPackagePath]] = None) -> None:
install_v2_modules = install_v2_modules if install_v2_modules is not None else []
Expand Down Expand Up @@ -1343,8 +1342,6 @@ def tmpvenv_active(

# patch env.process_env to recognize this environment as the active one, deactive_venv restores it
env.process_env.__init__(python_path=str(python_path))
# patch inmanta_plugins namespace path so importlib.util.find_spec("inmanta_plugins") includes the venv path
env.process_env.init_namespace(const.PLUGINS_PACKAGE)
env.process_env.notify_change()

# Force refresh build's decision on whether it should use virtualenv or venv. This decision is made based on the active
Expand Down

0 comments on commit 4c81bd1

Please sign in to comment.