diff --git a/.ruff.toml b/.ruff.toml index b34b48653..55e209384 100644 --- a/.ruff.toml +++ b/.ruff.toml @@ -106,6 +106,9 @@ ignore = [ "F403", # ignore * imports "I001", # ignore unsorted imports ] +"tools/*" = [ + "T201", # standalone developer scripts report their results on stdout +] [lint.pydocstyle] convention = "pep257" diff --git a/README.md b/README.md index f5969a1e4..112d932ad 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,18 @@ Before you can run this tool, make sure the below requirements are set on your s source install requirements - [Other Odoo dependencies](https://www.odoo.com/documentation/19.0/administration/on_premise/source.html#dependencies) +Odoo builds part of its Python dependencies (`gevent`, `lxml`, `python-ldap`, …) from source, which needs a C compiler +and the matching development packages: the headers of the Python version Odoo runs on, the PostgreSQL client library +and the OpenLDAP and SASL headers. On Debian and Ubuntu, install them all from the Odoo sources Odev has cloned: + +```sh +sudo ~/odoo/repositories/odoo/odoo/setup/debinstall.sh +``` + +On Fedora, Arch, openSUSE, Alpine or macOS, install the equivalents with your own package manager. Odev checks +whenever it creates a virtual environment for a version of Odoo and, whatever the system, tells you what is missing +along with the command that installs it. It never installs anything itself. + Make sure `git` is properly setup with SSH key authentication before using commands, as Odev will try to connect to the Odoo [Community](https://github.com/odoo/odoo) and [Enterprise](https://github.com/odoo/enterprise) repositories to pull sources when required. diff --git a/docs/tutorials/commands.md b/docs/tutorials/commands.md index 6a6f353a3..230c34fc5 100644 --- a/docs/tutorials/commands.md +++ b/docs/tutorials/commands.md @@ -18,6 +18,7 @@ In this basic tutorial, we'll see how we can define a brand new command to print - [Going further...](#going-further) - [Extended command classes](#extended-command-classes) - [Mutually exclusive arguments](#mutually-exclusive-arguments) + - [Flags that can be turned off](#flags-that-can-be-turned-off) - [Unknown arguments](#unknown-arguments) - [Raising errors](#raising-errors) @@ -265,6 +266,32 @@ class SampleCommand(Command): ![odev sample](img/command-sample-08.png) +### Flags that can be turned off + +A flag defined with `args.Flag` is either present or absent, so a command cannot tell "the user asked for the value to +be turned off" apart from "the user did not mention it". When that distinction matters, typically for a command editing +a value that already exists, use `args.FlagOptional` instead: it registers a `--no-` counterpart for each of its +aliases. + +```python +class SampleCommand(Command): + """Example command used for tutorials purposes.""" + + _name = "sample" + _aliases = ["example"] + + flag = args.FlagOptional(aliases=["--flag"], description="Sample three-state flag argument") + + def run(self): + if self.args.flag is None: + self.console.print("The flag was not mentioned, leaving the value as it is") + else: + self.console.print(f"The flag was set to {self.args.flag}") +``` + +`--flag` sets the value to `True`, `--no-flag` sets it to `False`, and omitting both leaves it at its default, `None` +unless another one is given to the argument. + ### Unknown arguments By default, Odev will treat any unknown argument received as invalid and raise an error. diff --git a/docs/tutorials/plugins.md b/docs/tutorials/plugins.md index 19fc4300d..71c956c60 100644 --- a/docs/tutorials/plugins.md +++ b/docs/tutorials/plugins.md @@ -9,6 +9,10 @@ To enable a plugin, run `odev plugin --enable /`. - [Plugins](#plugins) - [Table of contents](#table-of-contents) + - [Finding and managing plugins](#finding-and-managing-plugins) + - [Searching for plugins](#searching-for-plugins) + - [Listing local plugins](#listing-local-plugins) + - [Inspecting a plugin](#inspecting-a-plugin) - [Creating a new plugin](#creating-a-new-plugin) - [Plugin structure](#plugin-structure) - [The manifest](#the-manifest) @@ -18,6 +22,64 @@ To enable a plugin, run `odev plugin --enable /`. - [Adding a new command](#adding-a-new-command) - [Extending a command](#extending-a-command) +## Finding and managing plugins + +### Searching for plugins + +Run `odev plugin --search` to look for plugins published on GitHub. Odev queries the GitHub search API for the `odev` +and `plugin` keywords, then keeps only the repositories exposing a valid [manifest](#the-manifest) at their root, so +unrelated repositories never show up in the results. + +Add a term to narrow the search down, quoting it if it contains several words: + +```sh +odev plugin --search editor +odev plugin --search "upgrade code" +``` + +The results are displayed in a table showing, for each plugin, the version declared on its default branch, its number +of stars and whether it is already available locally. Archived repositories and the +[template repository](https://github.com/odoo-odev/odev-plugin-template), which cannot be installed, are left out. +`--limit` caps how many repositories are inspected (20 by default) to stay within the GitHub API rate limits. + +> [!NOTE] +> +> Searching never installs anything. Copy the name of a plugin from the results and run +> `odev plugin --enable /` to install it. + +### Listing local plugins + +Run `odev plugin --list` to display every plugin available on your machine, in one of the following states: + +| State | Meaning | +| ---------- | --------------------------------------------------------------------------------------------- | +| `enabled` | The plugin is loaded by odev. | +| `shadowed` | The plugin is enabled but another plugin already uses its module name, so it cannot be loaded. | +| `missing` | The plugin is enabled but its link under `~/.config/odev/plugins` is gone. | +| `disabled` | The plugin was downloaded previously but is not enabled; re-enabling it will not clone it again. | + +### Inspecting a plugin + +Use `odev plugin --show /` to get the details of a single plugin: its state, version, +branch, path, dependencies and description. Without an argument, `--show` details every plugin available locally, in +the same order as `--list`. + +A plugin that is not on your machine is looked up on GitHub, so `--show` also describes plugins you have not installed +yet, or that you uninstalled and whose clone you deleted: + +```sh +odev plugin --show odoo-odev/odev-plugin-editor-vscode +``` + +Plugins already available locally are read from disk and never trigger a request to GitHub. The full +`/` name is required to look a plugin up remotely: a repository name on its own is only +matched against the plugins present on your machine, as long as it is not ambiguous. + +> [!NOTE] +> +> When GitHub cannot be reached — no token configured, no network, rate limit exceeded — `--show` silently falls back +> to the information available locally instead of failing. + ## Creating a new plugin To create a new odev plugin, start by creating a new repository. You can copy a @@ -52,6 +114,9 @@ Create a new file `__manifest__.py` at the root of your plugin with the followin repository). Replace the docstring by a summary of your module's features. This will be read by Odev and displayed when required by the `odev plugin` command. +The `__version__` assignment is also what makes a repository recognizable as a plugin: a repository without a root +`__manifest__.py` declaring it is ignored by `odev plugin --search`. + If any, add the dependencies (other plugins) of your own plugin. For example, `odoo-odev/odev-plugin-editor-vscode` depends on the abstract plugin `odoo-odev/odev-plugin-editor-base` which is therefore required for the plugin to work: [VScode Editor plugin's depends](https://github.com/odoo-odev/odev-plugin-editor-vscode/blob/main/__manifest__.py#L38). diff --git a/odev/_version.py b/odev/_version.py index 7306664df..9c8ce602e 100644 --- a/odev/_version.py +++ b/odev/_version.py @@ -22,4 +22,4 @@ # or merged change. # ------------------------------------------------------------------------------ -__version__ = "4.29.9" +__version__ = "4.31.3" diff --git a/odev/commands/database/database.py b/odev/commands/database/database.py new file mode 100644 index 000000000..6d01acc6f --- /dev/null +++ b/odev/commands/database/database.py @@ -0,0 +1,187 @@ +"""Edit the parameters of a local database without starting it.""" + +from odev.common import args, string +from odev.common.commands import GitCommand, LocalDatabaseCommand +from odev.common.connectors import GitConnector +from odev.common.errors import ConnectorError +from odev.common.logging import logging +from odev.common.python import PythonEnv + + +logger = logging.getLogger(__name__) + + +class DatabaseSetCommand(LocalDatabaseCommand, GitCommand): + """Display and edit the parameters of a local database without starting it. + Called without any argument other than the database, the current parameters are displayed. + """ + + _name = "database" + _aliases = ["db"] + + set_repository = args.String( + aliases=["--set-repo"], + description="""Change the repository linked to the database. Accepts a repository name in + the format /, a git URL or the path to a local clone. + """, + metavar="REPOSITORY", + ) + remove_repository = args.Flag( + aliases=["--remove-repo"], + description="Remove the repository linked to the database.", + ) + + set_venv = args.String( + aliases=["--set-venv"], + description="Change the virtualenv linked to the database.", + metavar="VENV", + ) + remove_venv = args.Flag( + aliases=["--remove-venv"], + description="Remove the virtualenv linked to the database.", + ) + + set_worktree = args.String( + aliases=["--set-worktree"], + description="Change the worktree linked to the database.", + metavar="WORKTREE", + ) + remove_worktree = args.Flag( + aliases=["--remove-worktree"], + description="Remove the worktree linked to the database.", + ) + + whitelist = args.FlagOptional( + aliases=["--whitelist"], + description="Whitelist or unwhitelist the database, preventing or allowing its automatic removal.", + ) + + _exclusive_pairs = ( + ("set_repository", "remove_repository"), + ("set_venv", "remove_venv"), + ("set_worktree", "remove_worktree"), + ) + """Pairs of arguments that set and remove the same value, and cannot be used together.""" + + @classmethod + def prepare_command(cls, *args, **kwargs) -> None: + super().prepare_command(*args, **kwargs) + cls.remove_argument("version") + + def run(self): + self._check_exclusive_arguments() + + if self._has_changes: + self._set_values() + self._remove_values() + + self._print_values() + + @property + def _has_changes(self) -> bool: + """Whether the command was called with an argument changing a parameter.""" + return bool( + self.args.set_repository + or self.args.remove_repository + or self.args.set_venv + or self.args.remove_venv + or self.args.set_worktree + or self.args.remove_worktree + or self.args.whitelist is not None + ) + + def _check_exclusive_arguments(self) -> None: + """Ensure no parameter is both set and removed in the same call.""" + for set_argument, remove_argument in self._exclusive_pairs: + if getattr(self.args, set_argument) and getattr(self.args, remove_argument): + raise self.error( + f"Arguments {self.argument_name(set_argument)!r} and " + f"{self.argument_name(remove_argument)!r} cannot be used together" + ) + + def argument_name(self, argument: str) -> str: + """Return the CLI alias of an argument, for use in error messages.""" + aliases = self._arguments.get(argument, {}).get("aliases", []) + return next((alias for alias in aliases if alias.startswith("--")), argument) + + def _set_values(self) -> None: + if self.args.set_repository: + self._set_repository(self.args.set_repository) + + if self.args.set_venv: + venv = PythonEnv(self.args.set_venv) + + if not venv.exists: + raise self.error(f"Virtualenv {self.args.set_venv!r} not found, please create it and retry") + + self._database.venv = venv + logger.info(f"Virtualenv set to {venv.name!r}") + + if self.args.set_worktree: + if self.args.set_worktree not in self.grouped_worktrees: + raise self.error(f"Worktree {self.args.set_worktree!r} not found, please create it and retry") + + self._database.worktree = self.args.set_worktree + logger.info(f"Worktree set to {self.args.set_worktree!r}") + + if self.args.whitelist is True: + self._database.whitelisted = True + logger.info("Database whitelisted") + + def _set_repository(self, repository: str) -> None: + """Link a repository to the database, cloning it first if it is missing locally. + + :param repository: The repository name, git URL or path to a local clone. + """ + try: + connector = GitConnector(repository) + except ConnectorError as error: + raise self.error(str(error)) from error + + if not connector.exists and self.console.confirm( + f"Repository {connector.name!r} not found locally, clone now?" + ): + self.odev.run_command("clone", connector.name) + + linked = self._database.link_repository(connector) + logger.info(f"Repository set to {linked.full_name!r}") + + def _remove_values(self) -> None: + # Values are cleared through the data store rather than through the database properties: + # those fall back to reading the store when their cached value is empty, so assigning None + # to them would write the value that was just cleared straight back. + if self.args.remove_repository: + self.store.databases.set_value(self._database, "repository", None) + self.store.databases.set_value(self._database, "branch", None) + self._database._repository = None + self._database._branch = None + logger.info("Repository removed") + + if self.args.remove_venv: + self.store.databases.set_value(self._database, "virtualenv", None) + self._database._venv = None + logger.info("Virtualenv removed") + + if self.args.remove_worktree: + self.store.databases.set_value(self._database, "worktree", None) + self._database._worktree = None + logger.info("Worktree removed") + + if self.args.whitelist is False: + self._database.whitelisted = False + logger.info("Database unwhitelisted") + + def _print_values(self) -> None: + """Print the current parameters of the database.""" + info = self.store.databases.get(self._database) + values = { + "Repository": info and info.repository, + "Branch": info and info.branch, + "Virtualenv": info and info.virtualenv, + "Worktree": info and info.worktree, + "Whitelisted": "yes" if info and info.whitelisted else "no", + } + logger.info( + f"Parameters of database {self._database.name!r}:\n" + + string.join_bullet([f"{key}: {value or 'not set'}" for key, value in values.items()]) + ) diff --git a/odev/commands/database/quickstart.py b/odev/commands/database/quickstart.py index c014ee0bc..f56b25183 100644 --- a/odev/commands/database/quickstart.py +++ b/odev/commands/database/quickstart.py @@ -64,16 +64,31 @@ def run(self): raise self.error(f"Database {self._database.name!r} could not be restored") new_database = LocalDatabase(self.args.name or self._database.name) + + # Link the repository before restoring: `restore` neutralizes the database, and + # neutralization looks for `data/neutralize.sql` scripts in the custom modules found + # under the addons paths derived from the repository linked to the database. + self.link_repository(new_database) self.odev.run_command("restore", dump_file.as_posix(), database=new_database) - if self._database.repository: - if isinstance(self._database.repository, Repository): - repo_org = self._database.repository.organization - repo_name = self._database.repository.name - else: - repo_org, repo_name = self._database.repository.name.split("/") + # `restore` drops and recreates the database, clearing its entry in the data store. + self.link_repository(new_database) + + def link_repository(self, database: LocalDatabase) -> None: + """Link the repository of the source database to the target local database. + + :param database: The database to link the repository to. + """ + if not self._database.repository: + return + + if isinstance(self._database.repository, Repository): + repo_org = self._database.repository.organization + repo_name = self._database.repository.name + else: + repo_org, repo_name = self._database.repository.name.split("/") - new_database.repository = Repository(repo_name, repo_org) + database.repository = Repository(repo_name, repo_org) def get_dump_filename_kwargs(self) -> MutableMapping[str, Any]: """Return the keyword arguments to pass to Database.get_dump_filename().""" diff --git a/odev/commands/git/fetch.py b/odev/commands/git/fetch.py index 02598e5a8..7fc7e9b56 100644 --- a/odev/commands/git/fetch.py +++ b/odev/commands/git/fetch.py @@ -37,10 +37,15 @@ def run(self): for worktree in sorted_worktree: self.run_hook(worktree, changes_by_worktree[worktree]) + # Drop the blank line trailing the output of the last worktree. self.console.clear_line() def run_hook(self, worktree: str, changes: list[tuple[str, int, int]]): - """Print a summary of the pending changes for a worktree.""" + """Print a summary of the pending changes for a worktree. + + Overrides must terminate their output with a blank line, separating consecutive worktrees and letting + :meth:`run` clear the last one. + """ self.table( [ TableHeader("Repository", min_width=max(len(repository.name) for repository in self.repositories)), diff --git a/odev/commands/git/pull.py b/odev/commands/git/pull.py index 40d310bae..c95e48d6b 100644 --- a/odev/commands/git/pull.py +++ b/odev/commands/git/pull.py @@ -17,7 +17,9 @@ class PullCommand(FetchCommand): _help = "Pull changes in local worktrees managed by odev." def run_hook(self, name: str, changes: list[tuple[str, int, int]]): - """Print a summary of the pending changes for a worktree.""" + """Pull the pending changes for a worktree and print a summary of the operation.""" + self.console.title_rule(name) + for change in changes: repository, behind, _ = change worktree = next( @@ -33,19 +35,16 @@ def run_hook(self, name: str, changes: list[tuple[str, int, int]]): raise self.error(f"Worktree {name!r} does not exist") if worktree.detached: - logger.info(f"Worktree {name!r} is detached") + logger.info(f"Detached worktree in {repository!r}") continue if not behind: - logger.info( - f"No pending changes for worktree {name!r} in {repository!r} for version {worktree.branch!r}" - ) + logger.info(f"No pending changes in {repository!r} for version {worktree.branch!r}") continue - with progress.spinner( - f"Pulling {behind} commits in {worktree.connector.name!r} for version {worktree.branch!r}" - ): + with progress.spinner(f"Pulling {behind} commits in {repository!r} for version {worktree.branch!r}"): worktree.connector.pull_worktrees([worktree], force=True) - logger.info(f"Pulled {behind} commits in {worktree.connector.name!r} for version {worktree.branch!r}") + logger.info(f"Pulled {behind} commits in {repository!r} for version {worktree.branch!r}") + self.print() self.odev.config.repositories.set_date(name, datetime.today()) diff --git a/odev/commands/utilities/help.py b/odev/commands/utilities/help.py index 3d3725b90..b5e874ee4 100644 --- a/odev/commands/utilities/help.py +++ b/odev/commands/utilities/help.py @@ -125,22 +125,23 @@ def all_commands_help(self) -> str: arguments without brackets ('arg') are required. """ - commands = [command for name, command in self.odev.commands.items() if name == command._name] + # Read the registry's index rather than the command classes: listing every command must not import them all. + commands = self.odev.commands.summaries() message_indent = string.min_indent(message) commands_list = string.indent( string.format_options_list( [ ( - command._name, - command._help + command.name, + command.help + ( f"\nAliases: " - f"{string.join_and([f'[italic]{alias}[/italic]' for alias in sorted(command._aliases)])}" - if command._aliases + f"{string.join_and([f'[italic]{alias}[/italic]' for alias in sorted(command.aliases)])}" + if command.aliases else "" ), ) - for command in sorted(commands, key=lambda command: command._name) + for command in commands ], blanks=1, ), diff --git a/odev/commands/utilities/plugin.py b/odev/commands/utilities/plugin.py index ec506f4c3..6038e2add 100644 --- a/odev/commands/utilities/plugin.py +++ b/odev/commands/utilities/plugin.py @@ -1,23 +1,127 @@ -"""Enable and disable plugins to add new features and commands.""" +"""Search, enable and disable plugins to add new features and commands.""" -from typing import cast +import textwrap +from collections.abc import Mapping +from dataclasses import dataclass, field, replace +from pathlib import Path +from typing import Any -from odev.common import args, string +from git import InvalidGitRepositoryError, NoSuchPathError +from github import GithubException, RateLimitExceededException +from github.Repository import Repository + +from odev.common import args, progress, string from odev.common.commands import Command -from odev.common.connectors import GitConnector +from odev.common.connectors import GitConnector, GithubConnector +from odev.common.connectors.git import GITHUB_DOMAIN, GITHUB_SEARCH_DEFAULT_LIMIT +from odev.common.console import TableHeader +from odev.common.errors import ConnectorError from odev.common.logging import logging -from odev.common.odev import Plugin +from odev.common.odev import ( + PLUGIN_MANIFEST_FILENAME, + Manifest, + parse_plugin_manifest, + plugin_module_name, +) logger = logging.getLogger(__name__) +SEARCH_KEYWORDS = "odev plugin" +"""Keywords a repository must match to be considered a candidate odev plugin. +The `odev` keyword alone is far too noisy to be usable, as it collides with unrelated repositories. +""" + +SEARCH_QUALIFIERS = "in:name,description,topics" +"""Fields the search keywords are matched against on GitHub.""" + +EXCLUDED_REPOSITORIES = frozenset({"odoo-odev/odev-plugin-template"}) +"""Repositories exposing a plugin manifest but that cannot be installed, such as the template repository +used as a starting point to create new plugins. +""" + +STATE_ENABLED = "enabled" +"""The plugin is linked under the plugins directory and loaded by the framework.""" + +STATE_SHADOWED = "shadowed" +"""The plugin is enabled in the configuration but another plugin already uses its module name.""" + +STATE_MISSING = "missing" +"""The plugin is enabled in the configuration but is not linked under the plugins directory.""" + +STATE_DISABLED = "disabled" +"""The plugin is available locally but is not enabled.""" + +STATE_NOT_DOWNLOADED = "not downloaded" +"""The plugin exists on GitHub but is not available locally.""" + +STATE_STYLES: Mapping[str, str] = { + STATE_ENABLED: "color.green", + STATE_SHADOWED: "color.yellow", + STATE_MISSING: "color.red", + STATE_DISABLED: "color.black", + STATE_NOT_DOWNLOADED: "color.black", +} +"""Style used to render each possible state of a plugin.""" + +STATE_ORDER: tuple[str, ...] = ( + STATE_ENABLED, + STATE_SHADOWED, + STATE_MISSING, + STATE_DISABLED, + STATE_NOT_DOWNLOADED, +) +"""Order in which plugins are sorted in tables, most relevant states first.""" + +DESCRIPTION_MAX_WIDTH = 60 +"""Maximum width of a plugin description before it gets truncated in tables.""" + +NAME_MIN_WIDTH = 32 +"""Minimum width reserved for plugin names in tables, as they are needed to enable a plugin.""" + + +@dataclass(frozen=True) +class PluginInfo: + """Everything known about a plugin, whether it is available locally or only on GitHub.""" + + name: str + """Name of the plugin, in the format `organization/repository`.""" + + state: str + """Current state of the plugin, one of the `STATE_*` constants.""" + + version: str = "" + """Version number declared in the manifest of the plugin.""" + + branch: str = "" + """Branch currently checked out in the local clone of the plugin.""" + + path: Path | None = None + """Path to the local clone of the plugin, if it exists.""" + + depends: list[str] = field(default_factory=list) + """Other plugins this one depends on.""" + + description: str = "" + """Description of the plugin, taken from the docstring of its manifest.""" + + shadowed_by: str = "" + """Plugin already using the module name of this one, preventing it from being loaded.""" + + stars: int | None = None + """Number of stars of the repository, `None` if it was not looked up on GitHub.""" + + archived: bool = False + """Whether the repository is archived and no longer maintained.""" + + class PluginCommand(Command): - """Enable and disable plugins to add new features and commands.""" + """Search, enable and disable plugins to add new features and commands.""" _name = "plugin" _aliases = ["plugins"] - _exclusive_arguments = [("enable", "disable", "show")] + _exclusive_arguments = [("enable", "disable", "show", "search", "list")] enable = args.Flag(aliases=["-e", "--enable"], description="Download and enable an inactive plugin.") disable = args.Flag(aliases=["-d", "--disable"], description="Disable an active plugin.") @@ -25,10 +129,27 @@ class PluginCommand(Command): aliases=["-s", "--show"], description="Show the state of a plugin and its description if available.", ) + search = args.Flag( + aliases=["-S", "--search"], + description="""Search GitHub for plugins matching the given terms. + Only repositories exposing a valid plugin manifest are listed. + """, + ) + action_list = args.Flag( + name="list", + aliases=["-l", "--list"], + description="List the plugins available locally, whether they are enabled or not.", + ) + limit = args.Integer( + aliases=["-n", "--limit"], + default=GITHUB_SEARCH_DEFAULT_LIMIT, + description="Maximum number of repositories to inspect when searching for plugins.", + ) plugin = args.String( description="""Plugin to enable or disable, must be a git repository hosted on GitHub. Use format /. - If `--show` is used and no plugin is provided, show the state of all plugins. + If `--show` is used and no plugin is provided, show the state of all enabled plugins. + If `--search` is used, this is the term to search for; quote it to search for multiple terms. """, nargs="?", ) @@ -36,58 +157,465 @@ class PluginCommand(Command): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if not self.args.plugin and not self.args.show: + self.__local_plugins: list[PluginInfo] | None = None + """Cached list of the plugins known locally.""" + + if not self.args.plugin and not (self.args.show or self.args.search or self.args.list): raise self.error("Missing argument: plugin") def run(self): - """Enable or disable a plugin.""" - if self.args.show: - if self.args.plugin: - self.__show_plugin_info(self.args.plugin.split("/")[-1]) - return + """Search, list, enable or disable plugins.""" + if self.args.search: + self.search_plugins() - for plugin in self.odev.plugins: - self.__show_plugin_info(plugin.name) - self.console.print() + if self.args.list: + self.list_plugins() + + if self.args.show: + self.show_plugins() if self.args.enable: - self.odev.install_plugin(self.args.plugin) - return + self.odev.install_plugin(self.__resolve_plugin_name(self.args.plugin)) if self.args.disable: - self.odev.uninstall_plugin(self.args.plugin) + self.odev.uninstall_plugin(self.__resolve_plugin_name(self.args.plugin)) + + # --- Searching plugins on GitHub ------------------------------------------ + + def search_plugins(self) -> None: + """Search GitHub for odev plugins and display the results in a table.""" + terms = self.args.plugin or "" + query = " ".join(part for part in (SEARCH_KEYWORDS, terms, SEARCH_QUALIFIERS) if part) + states = {plugin.name: plugin.state for plugin in self._discover_plugins()} + github = GithubConnector() + rows: list[list[Any]] = [] + + with progress.spinner(f"Searching GitHub for plugins matching {SEARCH_KEYWORDS} {terms}".strip()) as status: + for repository in self.__search_repositories(github, query): + status.update(f"Inspecting repository {repository.full_name!r}") + manifest = self.__remote_manifest(github, repository) + + if manifest is None: + continue + + state = states.get(repository.full_name, STATE_NOT_DOWNLOADED) + rows.append( + [ + string.link(repository.full_name, repository.html_url), + manifest["version"], + str(repository.stargazers_count), + string.stylize(state, STATE_STYLES[state]), + self.__shorten(manifest["description"] or repository.description or ""), + ] + ) + + if not rows: + raise self.error(f"No odev plugin found matching {terms!r}" if terms else "No odev plugin found on GitHub") + + headers = [ + TableHeader("Plugin", min_width=NAME_MIN_WIDTH, style="color.purple"), + TableHeader("Version", align="right", style="repr.version"), + TableHeader("Stars", align="right"), + TableHeader("State"), + TableHeader("Description"), + ] + + self.print() + self.table(headers, rows, title=f"Plugins matching {terms!r}" if terms else "Plugins") + self.console.clear_line() + + logger.info("Run 'odev plugin --enable /' to install one of these plugins") + + def __search_repositories(self, github: GithubConnector, query: str) -> list[Repository]: + """Query the GitHub search API and discard the repositories that cannot be plugins. + + :param github: Connector to the GitHub API. + :param query: The search query, using the GitHub search syntax. + :return: The candidate repositories to inspect. + """ + try: + repositories = github.search_repositories(query, limit=self.args.limit) + except RateLimitExceededException as error: + raise self.error("GitHub API rate limit exceeded, please try again in a few minutes") from error + except GithubException as error: + raise self.error(f"Failed to search for plugins on GitHub: {error}") from error + + return [ + repository + for repository in repositories + if not repository.archived and repository.full_name not in EXCLUDED_REPOSITORIES + ] + + def __remote_manifest(self, github: GithubConnector, repository: Repository) -> Manifest | None: + """Fetch and parse the manifest of a remote repository to check whether it is an odev plugin. + + :param github: Connector to the GitHub API. + :param repository: The remote repository to inspect. + :return: The manifest of the plugin, or `None` if the repository is not an odev plugin. + """ + source = github.get_repository_file(repository, PLUGIN_MANIFEST_FILENAME) + + if source is None: + return None + + return parse_plugin_manifest(source, repository.full_name) + + # --- Listing local plugins ------------------------------------------------ + + def list_plugins(self) -> None: + """List the plugins known locally and display them in a table.""" + plugins = self._discover_plugins() + + if not plugins: + raise self.error("No plugin found locally") + + headers = [ + TableHeader("Plugin", min_width=NAME_MIN_WIDTH, style="color.purple"), + TableHeader("Version", align="right", style="repr.version"), + TableHeader("Branch", style="color.cyan"), + TableHeader("State"), + TableHeader("Depends"), + ] + rows = [ + [ + string.link(plugin.name, f"https://{GITHUB_DOMAIN}/{plugin.name}"), + plugin.version, + plugin.branch, + string.stylize(plugin.state, STATE_STYLES[plugin.state]), + ", ".join(dependency.split("/")[-1] for dependency in plugin.depends), + ] + for plugin in plugins + ] + + self.print() + self.table(headers, rows, title="Plugins") + self.console.clear_line() + + if shadowed := [plugin for plugin in plugins if plugin.state == STATE_SHADOWED]: + logger.warning( + "The following plugins are enabled but cannot be loaded as another plugin already uses their " + "module name:\n" + + string.join_bullet([f"{plugin.name} (shadowed by {plugin.shadowed_by})" for plugin in shadowed]) + ) + + # --- Showing a single plugin ---------------------------------------------- + + def show_plugins(self) -> None: + """Show detailed information about one plugin, or about all the plugins available locally.""" + if self.args.plugin: + self.__show_plugin_info(self.__resolve_plugin_name(self.args.plugin)) return - def __show_plugin_info(self, plugin_name: str): - """Show the plugin information. + for plugin in self._discover_plugins(): + self.__show_plugin_info(plugin.name) + self.console.print() - :param plugin_name: The name of the plugin to show information for. + def __show_plugin_info(self, name: str) -> None: + """Show the state of a plugin and its description if available. + + :param name: The name of the plugin, in the format `organization/repository`. """ - if plugin_name not in self.config.plugins.enabled: - logger.info(f"Plugin {plugin_name!r} is {string.stylize('disabled', 'color.red')}") - else: - plugin = self.__get_plugin(plugin_name) - plugin_git = GitConnector(plugin_name) + plugin = self.__plugin_info(name) + + if plugin is None: logger.info( - string.normalize_indent( - f""" - Plugin {plugin.name!r} is {string.stylize("enabled", "color.green")} - {string.stylize("Version:", "color.black")} {string.stylize(plugin.manifest["version"], "repr.version")} - {string.stylize("Branch:", "color.black")} {string.stylize(cast(str, plugin_git.branch), "color.cyan")} - {string.stylize("Path:", "color.black")} {plugin.path.resolve()} - """ - ) + f"Plugin {name!r} is {string.stylize(STATE_NOT_DOWNLOADED, STATE_STYLES[STATE_NOT_DOWNLOADED])}" + ) + + if "/" not in name: + logger.info("Use the full name of the plugin as '/' to look it up on GitHub") + + return + + fields = [ + ("Version", string.stylize(plugin.version, "repr.version") if plugin.version else ""), + ("Branch", string.stylize(plugin.branch, "color.cyan") if plugin.branch else ""), + ("Stars", "" if plugin.stars is None else str(plugin.stars)), + ("Path", plugin.path.as_posix() if plugin.path is not None else ""), + ("Depends", ", ".join(plugin.depends)), + ("URL", f"https://{GITHUB_DOMAIN}/{plugin.name}"), + ] + width = max(len(label) for label, value in fields if value) + 1 + details = [f"Plugin {plugin.name!r} is {string.stylize(plugin.state, STATE_STYLES[plugin.state])}"] + details.extend( + f"{string.stylize(f'{label}:'.ljust(width), 'color.black')} {value}" for label, value in fields if value + ) + logger.info("\n".join(details)) + + if plugin.description: + self.console.print() + self.console.print(string.indent(plugin.description, 4).rstrip("\n")) + + self.__warn_plugin_unusable(plugin) + + def __warn_plugin_unusable(self, plugin: PluginInfo) -> None: + """Warn about the reasons a plugin cannot be loaded or installed, and tell how to install it otherwise. + + :param plugin: The plugin the information of which is being displayed. + """ + if plugin.state == STATE_SHADOWED: + logger.warning(f"Plugin {plugin.name!r} is shadowed by {plugin.shadowed_by!r} and cannot be loaded") + + if plugin.archived: + logger.warning(f"Repository {plugin.name!r} is archived and is not maintained anymore") + + if plugin.name in EXCLUDED_REPOSITORIES: + logger.warning( + f"Repository {plugin.name!r} is a template used to create new plugins and cannot be installed" ) + elif plugin.state == STATE_NOT_DOWNLOADED: + logger.info(f"Run 'odev plugin --enable {plugin.name}' to install this plugin") + + def __plugin_info(self, name: str) -> PluginInfo | None: + """Gather everything known about a plugin, completing local information with GitHub when needed. + + Plugins available locally are never looked up on GitHub, so this stays offline for the common case. + + :param name: The name of the plugin, in the format `organization/repository`. + :return: The plugin, or `None` if nothing is known about it. + """ + plugin = next((known for known in self._discover_plugins() if known.name == name), None) + + if (plugin is not None and plugin.version) or "/" not in name: + return plugin - if plugin.manifest["description"]: - self.console.print() - self.console.print(string.indent(cast(str, plugin.manifest["description"]), 4).rstrip("\n")) + remote = self.__fetch_remote_plugin(name) - def __get_plugin(self, name: str) -> Plugin: - """Find a plugin by its name.""" - plugin = next((plugin for plugin in self.odev.plugins if plugin[0] == name), None) + if remote is None: + return plugin if plugin is None: - raise self.error(f"Plugin {name!r} not found or could not be loaded") + return remote + + return replace( + plugin, + version=remote.version, + branch=plugin.branch or remote.branch, + depends=remote.depends, + description=remote.description, + stars=remote.stars, + archived=remote.archived, + ) + + def __fetch_remote_plugin(self, name: str) -> PluginInfo | None: + """Fetch the details of a plugin from GitHub, without cloning it. + + Failing to reach GitHub is not an error: the plugin is then only described by what is known locally. + + :param name: The name of the plugin, in the format `organization/repository`. + :return: The plugin as published on GitHub, or `None` if it could not be fetched. + """ + try: + with progress.spinner(f"Fetching plugin {name!r} from GitHub"): + github = GithubConnector() + repository = github.get_repository(name) + + if repository is None: + return None + + manifest = self.__remote_manifest(github, repository) + except (ConnectorError, GithubException) as error: + logger.debug(f"Could not fetch plugin {name!r} from GitHub: {error}") + return None + + if manifest is None: + logger.debug(f"Repository {name!r} does not expose a valid plugin manifest") + return None + + return PluginInfo( + name=repository.full_name, + state=STATE_NOT_DOWNLOADED, + version=manifest["version"], + branch=repository.default_branch, + depends=list(manifest["depends"]), + description=manifest["description"] or repository.description or "", + stars=repository.stargazers_count, + archived=repository.archived, + ) + + # --- Discovering local plugins -------------------------------------------- + + def _discover_plugins(self) -> list[PluginInfo]: + """List all the plugins known locally, whether they are enabled, disabled or broken. + + :return: The plugins known locally, sorted by state then by name. + """ + if self.__local_plugins is None: + plugins = self.__discover_enabled() + plugins.update(self.__discover_configured(plugins)) + plugins.update(self.__discover_downloaded(plugins)) + + self.__local_plugins = sorted( + plugins.values(), + key=lambda plugin: (STATE_ORDER.index(plugin.state), plugin.name), + ) + + return self.__local_plugins + + def __discover_enabled(self) -> dict[str, PluginInfo]: + """List the plugins linked under the plugins directory and loaded by the framework. + + :return: The enabled plugins, mapped by name. + """ + plugins: dict[str, PluginInfo] = {} + + for plugin in self.odev.plugins: + resolved_path = plugin.path.resolve() + + if plugin.path.name.startswith((".", "_")) or not resolved_path.is_dir(): + continue + + plugins[plugin.name] = PluginInfo( + name=plugin.name, + state=STATE_ENABLED, + version=plugin.manifest["version"], + branch=self.__repository_branch(resolved_path), + path=resolved_path, + depends=list(plugin.manifest["depends"]), + description=(plugin.manifest["description"] or "").strip(), + ) + + return plugins + + def __discover_configured(self, enabled: Mapping[str, PluginInfo]) -> dict[str, PluginInfo]: + """List the plugins enabled in the configuration that the framework could not load. + + :param enabled: The plugins already discovered as enabled, mapped by name. + :return: The plugins whose link is either missing or taken by another plugin, mapped by name. + """ + modules = {plugin_module_name(name): name for name in enabled} + plugins: dict[str, PluginInfo] = {} - return plugin + for name in self.config.plugins.enabled: + if name in enabled: + continue + + shadowed_by = modules.get(plugin_module_name(name), "") + plugins[name] = self.__build_plugin( + name, + self.config.paths.repositories / name, + STATE_SHADOWED if shadowed_by else STATE_MISSING, + shadowed_by=shadowed_by, + ) + + return plugins + + def __discover_downloaded(self, known: Mapping[str, PluginInfo]) -> dict[str, PluginInfo]: + """List the plugins cloned locally but neither enabled nor referenced in the configuration. + + Repositories that cannot be installed are left out so they are never advertised as available, but a plugin + already enabled is always reported, whatever its repository. + + :param known: The plugins already discovered, mapped by name. + :return: The plugins available locally but not enabled, mapped by name. + """ + repositories_path = self.config.paths.repositories + plugins: dict[str, PluginInfo] = {} + + if not repositories_path.is_dir(): + return plugins + + for manifest_path in sorted(repositories_path.glob(f"*/*/{PLUGIN_MANIFEST_FILENAME}")): + path = manifest_path.parent + name = f"{path.parent.name}/{path.name}" + + if name in known or name in plugins or name in EXCLUDED_REPOSITORIES: + continue + + manifest = self.__read_manifest(path, name) + + if manifest is None: + continue + + plugins[name] = self.__build_plugin(name, path, STATE_DISABLED, manifest=manifest) + + return plugins + + def __build_plugin( + self, + name: str, + path: Path, + state: str, + manifest: Manifest | None = None, + shadowed_by: str = "", + ) -> PluginInfo: + """Build the representation of a plugin that is not loaded by the framework. + + :param name: The name of the plugin, in the format `organization/repository`. + :param path: The expected path to the local clone of the plugin. + :param state: The state of the plugin, one of the `STATE_*` constants. + :param manifest: The already parsed manifest of the plugin, read from `path` if omitted. + :param shadowed_by: The plugin already using the module name of this one, if any. + :return: The plugin as known locally. + """ + manifest = manifest or self.__read_manifest(path, name) + + return PluginInfo( + name=name, + state=state, + version=manifest["version"] if manifest else "", + branch=self.__repository_branch(path), + path=path if path.is_dir() else None, + depends=list(manifest["depends"]) if manifest else [], + description=manifest["description"] if manifest else "", + shadowed_by=shadowed_by, + ) + + def __read_manifest(self, path: Path, name: str) -> Manifest | None: + """Read the manifest of a plugin located at the given path, without executing it. + + :param path: The path to the local clone of the plugin. + :param name: The name of the plugin, in the format `organization/repository`. + :return: The manifest of the plugin, or `None` if it is not a valid odev plugin. + """ + manifest_path = path / PLUGIN_MANIFEST_FILENAME + + if not manifest_path.is_file(): + return None + + try: + source = manifest_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + logger.debug(f"Could not read the manifest of plugin {name!r} at {manifest_path.as_posix()}") + return None + + return parse_plugin_manifest(source, name) + + def __repository_branch(self, path: Path) -> str: + """Return the branch currently checked out in the local clone of a plugin. + + :param path: The path to the local clone of the plugin. + :return: The name of the active branch, empty if the path is not a git repository. + """ + if not (path / ".git").exists(): + return "" + + try: + return GitConnector(path.as_posix()).branch or "" + except (ConnectorError, InvalidGitRepositoryError, NoSuchPathError, ValueError): + logger.debug(f"Could not determine the branch of the repository at {path.as_posix()}") + return "" + + def __resolve_plugin_name(self, name: str) -> str: + """Resolve a plugin name given on the command line to its full `organization/repository` name. + + :param name: The name of the plugin, either fully qualified or the name of its repository only. + :return: The fully qualified name of the plugin, unchanged if it could not be resolved. + """ + if "/" in name: + return name + + candidates = [plugin.name for plugin in self._discover_plugins() if plugin.name.split("/")[-1] == name] + + if len(candidates) > 1: + raise self.error(f"Plugin name {name!r} is ambiguous, use one of:\n{string.join_bullet(candidates)}") + + return candidates[0] if candidates else name + + def __shorten(self, description: str) -> str: + """Collapse a description to a single line fitting the width of a table column. + + :param description: The description to shorten. + :return: The shortened description. + """ + return textwrap.shorten(description, DESCRIPTION_MAX_WIDTH, placeholder="...") diff --git a/odev/commands/utilities/update.py b/odev/commands/utilities/update.py index 43c140350..f21d74ee3 100644 --- a/odev/commands/utilities/update.py +++ b/odev/commands/utilities/update.py @@ -18,7 +18,7 @@ class UpdateCommand(Command): _aliases = ["u"] def run(self): - from_version = self.config.update.version + from_version = self.odev.version logger.info(f"Current version: {string.stylize(from_version, 'repr.version')}") update_mode = self.odev.config.update.mode self.odev.config.update.mode = "always" diff --git a/odev/commands/utilities/version.py b/odev/commands/utilities/version.py index 0bdfbed22..5554249c9 100644 --- a/odev/commands/utilities/version.py +++ b/odev/commands/utilities/version.py @@ -1,4 +1,3 @@ -from odev._version import __version__ from odev.common import args, string from odev.common.commands import Command from odev.common.connectors.git import GitConnector @@ -19,11 +18,11 @@ class VersionCommand(Command): def run(self): """Print the current version of the application.""" name = self.odev.name.capitalize() - version = string.stylize(self.odev.config.update.version, "repr.version") + version = string.stylize(self.odev.version, "repr.version") channel = string.stylize(f"({self.odev.release})", "color.black") logger.info(f"{name} version {version} {channel}") - if self.odev.config.update.version != __version__: + if self.odev.update_available(): logger.warning(f"A newer version is available, consider running '{self.odev.name} update'") if self.show_plugins: diff --git a/odev/common/arguments.py b/odev/common/arguments.py index 53e3052fc..141666baf 100644 --- a/odev/common/arguments.py +++ b/odev/common/arguments.py @@ -2,11 +2,9 @@ import pathlib import re +from argparse import Action, BooleanOptionalAction from collections.abc import MutableMapping -from typing import ( - Any, - Literal, -) +from typing import Any, Literal class Argument: @@ -30,7 +28,8 @@ def __init__( # noqa: PLR0913 "store_path", "store_regex", "store_eval", - ] = "store", + ] + | type[Action] = "store", **kwargs: Any, ) -> None: """Initialize the argument and converts it to a mapping that can be fed to the command's @@ -197,11 +196,56 @@ def __init__( :param kwargs: Additional keyword arguments to pass to the ArgumentParser. See: https://docs.python.org/3/library/argparse.html#quick-links-for-add-argument """ + action = kwargs.pop("action", None) + + if action is None: + # A plain flag has no value of its own: the default decides which way toggling it goes, + # and argparse infers the resulting default from the action. + action = "store_false" if default is True else "store_true" + else: + # An explicit action consumes the default itself, so it has to be forwarded. + kwargs["default"] = default + + super().__init__( + name=name, + aliases=aliases, + description=description, + action=action, + **kwargs, + ) + + +class FlagOptional(Flag): + """Flag with a three-state boolean value, registering both `--flag` and `--no-flag`.""" + + def __init__( + self, + name: str | None = None, + aliases: list[str] | None = None, + description: str | None = None, + default: bool | None = None, + **kwargs: Any, + ) -> None: + """Add a flag that can be set, unset or left alone. + + Unlike :class:`Flag`, which is either present or absent, this registers a `--no-` counterpart + for each of its aliases, so that `--flag` sets the value to True, `--no-flag` sets it to False + and omitting both leaves it at its default, `None` unless specified otherwise. Use it to tell + "the user asked for the value to be turned off" apart from "the user did not mention it". + + :param name: The name of the argument, will be used in the help command and in the command's class `args` attribute. + :param aliases: The aliases for the argument. + :param description: A description for the argument, will be displayed in the `help` command. + :param default: The value the argument takes when neither the flag nor its counterpart is present. + :param kwargs: Additional keyword arguments to pass to the ArgumentParser. + See: https://docs.python.org/3/library/argparse.html#quick-links-for-add-argument + """ super().__init__( name=name, aliases=aliases, description=description, - action="store_false" if default is True else "store_true", + action=BooleanOptionalAction, + default=default, **kwargs, ) diff --git a/odev/common/commands/git.py b/odev/common/commands/git.py index c6570509f..25d1613a8 100644 --- a/odev/common/commands/git.py +++ b/odev/common/commands/git.py @@ -29,7 +29,7 @@ def worktrees(self) -> Generator[GitWorktree, None, None]: if not worktree.path.exists(): logger.debug(f"Skipping missing worktree {worktree.name!r} at {worktree.path!s}") continue - if hasattr(self, "args") and self.args.version and worktree.name != self.args.version: + if getattr(getattr(self, "args", None), "version", None) and worktree.name != self.args.version: continue yield worktree diff --git a/odev/common/commands/odoobin.py b/odev/common/commands/odoobin.py index a6088f567..14ed7499d 100644 --- a/odev/common/commands/odoobin.py +++ b/odev/common/commands/odoobin.py @@ -168,24 +168,39 @@ def _guess_addons_paths(self) -> list[Path]: "Some additional addons paths are invalid, they will be ignored:\n" + string.join_bullet([path.as_posix() for path in invalid_paths]) ) - elif self._database.repository: - addons_paths = [GitConnector(self._database.repository.full_name).path.resolve()] + elif (repository_path := self._database_addons_path) is not None: + addons_paths = [repository_path] else: current_path = Path().resolve() addons_paths = [current_path] if self.odoobin.check_addons_path(current_path) else [] return addons_paths - def _set_addons_paths(self) -> None: - """Find additional addons paths from the database repository if any.""" - addons_paths = self._guess_addons_paths() + @property + def _database_addons_path(self) -> Path | None: + """Path to the repository linked to the database, if it exists on the filesystem. + Databases linked to a repository that cannot be found locally fall back to detecting + the repository from the current directory, so that a wrong link can be fixed by running + the command again from within the repository. + """ + if not self._database.repository: + return None + + path = GitConnector(self._database.repository.full_name).path.resolve() - globs = [path.glob(f"**/__{manifest}__.py") for path in addons_paths for manifest in ["manifest", "openerp"]] - addons_paths = [ - path.parents[1] for path in (p for g in globs for p in g) if self.odoobin.check_addons_path(path.parents[1]) - ] + if not path.is_dir(): + logger.warning( + f"Repository {self._database.repository.full_name!r} linked to database " + f"{self._database.name!r} was not found at {path.as_posix()}, " + "falling back to the current directory" + ) + return None - self.odoobin.additional_addons_paths = sorted(set(addons_paths)) + return path + + def _set_addons_paths(self) -> None: + """Find additional addons paths from the database repository if any.""" + self.odoobin.additional_addons_paths = self.odoobin.expand_addons_paths(self._guess_addons_paths()) self.odoobin.save_database_repository() def _set_odoobin_process(self, force=False) -> None: diff --git a/odev/common/commands/registry.py b/odev/common/commands/registry.py new file mode 100644 index 000000000..543b18c88 --- /dev/null +++ b/odev/common/commands/registry.py @@ -0,0 +1,318 @@ +"""Registry of the commands odev can run, resolved on demand.""" + +import json +from collections.abc import Iterator, MutableMapping +from dataclasses import asdict, dataclass, field +from importlib.util import module_from_spec, spec_from_file_location +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from odev.common.config import CONFIG_DIR +from odev.common.logging import logging + + +if TYPE_CHECKING: + from odev.common.commands.base import Command + from odev.common.odev import Odev + + +logger = logging.getLogger(__name__) + + +@dataclass(frozen=True) +class CommandSource: + """Module defining a command class, as found on disk.""" + + module: str + """Name the module is imported under.""" + + path: str + """Path to the file defining the command.""" + + command_class: str + """Name of the command class within that module.""" + + +@dataclass +class CommandEntry: + """What odev knows about a command before importing the module implementing it.""" + + name: str + """Name the command is invoked with.""" + + aliases: list[str] = field(default_factory=list) + """Alternative names the command answers to.""" + + help: str = "" + """Description of the command, as displayed by the help command.""" + + sources: list[CommandSource] = field(default_factory=list) + """Modules defining the command, in registration order: the core one first, then the plugins patching it.""" + + +class CommandRegistry(MutableMapping): + """Mapping of command names and aliases to the class implementing them. + + A command module imports everything its command needs at module level, so executing all of them only to read + their names makes every odev invocation pay for every command, plugins included. Names, aliases and help texts + are therefore cached on disk, and a command module is only executed once that command is actually requested. + """ + + def __init__(self, framework: "Odev"): + self.framework: Odev = framework + """Framework the commands are registered against.""" + + self.entries: dict[str, CommandEntry] = {} + """Known commands, by name.""" + + self.names: dict[str, str] = {} + """Name of the command each name and alias refers to.""" + + self.classes: dict[str, type[Command]] = {} + """Command classes that were imported during this run, by command name.""" + + def __repr__(self) -> str: + return f"{self.__class__.__name__}({len(self.entries)} commands, {len(self.classes)} imported)" + + # --- Mapping interface ---------------------------------------------------- + + def __getitem__(self, name: str) -> type["Command"]: + command_name = self.names[name] + + if command_name not in self.classes: + self.classes[command_name] = self.__resolve(self.entries[command_name]) + + return self.classes[command_name] + + def __setitem__(self, name: str, command_class: type["Command"]) -> None: + entry = self.entries.setdefault(command_class._name, CommandEntry(command_class._name)) + entry.aliases = list(command_class._aliases or []) + entry.help = command_class._help + self.names[name] = entry.name + self.classes[entry.name] = command_class + + def __delitem__(self, name: str) -> None: + command_name = self.names.pop(name) + + if command_name not in self.names.values(): + self.entries.pop(command_name, None) + self.classes.pop(command_name, None) + + def __iter__(self) -> Iterator[str]: + return iter(self.names) + + def __len__(self) -> int: + return len(self.names) + + def clear(self) -> None: + """Forget every registered command.""" + self.entries.clear() + self.names.clear() + self.classes.clear() + + # --- Registration --------------------------------------------------------- + + def register(self, command_class: type["Command"], module_path: Path) -> None: + """Register a command shipped with odev itself. + + :param command_class: The command class to register. + :param module_path: Path to the module defining the command class. + :raise ValueError: If another command already answers to one of its names. + """ + names = self.__names_of(command_class) + + if any(name in self.names for name in names): + raise ValueError(f"Another command {command_class._name!r} is already registered") + + logger.debug(f"Registering command {command_class._name!r}") + source = self.__source_of(command_class, module_path) + command_class.prepare_command(self.framework) + self.__store(command_class, names, source) + + def patch(self, command_class: type["Command"], module_path: Path) -> None: + """Register a command provided by a plugin, letting it patch a command of the same name. + + :param command_class: The command class provided by the plugin. + :param module_path: Path to the module defining the command class. + """ + names = self.__names_of(command_class) + registered = self[command_class._name] if command_class._name in self.names else None + source = self.__source_of(command_class, module_path) + + if registered is not None and command_class.__bases__ != registered.__bases__: + logger.debug(f"Patching command {command_class._name!r}") + command_class = self.__patched(command_class, registered) + else: + logger.debug(f"Registering command {command_class._name!r}") + + command_class.prepare_command(self.framework) + self.__store(command_class, names, source) + + def summaries(self) -> list[CommandEntry]: + """Describe every registered command without importing any of them. + + :return: The known commands, sorted by name. + :rtype: List[CommandEntry] + """ + return sorted(self.entries.values(), key=lambda entry: entry.name) + + # --- On-disk index -------------------------------------------------------- + + @property + def index_path(self) -> Path: + """Path to the file caching what odev knows about its commands.""" + return CONFIG_DIR / f"{self.framework.name}-commands.json" + + def load(self, fingerprint: Any) -> bool: + """Restore the command index cached by a previous run. + + :param fingerprint: Signature of the command sources, the index is discarded when it does not match. + :return: Whether the index could be restored. + :rtype: bool + """ + try: + with self.index_path.open(encoding="utf-8") as index: + cached = json.load(index) + except (OSError, json.JSONDecodeError): + return False + + if cached.get("fingerprint") != fingerprint: + logger.debug("Command index is out of date, commands will be imported again") + return False + + self.clear() + + for name, entry in cached["commands"].items(): + self.entries[name] = CommandEntry( + name=name, + aliases=entry["aliases"], + help=entry["help"], + sources=[CommandSource(**source) for source in entry["sources"]], + ) + + for alias in [name, *entry["aliases"]]: + self.names[alias] = name + + logger.debug(f"Loaded {len(self.entries)} commands from the index") + + return True + + def save(self, fingerprint: Any) -> None: + """Cache what odev knows about its commands so the next runs do not have to import them. + + :param fingerprint: Signature of the command sources this index was built from. + """ + index = { + "fingerprint": fingerprint, + "commands": { + entry.name: { + "aliases": entry.aliases, + "help": entry.help, + "sources": [asdict(source) for source in entry.sources], + } + for entry in self.entries.values() + }, + } + + try: + self.index_path.parent.mkdir(parents=True, exist_ok=True) + + with self.index_path.open("w", encoding="utf-8") as file: + json.dump(index, file) + except OSError as error: + logger.debug(f"Failed to cache the command index: {error}") + + # --- Private methods ------------------------------------------------------ + + def __store(self, command_class: type["Command"], names: list[str], source: CommandSource) -> None: + """Record a prepared command class and the module it came from. + + :param command_class: The command class to record. + :param names: Names and aliases the command answers to. + :param source: Module the command class was defined in, before any patching. + """ + entry = self.entries.setdefault(command_class._name, CommandEntry(command_class._name)) + entry.aliases = list(command_class._aliases or []) + entry.help = command_class._help + + if source not in entry.sources: + entry.sources.append(source) + + for name in names: + self.names[name] = entry.name + + self.classes[entry.name] = command_class + + def __resolve(self, entry: CommandEntry) -> type["Command"]: + """Import the modules defining a command and rebuild the class that was registered for it. + + Replaying the sources in the order they were registered in reproduces the patching a plugin applied to a + command of the same name, without having imported any of the commands that were not asked for. + + :param entry: The command to resolve. + :return: The command class to run. + :rtype: Type[Command] + """ + resolved: type[Command] | None = None + + for source in entry.sources: + command_class = self.__import(source) + + if resolved is not None and command_class.__bases__ != resolved.__bases__: + command_class = self.__patched(command_class, resolved) + + command_class.prepare_command(self.framework) + resolved = command_class + + if resolved is None: + raise ValueError(f"Command {entry.name!r} has no module to import") + + return resolved + + def __import(self, source: CommandSource) -> type["Command"]: + """Import the module defining a command and return its class. + + :param source: The module to import. + :return: The command class it defines. + :rtype: Type[Command] + """ + spec = spec_from_file_location(source.module, source.path) + + if spec is None or spec.loader is None: + raise ImportError(f"Cannot load module {source.module} from {source.path}") + + module = module_from_spec(spec) + spec.loader.exec_module(module) + + return getattr(module, source.command_class) + + @staticmethod + def __patched(command_class: type["Command"], registered: type["Command"]) -> type["Command"]: + """Combine a command provided by a plugin with the command it patches. + + :param command_class: The command class provided by the plugin. + :param registered: The command class already registered under the same name. + :return: A class inheriting from both. + :rtype: Type[Command] + """ + + class PatchedCommand(command_class, registered, *registered.__bases__): # type: ignore [misc, valid-type] + pass + + PatchedCommand.__name__ = registered.__name__ + + return PatchedCommand + + @staticmethod + def __names_of(command_class: type["Command"]) -> list[str]: + """List the names and aliases a command answers to.""" + return [command_class._name, *(command_class._aliases or [])] + + @staticmethod + def __source_of(command_class: type["Command"], module_path: Path) -> CommandSource: + """Describe where a command class is defined, so that it can be imported again later.""" + return CommandSource( + module=command_class.__module__, + path=module_path.as_posix(), + command_class=command_class.__name__, + ) diff --git a/odev/common/config.py b/odev/common/config.py index 5f7384cd9..c2512735f 100644 --- a/odev/common/config.py +++ b/odev/common/config.py @@ -19,6 +19,8 @@ CONFIG_DIR: Path = Path.home() / ".config" / "odev" DATETIME_FORMAT = "%Y-%m-%d %H:%M:%S" +XGRAM_UNKNOWN = "" +"""Marker stored in the configuration while the user's trigram has never been resolved.""" class Section: @@ -299,6 +301,23 @@ def enabled(self, value: bool): self.set("enabled", "true" if value else "false") +class UserSection(Section): + """Configuration about the developer running odev.""" + + @property + def xgram(self) -> str: + """Odoo trigram of the current user, cached across runs. + + Resolving it requires a vault lookup and a call to git, which is too expensive to repeat on every command. + An empty value means the user is known not to be an Odoo employee, `` that the check never ran. + """ + return cast(str, self.get("xgram", XGRAM_UNKNOWN)) + + @xgram.setter + def xgram(self, value: str): + self.set("xgram", value) + + class Config: """Odev configuration. Light wrapper around configparser to write and retrieve configuration values saved on disk. diff --git a/odev/common/connectors/__init__.py b/odev/common/connectors/__init__.py index 3341a3f0b..bd0f6cef2 100644 --- a/odev/common/connectors/__init__.py +++ b/odev/common/connectors/__init__.py @@ -1,17 +1,40 @@ """Connectors to external services.""" -from .base import Connector -from .git import GitConnector, GitWorktree, Stash -from .postgres import PostgresConnector -from .rest import RestConnector -from .rpc import RpcConnector +from typing import TYPE_CHECKING + +from odev.common.lazy import lazy_exports + + +# Declared for type checkers and IDEs only: at runtime the names below are resolved by `__getattr__`, so that +# importing this package does not import every connector it exposes. +if TYPE_CHECKING: + from odev.common.connectors.base import Connector + from odev.common.connectors.git import GitConnector, GithubConnector, GitWorktree, Stash + from odev.common.connectors.postgres import PostgresConnector + from odev.common.connectors.rest import RestConnector + from odev.common.connectors.rpc import RpcConnector __all__ = [ "Connector", "GitConnector", "GitWorktree", + "GithubConnector", "PostgresConnector", "RestConnector", "RpcConnector", "Stash", ] + +__getattr__ = lazy_exports( + __name__, + { + "Connector": "base", + "GitConnector": "git", + "GithubConnector": "git", + "GitWorktree": "git", + "Stash": "git", + "PostgresConnector": "postgres", + "RestConnector": "rest", + "RpcConnector": "rpc", + }, +) diff --git a/odev/common/connectors/git.py b/odev/common/connectors/git.py index 3f0011d28..67c91cc25 100644 --- a/odev/common/connectors/git.py +++ b/odev/common/connectors/git.py @@ -3,16 +3,17 @@ import re import shutil from collections.abc import Callable, Generator, Mapping, Sequence +from itertools import islice from pathlib import Path from types import FrameType from typing import ( + TYPE_CHECKING, ClassVar, cast, ) from urllib.parse import urlparse from git import GitCommandError, InvalidGitRepositoryError, NoSuchPathError, Remote, RemoteReference, Repo -from github import Auth as GithubAuth, Github, GithubException from odev.common import bash, progress, string from odev.common.connectors.base import Connector @@ -23,6 +24,11 @@ from odev.common.signal_handling import capture_signals +if TYPE_CHECKING: + from github import Github + from github.Repository import Repository + + GITHUB_DOMAIN = "github.com" """The domain of the GitHub API.""" @@ -32,6 +38,9 @@ GIT_EXPECTED_REPO_PARTS = 2 """The expected number of parts in a git repository name (organization/repository).""" +GITHUB_SEARCH_DEFAULT_LIMIT = 20 +"""The default maximum number of repositories returned by a search on the GitHub API.""" + # Git progress opcodes, used for progress reporting GIT_OPCODE_DOWNLOAD = 33 GIT_OPCODE_RESOLVE = 65 @@ -230,15 +239,164 @@ def pending_changes(self) -> tuple[int, int]: return commits_behind, commits_ahead -class GitConnector(Connector): - """A class for connecting to the Github API.""" +class GithubConnector(Connector): + """A class for connecting to the Github API, without any repository context.""" _token: str | None = None """The Github API token for the current session.""" - _connection: Github | None = None + _connection: "Github | None" = None """The connection to the Github API.""" + @property + def url(self) -> str: + """The URL to the Github API.""" + return f"https://api.{GITHUB_DOMAIN}" + + @property + def authenticated(self) -> bool: + """Whether the current session is authenticated.""" + if self._connection is None: + return False + + from github import GithubException # noqa: PLC0415 - importing the GitHub API client is expensive + + try: + self._connection.get_user().login # noqa: B018 - login is a property + except GithubException: + return False + else: + return True + + def connect(self): + """Connect to the Github API.""" + from github import Auth as GithubAuth, Github # noqa: PLC0415 - importing the GitHub API client is expensive + + if self._token is None: + + def get_token(prompt: bool) -> str | None: + return self.store.secrets.get( + GITHUB_DOMAIN, + scope="api", + fields=["password"], + prompt_format="GitHub API token:", + ask_missing=prompt, + ).password + + token = get_token(prompt=False) + + if not token: + logger.info( + """Connection to your GitHub account is necessary to pursue this operation, please configure a + Personal Access Token (classic) on https://github.com/settings/tokens with the following permissions: + - repo (all) + - user: + - read:user + - user:email + """ + ) + token = get_token(prompt=True) + + self._token = token + + if not self.connected: + self._connection = Github(auth=GithubAuth.Token(self._token)) # type: ignore [assignment] + + if not self.authenticated: + logger.warning("Failed to connect to Github API, please check your token is valid") + self.store.secrets.invalidate(GITHUB_DOMAIN, scope="api") + self._disconnect() + self.connect() + return + + logger.debug("Connected to Github API") + + def disconnect(self): + """Disconnect from the Github API.""" + self._disconnect() + logger.debug("Disconnected from Github API") + + def _disconnect(self): + """Disconnect from the Github API.""" + self._token = None + del self._connection + + def search_repositories( + self, + query: str, + limit: int = GITHUB_SEARCH_DEFAULT_LIMIT, + sort: str | None = None, + order: str = "desc", + ) -> "list[Repository]": + """Search for repositories on GitHub. + + :param query: The search query, using the GitHub search syntax. + :param limit: The maximum number of repositories to return. + :param sort: How to sort the results, one of `stars`, `forks` or `updated`; defaults to best match. + :param order: The direction of the sort, one of `asc` or `desc`. + :return: The repositories matching the search query. + """ + if limit <= 0: + return [] + + with self: + results = cast("Github", self._connection).search_repositories( + query, + **({"sort": sort, "order": order} if sort else {}), + ) + + return list(islice(results, limit)) + + def get_repository(self, name: str) -> "Repository | None": + """Fetch a repository from GitHub by its full name, without cloning it. + + :param name: The full name of the repository, in the format `organization/repository`. + :return: The repository, or `None` if it does not exist or cannot be accessed. + """ + from github import GithubException, UnknownObjectException # noqa: PLC0415 - the GitHub client is expensive + + with self: + try: + return cast("Github", self._connection).get_repo(name) + except UnknownObjectException: + return None + except GithubException as error: + logger.debug(f"Failed to fetch repository {name!r}: {error}") + return None + + @staticmethod + def get_repository_file(repository: "Repository", path: str, ref: str | None = None) -> str | None: + """Fetch the content of a file inside a remote repository, without cloning it. + + :param repository: The remote repository to fetch the file from. + :param path: The path to the file, relative to the root of the repository. + :param ref: The branch, tag or commit to fetch the file from; defaults to the default branch. + :return: The decoded content of the file, or `None` if it does not exist or cannot be read. + """ + from github import GithubException, UnknownObjectException # noqa: PLC0415 - the GitHub client is expensive + + try: + contents = repository.get_contents(path, **({"ref": ref} if ref else {})) + except UnknownObjectException: + return None + except GithubException as error: + logger.debug(f"Failed to fetch {path!r} from repository {repository.full_name!r}: {error}") + return None + + if isinstance(contents, list): + return None + + try: + return contents.decoded_content.decode("utf-8") + except (AssertionError, UnicodeDecodeError): + # Contents are not base64-encoded for files bigger than 1 MB, which `decoded_content` asserts + logger.debug(f"Could not decode {path!r} from repository {repository.full_name!r}") + return None + + +class GitConnector(GithubConnector): + """A class for connecting to a git repository hosted on GitHub.""" + _organization: str """The organization to which the repository belongs.""" @@ -260,38 +418,84 @@ def __init__(self, repo: str, path: Path | None = None): path = Path(repo) self._path: Path | None = path + name: tuple[str, str] | None = None - if path and path.joinpath(".git").exists(): - repo_url = Repo(path).remote().url - self._organization, self._repository = repo_url.removesuffix(".git").split("/")[-2:] + if path is not None and path.joinpath(".git").exists(): + name = self._name_from_remote(path) - if ":" in self._organization: - self._organization = self._organization.split(":")[-1] - else: - if "@" in repo and ":" in repo: - # Assume the repo is in the format git@github.com:organization/repository.git - repo = repo.split(":")[-1] - - repo = urlparse(repo).path.removeprefix("/").removesuffix(".git") - repo_values = repo.split("/") - - if len(repo_values) != GIT_EXPECTED_REPO_PARTS: - raise ConnectorError( - "Invalid repository format: expected a valid git URL or repository name in one of the formats:\n" - + string.join_bullet( - [ - string.stylize(url, "color.purple") - for url in ( - "organization/repository", - "https://github.com/organization/repository", - "git@github.com:organization/repository.git", - ) - ], - ), - self, - ) + self._organization, self._repository = name or self._name_from_string(self._fallback_name(repo, path)) + + @staticmethod + def _fallback_name(repo: str, path: Path | None) -> str: + """Best guess of the repository name when it cannot be read from a git remote. + :param repo: The repository as passed to the connector. + :param path: The path to the repository, if any. + :return: The repository name in the format `organization/repository`. + """ + if path is not None and (not repo or Path(repo).is_absolute()): + return f"{path.parent.name}/{path.name}" + + return repo + + @classmethod + def _name_from_remote(cls, path: Path) -> tuple[str, str] | None: + """Read the organization and repository names from the remote of a local git repository. + The remote is the only reliable source of truth: the directory a repository is cloned to + may not follow the `//` convention. + :param path: The path to the local repository. + :return: The organization and repository names, or None if they cannot be determined. + """ + try: + remotes = list(Repo(path).remotes) + except (GitCommandError, InvalidGitRepositoryError, NoSuchPathError, ValueError) as error: + logger.debug(f"Could not read the git repository at {path.as_posix()}: {error}") + return None + + remote = next((remote for remote in remotes if remote.name == "origin"), None) or next(iter(remotes), None) + + if remote is None: + logger.debug(f"No git remote configured for the repository at {path.as_posix()}") + return None + + parts = remote.url.removesuffix(".git").rstrip("/").split("/") + + if len(parts) < GIT_EXPECTED_REPO_PARTS: + logger.debug(f"Unexpected git remote URL {remote.url!r} for the repository at {path.as_posix()}") + return None + + organization, repository = parts[-2:] + return organization.split(":")[-1], repository + + def _name_from_string(self, repo: str) -> tuple[str, str]: + """Parse the organization and repository names out of a repository name or git URL. + :param repo: The repository in the format `organization/repository` or a valid git URL. + :return: The organization and repository names. + """ + if "@" in repo and ":" in repo: + # Assume the repo is in the format git@github.com:organization/repository.git + repo = repo.split(":")[-1] + + repo = urlparse(repo).path.removeprefix("/").removesuffix(".git") + repo_values = repo.split("/") + + if len(repo_values) != GIT_EXPECTED_REPO_PARTS: + raise ConnectorError( + "Invalid repository format: expected a valid git URL or repository name in one of the formats:\n" + + string.join_bullet( + [ + string.stylize(url, "color.purple") + for url in ( + "organization/repository", + "https://github.com/organization/repository", + "git@github.com:organization/repository.git", + ) + ], + ), + self, + ) - self._organization, self._repository = repo_values + organization, repository = repo_values + return organization, repository def __repr__(self) -> str: return f"GitConnector({self.name!r})" @@ -373,7 +577,7 @@ def default_branch(self) -> str | None: return self.repository.heads[0].name.split("/")[-1] with self: - return cast(Github, self._connection).get_repo(self.name).default_branch + return cast("Github", self._connection).get_repo(self.name).default_branch @property def branch(self) -> str | None: @@ -398,19 +602,6 @@ def requirements_path(self) -> Path: """Path to the requirements.txt path of the repo, if present.""" return self.path / "requirements.txt" - @property - def authenticated(self) -> bool: - """Whether the current session is authenticated.""" - if self._connection is None: - return False - - try: - self._connection.get_user().login # noqa: B018 - login is a property - except GithubException: - return False - else: - return True - @property def worktrees_path(self) -> Path: """Path to the worktrees directory.""" @@ -424,57 +615,6 @@ def update(self): self.fetch() self.fetch_worktrees() - def connect(self): - """Connect to the Github API.""" - if self._token is None: - - def get_token(prompt: bool) -> str | None: - return self.store.secrets.get( - GITHUB_DOMAIN, - scope="api", - fields=["password"], - prompt_format="GitHub API token:", - ask_missing=prompt, - ).password - - token = get_token(prompt=False) - - if not token: - logger.info( - """Connection to your GitHub account is necessary to pursue this operation, please configure a - Personal Access Token (classic) on https://github.com/settings/tokens with the following permissions: - - repo (all) - - user: - - read:user - - user:email - """ - ) - token = get_token(prompt=True) - - self._token = token - - if not self.connected: - self._connection = Github(auth=GithubAuth.Token(self._token)) # type: ignore [assignment] - - if not self.authenticated: - logger.warning("Failed to connect to Github API, please check your token is valid") - self.store.secrets.invalidate(GITHUB_DOMAIN, scope="api") - self._disconnect() - self.connect() - return - - logger.debug("Connected to Github API") - - def disconnect(self): - """Disconnect from the Github API.""" - self._disconnect() - logger.debug("Disconnected from Github API") - - def _disconnect(self): - """Disconnect from the Github API.""" - self._token = None - del self._connection - def _check_repository(self, force_clone: bool = False): """Check whether the repository exists locally.""" if self.repository is None: @@ -996,7 +1136,7 @@ def list_remote_branches(self) -> list[str]: :rtype: List[str] """ with self: - branches = cast(Github, self._connection).get_repo(self.name).get_branches() + branches = cast("Github", self._connection).get_repo(self.name).get_branches() return [branch.name for branch in branches] diff --git a/odev/common/connectors/postgres.py b/odev/common/connectors/postgres.py index bded4f087..aa2e68d80 100644 --- a/odev/common/connectors/postgres.py +++ b/odev/common/connectors/postgres.py @@ -314,27 +314,31 @@ def create_table(self, table: str, columns: Mapping[str, str]) -> bool: return bool(self.query(f"CREATE TABLE IF NOT EXISTS {table} ({sql_columns})")) def columns_exist(self, table: str, columns: list[str]) -> list[str]: - """Check whether a column exists in a table. + """List the columns missing from a table, among the ones requested. :param table: The name of the table to check. - :param columns: The name of the column to check. - :return: Whether the column exists. + :param columns: The names of the columns to look for. + :return: The requested columns that do not exist in the table, in the order they were requested. :rtype: list[str] """ if not isinstance(columns, list): raise TypeError("Columns should be a list of strings") + if not columns: + return [] + results = self.query( f""" SELECT column_name FROM information_schema.columns - WHERE table_name = '{table}' AND column_name IN ({",".join([f"'{c}'" for c in columns])}) + WHERE table_name = '{table}' AND column_name IN ({",".join([f"'{column}'" for column in columns])}) """, ) - if results and isinstance(results, list) and columns: - return [c for c in columns if c not in [r[0] for r in results]] - return [] + # An empty result means none of the requested columns exist, not that none are missing. + existing = {result[0] for result in results} if isinstance(results, list) else set() + + return [column for column in columns if column not in existing] def create_column(self, table: str, column: str, attributes: str) -> bool: """Create a column in a table. diff --git a/odev/common/connectors/rpc.py b/odev/common/connectors/rpc.py index 1dbd7b2d8..610df3b9d 100644 --- a/odev/common/connectors/rpc.py +++ b/odev/common/connectors/rpc.py @@ -10,7 +10,6 @@ ) from urllib.parse import urlparse -import black import odoolib # type: ignore [import] from odev.common import string @@ -374,6 +373,9 @@ def _format_key_value(key: str, value: Any) -> str: call += f".with_context({_context})" call += f".{args[4]}({', '.join(filter(None, [_args, _kwargs]))})" + + import black # noqa: PLC0415 - only needed to pretty-print calls in debug mode + call = black.format_str(call, mode=black.FileMode(line_length=120)).rstrip() logger.debug(f"RPC call to {self.database.platform.display} database {self.database.name!r}") console.code(string.indent(call, indent=4), "python") diff --git a/odev/common/console.py b/odev/common/console.py index c8987bc52..0fb79c783 100644 --- a/odev/common/console.py +++ b/odev/common/console.py @@ -6,19 +6,15 @@ from collections.abc import Sequence from contextlib import contextmanager from dataclasses import dataclass +from functools import cache from pathlib import Path from typing import ( + TYPE_CHECKING, Any, ClassVar, Literal, ) -from InquirerPy import inquirer -from InquirerPy.base.control import Choice -from InquirerPy.base.simple import BaseSimplePrompt -from InquirerPy.utils import get_style -from InquirerPy.validator import EmptyInputValidator, NumberValidator, PathValidator -from prompt_toolkit.validation import ValidationError from rich import box from rich.console import Console as RichConsole, RenderableType from rich.control import Control @@ -32,6 +28,11 @@ from odev.common.deprecation import deprecated +if TYPE_CHECKING: + from InquirerPy.utils import InquirerPyStyle + from InquirerPy.validator import PathValidator + + __all__ = ["Colors", "console"] @@ -173,19 +174,29 @@ class Colors: INQUIRER_MARK = "[?]" -INQUIRER_STYLE = get_style( - style_override=False, - style={ - "questionmark": f"fg:{Colors.PURPLE} bold", - "answermark": f"fg:{Colors.PURPLE} bold", - "answer": Colors.PURPLE, - "input": Colors.CYAN, - "pointer": Colors.CYAN, - "validator": f"fg:{Colors.RED} bg: bold", - "skipped": Colors.GRAY, - "checkbox": Colors.CYAN, - }, -) + +@cache +def inquirer_style() -> "InquirerPyStyle": + """Style applied to every prompt shown to the user. + + Building it requires InquirerPy, which pulls in prompt_toolkit and is by far the most expensive dependency of + this module. Since odev only prompts in interactive sessions, it is imported on first use rather than on import. + """ + from InquirerPy.utils import get_style # noqa: PLC0415 - importing prompt_toolkit is expensive + + return get_style( + style_override=False, + style={ + "questionmark": f"fg:{Colors.PURPLE} bold", + "answermark": f"fg:{Colors.PURPLE} bold", + "answer": Colors.PURPLE, + "input": Colors.CYAN, + "pointer": Colors.CYAN, + "validator": f"fg:{Colors.RED} bg: bold", + "skipped": Colors.GRAY, + "checkbox": Colors.CYAN, + }, + ) # --- Logging highlighter customization ---------------------------------------- @@ -211,24 +222,36 @@ def __init__(self, *args, **kwargs): # Validators for inquirer prompts. -class PurportedPathValidator(PathValidator): - """Path validator that doesn't check if the path exists.""" +@cache +def purported_path_validator() -> type["PathValidator"]: + """Build the validator accepting paths that do not exist yet. - def validate(self, document) -> None: - """Check if user input is a valid path.""" - path = Path(document.text).expanduser() + It derives from an InquirerPy class, so it can only be declared once InquirerPy has been imported, which this + module defers until the user is actually prompted. + """ + from InquirerPy.validator import PathValidator # noqa: PLC0415 - importing prompt_toolkit is expensive + from prompt_toolkit.validation import ValidationError # noqa: PLC0415 - if self._is_file and path.is_dir(): - raise ValidationError( - message=self._message, - cursor_position=document.cursor_position, - ) + class PurportedPathValidator(PathValidator): + """Path validator that doesn't check if the path exists.""" - if self._is_dir and path.is_file(): - raise ValidationError( - message=self._message, - cursor_position=document.cursor_position, - ) + def validate(self, document) -> None: + """Check if user input is a valid path.""" + path = Path(document.text).expanduser() + + if self._is_file and path.is_dir(): + raise ValidationError( + message=self._message, + cursor_position=document.cursor_position, + ) + + if self._is_dir and path.is_file(): + raise ValidationError( + message=self._message, + cursor_position=document.cursor_position, + ) + + return PurportedPathValidator # --- Rich console ------------------------------------------------------------- @@ -392,6 +415,20 @@ def print( else: super().print(renderable, *args, **kwargs) + def title_rule(self, title: str) -> None: + """Print a left-aligned rule introducing a section of output. + + :param title: Text to display inside the rule. + """ + rule_char: str = "─" + + self.rule( + f"{rule_char} {string.stylize(title, 'bold color.cyan')}", + align="left", + style="", + characters=rule_char, + ) + def table( self, headers: Sequence[TableHeader], @@ -408,13 +445,7 @@ def table( :param kwargs: Additional keyword arguments to pass to the Rich Table. """ if title is not None: - rule_char: str = "─" - self.rule( - f"{rule_char} {string.stylize(title, 'bold color.cyan')}", - align="left", - style="", - characters=rule_char, - ) + self.title_rule(title) return self.table(headers, rows, totals, show_header=any(header.title for header in headers), box=None) kwargs.setdefault("show_header", True) @@ -464,17 +495,19 @@ def code(self, text: str, language: str = "python", file: Path | None = None, ** kwargs.setdefault("theme", "github-dark") self.print(Syntax(text, language, **kwargs)) - def __prompt_factory(self, prompt_type: type[BaseSimplePrompt], message: str, **kwargs) -> Any: + def __prompt_factory(self, prompt_name: str, message: str, **kwargs) -> Any: """Create a prompt object. - :param prompt_type: Type of prompt to create. + :param prompt_name: Name of the InquirerPy prompt to create. :param message: Prompt message. :param kwargs: Keyword arguments to pass to the prompt constructor. :return: The result of the prompt. """ + from InquirerPy import inquirer # noqa: PLC0415 - importing prompt_toolkit is expensive + self.pause_live() - prompt = prompt_type( + prompt = getattr(inquirer, prompt_name)( raise_keyboard_interrupt=True, - style=INQUIRER_STYLE, + style=inquirer_style(), amark=INQUIRER_MARK, qmark=INQUIRER_MARK, message=message, @@ -484,7 +517,7 @@ def __prompt_factory(self, prompt_type: type[BaseSimplePrompt], message: str, ** def patched_run(): if self.bypass_prompt: - default_key = "defaults" if prompt_type == "checkbox" else "default" + default_key = "defaults" if prompt_name == "checkbox" else "default" if default_key in kwargs: prompt.status = { @@ -519,8 +552,10 @@ def text(self, message: str, default: str = "") -> str: :return: The text entered by the user :rtype: str """ + from InquirerPy.validator import EmptyInputValidator # noqa: PLC0415 - importing prompt_toolkit is expensive + return self.__prompt_factory( - inquirer.text, + "text", message=message, default=default, validate=EmptyInputValidator(), @@ -542,8 +577,10 @@ def integer( :return: The selected choice :rtype: int or None """ + from InquirerPy.validator import NumberValidator # noqa: PLC0415 - importing prompt_toolkit is expensive + return self.__prompt_factory( - inquirer.number, + "number", message=message, default=default, min_allowed=min_value, @@ -569,8 +606,10 @@ def floating( :return: The selected choice :rtype: float or None """ + from InquirerPy.validator import NumberValidator # noqa: PLC0415 - importing prompt_toolkit is expensive + return self.__prompt_factory( - inquirer.number, + "number", message=message, default=default, min_allowed=min_value, @@ -590,7 +629,7 @@ def secret(self, message: str = "Password") -> str: :rtype: str """ return self.__prompt_factory( - inquirer.secret, + "secret", message=message, mandatory=True, mandatory_message="A value is required", @@ -605,7 +644,7 @@ def confirm(self, message: str, default: bool = False) -> bool: :rtype: bool """ return self.__prompt_factory( - inquirer.confirm, + "confirm", message=message, default=default, ) @@ -619,11 +658,11 @@ def directory(self, message: str, default: str | None = None) -> str | None: :rtype: str or None """ return self.__prompt_factory( - inquirer.filepath, + "filepath", message=message, default=default, only_directories=True, - validate=PurportedPathValidator(message="Path must not be a file", is_dir=True), + validate=purported_path_validator()(message="Path must not be a file", is_dir=True), ) def filepath(self, message: str, default: str | None = None) -> str | None: @@ -635,11 +674,11 @@ def filepath(self, message: str, default: str | None = None) -> str | None: :rtype: str or None """ return self.__prompt_factory( - inquirer.filepath, + "filepath", message=message, default=default, only_directories=True, - validate=PurportedPathValidator(message="Path must not be a directory", is_file=True), + validate=purported_path_validator()(message="Path must not be a directory", is_file=True), ) def select( @@ -655,8 +694,10 @@ def select( :return: The selected choice :rtype: str or None """ + from InquirerPy.base.control import Choice # noqa: PLC0415 - importing prompt_toolkit is expensive + return self.__prompt_factory( - inquirer.select, + "select", message=message, choices=[Choice(choice[0], name=choice[-1]) for choice in choices], default=default, @@ -673,10 +714,12 @@ def checkbox(self, message: str, choices: Sequence[tuple[Any, str | None]], defa :return: The selected choice :rtype: str or None """ + from InquirerPy.base.control import Choice # noqa: PLC0415 - importing prompt_toolkit is expensive + defaults = defaults or [] return self.__prompt_factory( - inquirer.checkbox, + "checkbox", message=message, choices=[Choice(choice[0], name=choice[-1], enabled=choice[0] in defaults) for choice in choices], transformer=lambda selected: string.join_and(selected) if selected else "None", @@ -693,8 +736,10 @@ def fuzzy(self, message: str, choices: Sequence[tuple[str, str | None]], default :return: The selected choice :rtype: str or None """ + from InquirerPy.base.control import Choice # noqa: PLC0415 - importing prompt_toolkit is expensive + return self.__prompt_factory( - inquirer.fuzzy, + "fuzzy", message=message, choices=[Choice(choice[0], name=choice[-1]) for choice in choices], default=default, diff --git a/odev/common/databases/local.py b/odev/common/databases/local.py index d9a732646..56f05c7a8 100644 --- a/odev/common/databases/local.py +++ b/odev/common/databases/local.py @@ -6,7 +6,7 @@ import re import shutil import tempfile -from collections.abc import Generator, Mapping +from collections.abc import Generator, Mapping, MutableMapping from datetime import datetime from functools import cached_property from pathlib import Path @@ -23,9 +23,10 @@ from zipfile import ZipFile from packaging.version import Version +from psycopg2 import OperationalError from odev.common import bash, progress, string -from odev.common.connectors import GitWorktree, PostgresConnector +from odev.common.connectors import GitConnector, GitWorktree, PostgresConnector from odev.common.databases import Branch, Database, Filestore, Repository from odev.common.databases.base import DatabaseInfoSection from odev.common.errors import OdevError @@ -60,9 +61,6 @@ class LocalDatabase(PostgresConnectorMixin, Database): """Class for manipulating PostgreSQL (local) databases.""" - connector: PostgresConnector | None = None - """The PostgreSQL connector of the database.""" - _whitelisted: bool = False """Whether the database is whitelisted and should not be removed automatically.""" @@ -101,11 +99,11 @@ def __init__(self, name: str): self.whitelisted = info is not None and info.whitelisted def __enter__(self): - self.connector = self.psql(self.name).__enter__() # type: ignore [assignment] + self._enter_connector(self.name) return self def __exit__(self, *args): - self.psql(self.name).__exit__(*args) + self._exit_connector(*args) @property def rpc_port(self): @@ -116,8 +114,20 @@ def is_odoo(self) -> bool: if not self.exists: return False - with self: - return self.table_exists("ir_module_module") + try: + with self: + return self.table_exists("ir_module_module") + + except OperationalError: + # Any process can drop a database at any time, including between the check above and this + # connection. Listing databases inspects each of them in turn and must not fail because one + # went away in the meantime; anything else is a genuine connection error. The check has to + # reach the server rather than the cache, which is what said the database was still there. + with self.psql() as psql, psql.nocache(): + if psql.database_exists(self.name): + raise + + return False @property def venv(self) -> PythonEnv: @@ -362,11 +372,36 @@ def repository(self) -> Repository | None: return self._repository @repository.setter - def repository(self, value: Repository): + def repository(self, value: Repository | None): """Set the repository of the database.""" self._repository = value + # The branch is a branch *of the repository*, keeping a cached one would persist + # the branch of the previous repository under the new one. + self._branch = None self.store.databases.set(self) + def link_repository(self, repository: str | Repository | GitConnector | None) -> Repository | None: + """Link a git repository to this database and save the link in the data store. + + The value is normalized through :class:`GitConnector`, so repository names, HTTPS and SSH + URLs and paths to local clones are all accepted and stored as `organization/repository`. + + :param repository: The repository to link, or None to unlink the current one. + :return: The linked repository, or None if it was unlinked. + :rtype: Optional[Repository] + """ + if repository is None: + self.repository = None + return None + + if not isinstance(repository, GitConnector): + repository = GitConnector( + repository.full_name if isinstance(repository, Repository) else repository, + ) + + self.repository = Repository(name=repository._repository, organization=repository._organization) + return self._repository + @property def branch(self) -> Branch | None: if self.repository is None: @@ -468,12 +503,13 @@ def drop(self) -> bool: def neutralize(self): """Neutralize the database.""" max_retries = 5 + retries = 0 with self.connector.nocache(): # Artificially wait for SQL transaction to be committed and for the process to be ready # before running the neutralize command # This is not clean but it works, I guess - while (not self.process or not self.version) and (retries := 0) < max_retries: + while (not self.process or not self.version) and retries < max_retries: retries += 1 sleep(0.2) @@ -500,24 +536,7 @@ def neutralize(self): self.process.run(["-d", self.name], subcommand="neutralize") self.console.print() - installed_modules: set[str] = set(self.installed_modules) & { - path.name for path in self.process.additional_addons_paths - } - scripts: list[Path] = [self.odev.static_path / "neutralize-pre.sql"] - - with progress.spinner(f"Looking up neutralization scripts in {len(installed_modules)} installed modules"): - for addon in self.process.additional_addons_paths: - for module in installed_modules: - neutralize_path: Path = addon / module / "data" / "neutralize.sql" - - if neutralize_path.is_file(): - scripts.append(neutralize_path) - - scripts.append(self.odev.static_path / "neutralize-post.sql") - - if self.version < NEUTRALIZE_BEFORE_ODOO_VERSION: - scripts.append(self.odev.static_path / "neutralize-post-before-15.0.sql") - + scripts = self._neutralize_scripts() tracker = progress.Progress() task = tracker.add_task(f"Running {len(scripts)} neutralization scripts", total=len(scripts)) @@ -529,6 +548,38 @@ def neutralize(self): tracker.stop() + def _neutralize_scripts(self) -> list[Path]: + """Return the neutralization scripts to run, including those shipped by the custom modules + installed in the database. + + :return: The paths to the neutralization scripts, in the order they must be run. + :rtype: List[Path] + """ + addons_paths = self.process.additional_addons_paths if self.process else [] + modules: MutableMapping[str, Path] = { + module.name: module + for path in addons_paths + if path.is_dir() + for module in path.iterdir() + if module.is_dir() + } + installed_modules: set[str] = set(self.installed_modules) & modules.keys() + scripts: list[Path] = [self.odev.static_path / "neutralize-pre.sql"] + + with progress.spinner(f"Looking up neutralization scripts in {len(installed_modules)} installed modules"): + scripts.extend( + script + for module in sorted(installed_modules) + if (script := modules[module] / "data" / "neutralize.sql").is_file() + ) + + scripts.append(self.odev.static_path / "neutralize-post.sql") + + if self.version < NEUTRALIZE_BEFORE_ODOO_VERSION: + scripts.append(self.odev.static_path / "neutralize-post-before-15.0.sql") + + return scripts + def dump(self, filestore: bool = False, path: Path | None = None) -> Path: if path is None: path = self.odev.dumps_path @@ -596,7 +647,9 @@ def signal_handler_progress( tracker.stop() - if self.connector is not None: + # The attribute holds the connector class until a block connects for the first time, so the check + # is on the type rather than on `None`, as in `drop`. + if isinstance(self.connector, PostgresConnector): self.connector.invalidate_cache() def _replace_filestore(self, source_dir: Path) -> None: diff --git a/odev/common/debug.py b/odev/common/debug.py index c61637b4e..f78feb034 100644 --- a/odev/common/debug.py +++ b/odev/common/debug.py @@ -1,11 +1,11 @@ """Shared method for debugging odev or interacting with debuggers.""" +import json +import os import subprocess -from collections.abc import Generator -from functools import lru_cache from pathlib import Path -from odev.common import bash, string +from odev.common import bash from odev.common.config import CONFIG_DIR from odev.common.logging import logging @@ -13,16 +13,19 @@ logger = logging.getLogger(__name__) -DEBUG_MODE: bool = False -"""Whether odev is currently in debug mode.""" +DEBUG_CACHE_PATH: Path = CONFIG_DIR / "debuggers.json" +"""Path to the file caching the result of the last scan for interactive debuggers.""" -@lru_cache -def find_debuggers(root: str | Path, *, follow_symlinks: bool = True) -> Generator[tuple[Path, int], None, None]: +_debuggers: list[str] | None = None +"""Calls to interactive debuggers found in odev's sources, resolved on first access.""" + + +def find_debuggers(root: str | Path, *, follow_symlinks: bool = True) -> list[tuple[Path, int]]: """Find all call to interactive debuggers in the given directory and its subdirectories. :param root: The directory to search for debugger instances. :param follow_symlinks: Whether to descend into symlinked directories found under the root. - :return: A generator of tuples containing the file path and the line number of the call to the debugger. + :return: A list of tuples containing the file path and the line number of the call to the debugger. """ if isinstance(root, str): root = Path(root) @@ -40,27 +43,113 @@ def find_debuggers(root: str | Path, *, follow_symlinks: bool = True) -> Generat except subprocess.CalledProcessError: output = "" + found: list[tuple[Path, int]] = [] + for line in output.splitlines(): file, position, _ = line.split(":", 2) - yield Path(file), int(position) + found.append((Path(file), int(position))) + + return found + + +def debuggers() -> list[str]: + """List the calls to interactive debuggers found in odev's sources and its plugins. + + :return: The locations of the calls, formatted as `path:line`. + """ + global _debuggers # noqa: PLW0603 + + if _debuggers is None: + _debuggers = _resolve_debuggers() + + return _debuggers + + +def debug_mode() -> bool: + """Whether odev's sources contain a call to an interactive debugger. + + A live display and an interactive debugger cannot share the terminal, so odev degrades spinners and progress + bars to plain log messages as soon as one is found. + """ + return bool(debuggers()) + + +def _sources() -> list[tuple[Path, bool]]: + """List the directories to scan, and whether symlinks must be followed within them. + + The repository may contain an `odev/plugins` symlink used for IDE import resolution: do not follow symlinks when + scanning the package, the installed plugins are scanned separately from their canonical location. + """ + sources: list[tuple[Path, bool]] = [(Path(__file__).parents[1], False)] + plugins_path = CONFIG_DIR / "plugins" + + if plugins_path.is_dir(): + sources.append((plugins_path, True)) + return sources -# ------------------------------------------------------------------------------ -# Find calls to interactive debuggers within odev's source code. -# The repository may contain an `odev/plugins` symlink used for IDE import resolution: do not follow symlinks when -# scanning the package, the installed plugins are scanned separately from their canonical location. -sources: list[tuple[Path, bool]] = [(Path(__file__).parents[1], False)] -plugins_path = CONFIG_DIR / "plugins" -if plugins_path.is_dir(): - sources.append((plugins_path, True)) +def _fingerprint(sources: list[tuple[Path, bool]]) -> list[float]: + """Compute a cheap signature of the sources, used to detect changes since the last scan. -debuggers = [ - f"{file.as_posix()}:{line}" - for source, follow_symlinks in sources - for file, line in find_debuggers(source, follow_symlinks=follow_symlinks) -] + Walking the trees for their modification times costs a fraction of what grepping through their content does, so + the actual scan only runs again once a Python file was added, removed or modified. + + :param sources: The directories to scan, and whether symlinks must be followed within them. + :return: The number of Python files found, and the most recent modification time among them. + """ + count: int = 0 + newest: float = 0.0 + + for root, follow_symlinks in sources: + for directory, _, filenames in os.walk(root, followlinks=follow_symlinks): + for filename in filenames: + if filename.endswith(".py"): + count += 1 + newest = max(newest, os.stat(Path(directory, filename)).st_mtime) + + return [count, newest] + + +def _resolve_debuggers() -> list[str]: + """Scan the sources for calls to interactive debuggers, reusing the cached result when they did not change.""" + sources = _sources() + fingerprint = _fingerprint(sources) + cached = _read_cache() + + if cached is not None and cached.get("fingerprint") == fingerprint: + return cached["debuggers"] + + found = [ + f"{file.as_posix()}:{line}" + for source, follow_symlinks in sources + for file, line in find_debuggers(source, follow_symlinks=follow_symlinks) + ] + + _write_cache(fingerprint, found) + + return found + + +def _read_cache() -> dict | None: + """Read the result of the last scan, or None if it is missing or unusable.""" + try: + with DEBUG_CACHE_PATH.open(encoding="utf-8") as cache: + return json.load(cache) + except (OSError, json.JSONDecodeError): + return None + + +def _write_cache(fingerprint: list[float], found: list[str]) -> None: + """Save the result of a scan so that the next runs can reuse it. + + :param fingerprint: Signature of the sources that were scanned. + :param found: The locations of the calls to interactive debuggers that were found. + """ + try: + DEBUG_CACHE_PATH.parent.mkdir(parents=True, exist_ok=True) -if debuggers: - logger.warning(f"Interactive debuggers detected:\n{string.join_bullet(debuggers)}") - DEBUG_MODE = True + with DEBUG_CACHE_PATH.open("w", encoding="utf-8") as cache: + json.dump({"fingerprint": fingerprint, "debuggers": found}, cache) + except OSError as error: + logger.debug(f"Failed to cache the scan for interactive debuggers: {error}") diff --git a/odev/common/lazy.py b/odev/common/lazy.py new file mode 100644 index 000000000..83c49a148 --- /dev/null +++ b/odev/common/lazy.py @@ -0,0 +1,36 @@ +"""Helpers to re-export names from a package without importing their module eagerly.""" + +import sys +from collections.abc import Callable, Mapping +from importlib import import_module +from typing import Any + + +__all__ = ["lazy_exports"] + + +def lazy_exports(package: str, exports: Mapping[str, str]) -> Callable[[str], Any]: + """Build the `__getattr__` of a package re-exporting names from its submodules. + + Importing a package should not drag in every module it re-exports: the connectors alone pull in a GitHub API + client, an RPC client and a database driver, none of which most commands ever touch. Names are instead resolved + the first time they are accessed, then cached on the package so subsequent accesses are plain lookups. + + :param package: Name of the package the names are re-exported from, usually its `__name__`. + :param exports: Mapping of each re-exported name to the submodule defining it, relative to the package. + :return: A function to assign to the package's `__getattr__`. + :rtype: Callable[[str], Any] + """ + + def resolve(name: str) -> Any: + module = exports.get(name) + + if module is None: + raise AttributeError(f"module {package!r} has no attribute {name!r}") + + value = getattr(import_module(f"{package}.{module}"), name) + setattr(sys.modules[package], name, value) + + return value + + return resolve diff --git a/odev/common/mixins/__init__.py b/odev/common/mixins/__init__.py index dd162ed76..19b1fa185 100644 --- a/odev/common/mixins/__init__.py +++ b/odev/common/mixins/__init__.py @@ -1,4 +1,34 @@ """Mixins to extend the functionality of command classes.""" -from .connectors import * -from .databases import ListLocalDatabasesMixin +from typing import TYPE_CHECKING + +from odev.common.lazy import lazy_exports + + +# Declared for type checkers and IDEs only: at runtime the names below are resolved by `__getattr__`, so that +# importing this package does not import the connectors the mixins wrap. +if TYPE_CHECKING: + from odev.common.mixins.connectors.base import ConnectorMixin, ensure_connected + from odev.common.mixins.connectors.github import GitConnectorMixin + from odev.common.mixins.connectors.postgres import PostgresConnectorMixin + from odev.common.mixins.databases.list import ListLocalDatabasesMixin + + +__all__ = [ + "ConnectorMixin", + "GitConnectorMixin", + "ListLocalDatabasesMixin", + "PostgresConnectorMixin", + "ensure_connected", +] + +__getattr__ = lazy_exports( + __name__, + { + "ConnectorMixin": "connectors.base", + "ensure_connected": "connectors.base", + "GitConnectorMixin": "connectors.github", + "PostgresConnectorMixin": "connectors.postgres", + "ListLocalDatabasesMixin": "databases.list", + }, +) diff --git a/odev/common/mixins/connectors/__init__.py b/odev/common/mixins/connectors/__init__.py index aee67c689..c7e25eb5c 100644 --- a/odev/common/mixins/connectors/__init__.py +++ b/odev/common/mixins/connectors/__init__.py @@ -1,8 +1,16 @@ """Connector mixins.""" -from .base import ConnectorMixin, ensure_connected -from .github import GitConnectorMixin -from .postgres import PostgresConnectorMixin +from typing import TYPE_CHECKING + +from odev.common.lazy import lazy_exports + + +# Declared for type checkers and IDEs only: at runtime the names below are resolved by `__getattr__`, so that +# importing this package does not import the connectors the mixins wrap. +if TYPE_CHECKING: + from odev.common.mixins.connectors.base import ConnectorMixin, ensure_connected + from odev.common.mixins.connectors.github import GitConnectorMixin + from odev.common.mixins.connectors.postgres import PostgresConnectorMixin __all__ = [ @@ -11,3 +19,13 @@ "PostgresConnectorMixin", "ensure_connected", ] + +__getattr__ = lazy_exports( + __name__, + { + "ConnectorMixin": "base", + "ensure_connected": "base", + "GitConnectorMixin": "github", + "PostgresConnectorMixin": "postgres", + }, +) diff --git a/odev/common/mixins/connectors/postgres.py b/odev/common/mixins/connectors/postgres.py index 2cf96c3c4..a6718e38e 100644 --- a/odev/common/mixins/connectors/postgres.py +++ b/odev/common/mixins/connectors/postgres.py @@ -10,6 +10,38 @@ class PostgresConnectorMixin(ConnectorMixin): _connector_class: type[PostgresConnector] = PostgresConnector # type: ignore [assignment] connector: PostgresConnector # type: ignore [assignment] + _connection_depth: int = 0 + """Number of nested blocks currently sharing the connector.""" + def psql(self, name: str = "postgres") -> PostgresConnector: """Return a PostgreSQL connector to the selected database.""" return self._connector_class(name) + + def _enter_connector(self, name: str) -> PostgresConnector: + """Connect to a database, or join the connection an enclosing block already opened. + + `ensure_connected` runs every database method inside its own block, and those blocks nest, so + opening a connection per block would mean a new PostgreSQL backend for each call. + + :param name: The name of the database to connect to. + :return: The connector the caller should use. + :rtype: PostgresConnector + """ + # A held connection that was closed in the meantime is replaced rather than reused: `drop` + # disconnects explicitly, and the datastore holds its own connection across such a call. At depth + # zero the check is skipped, `connector` still being the connector class rather than an instance. + if not self._connection_depth or not self.connector.connected: + # Narrows what the base mixin types as a `Connector`, deliberately: everything reached through + # this mixin talks to PostgreSQL, and its callers query rather than use the base interface. + self.connector = self.psql(name).__enter__() # pyright: ignore[reportIncompatibleVariableOverride] + + self._connection_depth += 1 + + return self.connector + + def _exit_connector(self, *args) -> None: + """Close the connection once the outermost block sharing it is done with it.""" + self._connection_depth -= 1 + + if not self._connection_depth: + self.connector.__exit__(*args) diff --git a/odev/common/odev.py b/odev/common/odev.py index c84426e36..636d5636c 100644 --- a/odev/common/odev.py +++ b/odev/common/odev.py @@ -1,5 +1,6 @@ """Self update Odev by pulling latest changes from the git repository.""" +import ast import contextlib import importlib import inspect @@ -9,9 +10,10 @@ import sys from argparse import Namespace from collections import defaultdict -from collections.abc import Generator, Iterable, Iterator, Mapping, MutableMapping, Sequence +from collections.abc import Generator, Iterable, Iterator, Mapping, Sequence from datetime import datetime from functools import lru_cache +from hashlib import sha256 from importlib.abc import Loader from importlib.machinery import FileFinder, ModuleSpec from importlib.util import module_from_spec, spec_from_file_location @@ -23,20 +25,19 @@ Any, ClassVar, Generic, - Literal, NamedTuple, TypedDict, cast, ) from git import GitCommandError, NoSuchPathError, Repo -from networkx import DiGraph, NetworkXUnfeasible, simple_cycles, topological_sort from packaging import version from odev._version import __version__ from odev.common import progress, string from odev.common.commands import CommandType from odev.common.commands.database import DatabaseType +from odev.common.commands.registry import CommandRegistry from odev.common.config import CONFIG_DIR, Config from odev.common.connectors.git import GitConnector, Stash from odev.common.console import Console, console @@ -60,7 +61,7 @@ UTC = timezone.utc -__all__ = ["Odev"] +__all__ = ["Odev", "parse_plugin_manifest", "plugin_module_name"] PRUNING_INTERVAL = 14 @@ -80,6 +81,9 @@ MIN_ARGV_LENGTH = 2 """Minimum number of command line arguments required (command and subcommand).""" +PLUGIN_MANIFEST_FILENAME = "__manifest__.py" +"""Name of the manifest file located at the root of a plugin repository.""" + class Manifest(TypedDict): """Plugin manifest information.""" @@ -101,6 +105,67 @@ class Plugin(NamedTuple): logger = logging.getLogger(__name__) +def plugin_module_name(plugin: str) -> str: + """Convert the name of a plugin to the name of the module it is linked to under the plugins directory. + + :param plugin: Name of the plugin, in the format `organization/repository` + :return: Name of the python module for this plugin + """ + return plugin.split("/")[-1].replace("-", "_") + + +def parse_plugin_manifest(source: str, name: str) -> Manifest | None: + """Extract the metadata of a plugin from the source of its manifest, without executing it. + + Only the module docstring and top-level assignments of literal values are read, which makes this function safe + to use on manifests originating from untrusted repositories. A source that does not define a top-level + `__version__` string is not considered a valid odev plugin manifest. + + :param source: Content of the `__manifest__.py` file + :param name: Name of the plugin, in the format `organization/repository` + :return: Manifest of the plugin, or `None` if the source is not a valid odev plugin manifest + """ + try: + tree = ast.parse(source) + except (SyntaxError, ValueError, RecursionError): + logger.debug(f"Failed to parse the manifest of plugin {name!r}") + return None + + assignments: dict[str, Any] = {} + + for node in tree.body: + if isinstance(node, ast.Assign): + targets, value = node.targets, node.value + elif isinstance(node, ast.AnnAssign) and node.value is not None: + targets, value = [node.target], node.value + else: + continue + + try: + literal = ast.literal_eval(value) + except (SyntaxError, TypeError, ValueError, MemoryError, RecursionError): + continue + + assignments.update({target.id: literal for target in targets if isinstance(target, ast.Name)}) + + manifest_version = assignments.get("__version__") + + if not isinstance(manifest_version, str): + logger.debug(f"Manifest of plugin {name!r} does not declare a version number") + return None + + depends = assignments.get("depends") + + return { + "name": name, + "version": manifest_version, + "description": (ast.get_docstring(tree) or "").strip(), + "depends": [dependency for dependency in depends if isinstance(dependency, str)] + if isinstance(depends, list) + else [], + } + + class Odev(Generic[CommandType]): """Main framework class.""" @@ -116,8 +181,8 @@ class Odev(Generic[CommandType]): store: ClassVar[DataStore] """Odev data storage.""" - commands: MutableMapping[str, type[CommandType]] = {} - """Collection of existing and loaded commands.""" + commands: "CommandRegistry" + """Collection of existing commands, imported on demand.""" executable: ClassVar[Path] = Path(sys.argv[0]).parent.resolve() / "odev.sh" """Path to the current executable.""" @@ -131,10 +196,11 @@ class Odev(Generic[CommandType]): _command_stack: list[CommandType] = [] """Stack of current commands being executed. Last command in list is the one currently running.""" - def __init__(self, test: bool = False): + def __init__(self, test: bool = False, name: str | None = None): """Initialize the framework. :param test: Whether the framework is being initialized for testing purposes + :param name: Namespace of the framework, overriding the one inferred from the test mode """ self.start_time = monotonic() """Time when the framework was started.""" @@ -142,6 +208,12 @@ def __init__(self, test: bool = False): self.in_test_mode = test """Whether the framework is in testing mode.""" + self._name = name + """Namespace explicitly assigned to this instance, if any.""" + + self.commands = CommandRegistry(self) + """Collection of existing commands, imported on demand.""" + self._load_config() self.__class__.store = DataStore(self.name) self.telemetry = Telemetry(self) @@ -156,8 +228,15 @@ def git(self) -> GitConnector: return GitConnector(f"{self.path.parent.name}/{self.path.name}", self.path) @property - def name(self) -> Literal["odev", "odev-test"]: - """Name of the framework.""" + def name(self) -> str: + """Name of the framework, and the namespace of everything it owns. + + The configuration file and the datastore database are both named after it, so an instance given an + explicit name works on its own resources rather than on the ones of the user. + """ + if self._name is not None: + return self._name + return "odev" if not self.in_test_mode else "odev-test" @property @@ -231,7 +310,7 @@ def dumps_path(self) -> Path: def plugins(self) -> Generator[Plugin, None, None]: """Yields enabled plugins sorted topologically.""" for plugin_name in self._plugins_dependency_tree(): - plugin_path = self.plugins_path / plugin_name.split("/")[-1].replace("-", "_") + plugin_path = self.plugins_path / plugin_module_name(plugin_name) plugin_manifest = self._load_plugin_manifest(plugin_path) yield Plugin(plugin_name, plugin_path, plugin_manifest) @@ -288,10 +367,18 @@ def start(self, start_time: float | None = None) -> None: with progress.spinner("Loading commands"): self.load_plugins() - self.register_commands() - self.register_plugin_commands() + + # Importing every command module only to read its name makes each run pay for every command, plugins + # included. Do it once and remember the outcome until the commands on disk actually change. + fingerprint = self._commands_fingerprint() + + if not self.commands.load(fingerprint): + self.register_commands() + self.register_plugin_commands() + self.commands.save(fingerprint) self.prune_databases() + self.telemetry.flush() self._started = True def update(self, restart: bool = True, upgrade: bool = False) -> bool: @@ -306,9 +393,9 @@ def update(self, restart: bool = True, upgrade: bool = False) -> bool: plugins_upgrade = any(self._update(path, plugin) for plugin, path, _ in self.plugins) updated = repo_updated or plugins_upgrade or upgrade + self.config.update.date = datetime.now() if updated: - self.config.update.date = datetime.now(UTC) self._set_version_after_update() self.upgrade() @@ -342,11 +429,15 @@ def _update(self, path: Path, plugin: str | None = None, _retry: int = 0) -> boo if git.repository is None: raise OdevError(f"Repository for {self.name!r} not found at {path.as_posix()}") + prompt_name = f"plugin {plugin}" if plugin else self.name + logger.debug(f"Checking for updates in {git.name!r}") + if not self.__git_branch_behind(git.repository): git.fetch(detached=False) - prompt_name = f"plugin {plugin}" if plugin else self.name - logger.debug(f"Checking for updates in {git.name!r}") + if not self.__git_branch_behind(git.repository): + logger.debug(f"No update available for {git.name!r}") + return False if not self.__update_prompt(prompt_name): return False @@ -437,6 +528,21 @@ def _set_version_after_update(self): spec.loader.exec_module(version_module) self.__class__.version = version_module.__version__ + def _reconcile_recorded_version(self) -> bool: + """Reset the recorded version if it is ahead of the version currently running, which happens + after switching release channel, checking out an older revision or downgrading odev. + + :return: Whether the recorded version was reset + :rtype: bool + """ + if version.parse(self.config.update.version) <= version.parse(self.version): + return False + + recorded_version = string.stylize(self.config.update.version, "repr.version") + logger.debug(f"Recorded version {recorded_version} is ahead of the current version, resetting it") + self.config.update.version = self.version + return True + def check_upgrade(self) -> bool: """Check whether the current version of odev is the latest available version. @@ -451,6 +557,9 @@ def check_upgrade(self) -> bool: def upgrade(self) -> None: """Upgrade the current version of odev.""" + if self._reconcile_recorded_version(): + return + if not self.check_upgrade(): return @@ -532,15 +641,15 @@ def list_commands(self, sources: Iterable[Path]) -> Iterator[pkgutil.ModuleInfo] command_dirs = [path for path in sources if path.is_dir() and not path.name.startswith("_")] return pkgutil.iter_modules([d.as_posix() for d in command_dirs]) - def import_commands(self, sources: Iterable[Path]) -> list[type[CommandType]]: + def import_commands(self, sources: Iterable[Path]) -> list[tuple[type[CommandType], Path]]: """Import all commands from the source directories. :param sources: Source directories to search for commands. - :return: List of imported command classes - :rtype: List[CommandType] + :return: List of imported command classes, paired with the module they were defined in + :rtype: List[Tuple[CommandType, Path]] """ command_modules = self.list_commands(sources) - command_classes: list[type[CommandType]] = [] + command_classes: list[tuple[type[CommandType], Path]] = [] for module_info in command_modules: if not isinstance(module_info.module_finder, FileFinder): @@ -558,23 +667,18 @@ def import_commands(self, sources: Iterable[Path]) -> list[type[CommandType]]: command_module: ModuleType = module_from_spec(spec) spec.loader.exec_module(command_module) - command_classes.extend(command[1] for command in inspect.getmembers(command_module, self.__filter_commands)) + command_classes.extend( + (command[1], module_path) for command in inspect.getmembers(command_module, self.__filter_commands) + ) return command_classes def register_commands(self) -> None: """Register all commands from the commands directory.""" - for command_class in self.import_commands(self.commands_path.iterdir()) + self.import_commands( + for command_class, module_path in self.import_commands(self.commands_path.iterdir()) + self.import_commands( [self.commands_path] ): - logger.debug(f"Registering command {command_class._name!r}") - command_names = [command_class._name] + (list(command_class._aliases) or []) - - if any(name in command_names for name in self.commands): - raise ValueError(f"Another command {command_class._name!r} is already registered") - - command_class.prepare_command(self) - self.commands.update(dict.fromkeys(command_names, command_class)) + self.commands.register(command_class, module_path) def load_plugins(self) -> None: """Import all enabled plugins to allow them to patch the framework.""" @@ -633,33 +737,33 @@ def _load_plugin_module(self, plugin: Plugin) -> None: :param plugin: Plugin whose module should be imported. """ # Module names MUST use underscores even if directories use dashes - plugin_module_name = plugin.path.name.replace("-", "_") - module_name = f"odev.plugins.{plugin_module_name}" + module_basename = plugin.path.name.replace("-", "_") + module_name = f"odev.plugins.{module_basename}" # Try to import directly from sys.path first try: - module = importlib.import_module(plugin_module_name) + module = importlib.import_module(module_basename) except ImportError: # Fallback to explicit file loading if direct import fails init_path = plugin.path / "__init__.py" if not init_path.exists(): return - spec = spec_from_file_location(plugin_module_name, init_path) + spec = spec_from_file_location(module_basename, init_path) if not spec or not spec.loader: return module = module_from_spec(spec) - sys.modules[plugin_module_name] = module + sys.modules[module_basename] = module try: spec.loader.exec_module(module) except Exception: # Drop the half-initialized module so a later retry starts from a clean state - sys.modules.pop(plugin_module_name, None) + sys.modules.pop(module_basename, None) raise # Ensure it's available as odev.plugins.X sys.modules[module_name] = module - setattr(sys.modules["odev.plugins"], plugin_module_name, module) + setattr(sys.modules["odev.plugins"], module_basename, module) def _install_missing_plugin_requirements(self) -> bool: """Install missing python packages from the requirements of all enabled plugins. @@ -715,31 +819,31 @@ def register_plugin_commands(self) -> None: def _register_plugin_commands(self) -> None: """Register all commands from the plugins directories.""" for plugin in self.plugins: - for command_class in self.import_commands(plugin.path.glob("commands/**")): - command_names = [command_class._name] + (list(command_class._aliases) or []) - base_command_class = self.commands.get(command_class._name) - action = ( - "Registering" - if base_command_class is None or issubclass(base_command_class, command_class) - else "Patching" - ) + for command_class, module_path in self.import_commands(plugin.path.glob("commands/**")): + self.commands.patch(command_class, module_path) - logger.debug(f"{action} command {command_class._name!r}") + def _commands_fingerprint(self) -> list[Any]: + """Compute a cheap signature of the command modules available to odev. - if ( - command_class._name in self.commands - and base_command_class is not None - and command_class.__bases__ != base_command_class.__bases__ - ): + Walking the command directories for their names and modification times costs a fraction of what importing + them does, so the commands are only discovered again once one of them was added, removed, renamed or + modified. - class PatchedCommand(command_class, base_command_class, *base_command_class.__bases__): - pass + :return: The odev version, the version of each enabled plugin, and the state of the command directories + :rtype: List[Any] + """ + modules: list[str] = [] - command_class = PatchedCommand # noqa: PLW2901 - we want to override the variable - PatchedCommand.__name__ = base_command_class.__name__ + for commands_path in [self.commands_path, *(plugin.path / "commands" for plugin in self.plugins)]: + modules.extend( + f"{module_path.as_posix()}:{module_path.stat().st_mtime}" for module_path in commands_path.rglob("*.py") + ) - command_class.prepare_command(self) - self.commands.update(dict.fromkeys(command_names, command_class)) + return [ + self.version, + {plugin.name: plugin.manifest["version"] for plugin in self.plugins}, + sha256("\n".join(sorted(modules)).encode()).hexdigest(), + ] def _load_config(self) -> None: """Reload the configuration file.""" @@ -781,7 +885,7 @@ def install_plugin(self, plugin: str, as_dependency: bool = False) -> None: for dependency in depends: self.install_plugin(dependency, as_dependency=True) - plugin_path = self.plugins_path / repository._repository.replace("-", "_") + plugin_path = self.plugins_path / plugin_module_name(repository.name) self.plugins_path.mkdir(parents=True, exist_ok=True) if self._plugin_is_installed(plugin): @@ -823,7 +927,7 @@ def uninstall_plugin(self, plugin: str) -> None: if installed_plugin == plugin: continue - installed_plugin_path = self.plugins_path / installed_plugin.split("/")[-1].replace("-", "_") + installed_plugin_path = self.plugins_path / plugin_module_name(installed_plugin) manifest = self._load_plugin_manifest(installed_plugin_path) if any(dep in manifest.get("depends", []) for dep in dependents | {plugin}): @@ -841,7 +945,7 @@ def uninstall_plugin(self, plugin: str) -> None: raise OdevError("Aborting plugin uninstallation") for dependent in dependents | {plugin}: - plugin_path = self.plugins_path / dependent.split("/")[-1].replace("-", "_") + plugin_path = self.plugins_path / plugin_module_name(dependent) plugin_path.unlink(missing_ok=True) self.config.plugins.enabled = {p for p in self.config.plugins.enabled if p != dependent} logger.info(f"Uninstalled plugin {dependent!r}") @@ -880,13 +984,15 @@ def _load_plugin_manifest(self, plugin_path: Path) -> Manifest: "depends": [], } - if not (plugin_path / "__manifest__.py").exists(): + manifest_path = plugin_path / PLUGIN_MANIFEST_FILENAME + + if not manifest_path.exists(): return defaults - spec = spec_from_file_location(f"{plugin_path.name}.__manifest__", (plugin_path / "__manifest__.py").as_posix()) + spec = spec_from_file_location(f"{plugin_path.name}.__manifest__", manifest_path.as_posix()) if spec is None: - raise ImportError(f"Cannot load manifest module from {(plugin_path / '__manifest__.py').as_posix()}") + raise ImportError(f"Cannot load manifest module from {manifest_path.as_posix()}") manifest = module_from_spec(spec) cast(Loader, spec.loader).exec_module(manifest) @@ -903,32 +1009,103 @@ def _plugins_dependency_tree(self) -> list[str]: """Order plugins by mutual dependencies, the first one in the returned list being the first one that needs to be imported to respect the dependency graph. """ - graph = DiGraph() + dependents: dict[str, list[str]] = {} for plugin_path in self.plugins_path.iterdir(): manifest = self._load_plugin_manifest(plugin_path) - graph.add_node(manifest["name"]) + dependents.setdefault(manifest["name"], []) for dependency in manifest["depends"]: - graph.add_edge(dependency, manifest["name"]) + dependents.setdefault(dependency, []).append(manifest["name"]) - try: - resolved_graph: list[str] = list(topological_sort(graph)) - logger.debug(f"Resolved plugins dependency tree:\n{join_bullet(resolved_graph)}") - except NetworkXUnfeasible as exception: - cycles = list(simple_cycles(graph))[:20] - if cycles: - parts: list[str] = [] - for c in cycles: - if len(c) == 1: - parts.append(f"{c[0]} depends on itself") - else: - parts.append(" → ".join([*c, c[0]])) - raise OdevError("Circular dependency detected in plugins: " + "; ".join(parts)) from exception - raise OdevError("Circular dependency detected in plugins") from exception + resolved_graph = self.__topological_sort(dependents) + logger.debug(f"Resolved plugins dependency tree:\n{join_bullet(resolved_graph)}") return resolved_graph + @classmethod + def __topological_sort(cls, dependents: Mapping[str, list[str]]) -> list[str]: + """Order nodes of a dependency graph so that each one comes after the nodes it depends on. + + :param dependents: Mapping of each node to the nodes that directly depend on it. + :return: The ordered nodes. + :rtype: List[str] + :raise OdevError: If the graph contains a circular dependency. + """ + indegrees = dict.fromkeys(dependents, 0) + + for node_dependents in dependents.values(): + for dependent in node_dependents: + indegrees[dependent] += 1 + + ordered: list[str] = [] + generation = [node for node, indegree in indegrees.items() if not indegree] + + while generation: + ordered.extend(generation) + next_generation: list[str] = [] + + for node in generation: + for dependent in dependents[node]: + indegrees[dependent] -= 1 + + if not indegrees[dependent]: + next_generation.append(dependent) + + generation = next_generation + + if len(ordered) == len(dependents): + return ordered + + cycles = cls.__find_cycles(dependents, set(dependents) - set(ordered)) + + if not cycles: + raise OdevError("Circular dependency detected in plugins") + + described = [ + f"{cycle[0]} depends on itself" if len(cycle) == 1 else " → ".join([*cycle, cycle[0]]) for cycle in cycles + ] + + raise OdevError("Circular dependency detected in plugins: " + "; ".join(described)) + + @staticmethod + def __find_cycles(dependents: Mapping[str, list[str]], nodes: set[str], limit: int = 20) -> list[list[str]]: + """Find the circular dependencies formed by the given nodes, for reporting purposes. + + :param dependents: Mapping of each node to the nodes that directly depend on it. + :param nodes: The nodes known to take part in a cycle. + :param limit: Maximum number of cycles to report. + :return: The cycles found, each as the list of nodes it goes through. + :rtype: List[List[str]] + """ + cycles: list[list[str]] = [] + reported: set[tuple[str, ...]] = set() + + def walk(path: list[str]) -> None: + if len(cycles) >= limit: + return + + for dependent in dependents.get(path[-1], []): + if dependent not in nodes: + continue + + if dependent not in path: + walk([*path, dependent]) + continue + + cycle = path[path.index(dependent) :] + start = cycle.index(min(cycle)) + canonical = tuple(cycle[start:] + cycle[:start]) + + if canonical not in reported: + reported.add(canonical) + cycles.append(list(canonical)) + + for node in sorted(nodes): + walk([node]) + + return cycles + def parse_arguments(self, command_cls: type[CommandType], *args) -> Namespace: """Parse arguments for a command. @@ -1015,12 +1192,11 @@ def run_command( command.cleanup() command.console.bypass_prompt = command._bypass_prompt_orig - if telemetry is not None and self.config.telemetry.enabled: - telemetry[0].join() - telemetry_line = telemetry[1].get() - - if telemetry_line is not None: - self.telemetry.update(telemetry_line) + if telemetry is not None: + telemetry.finish( + exit_code=int(command_errored), + execution_time=(monotonic() - self.start_time) / 60, + ) return not command_errored @@ -1060,6 +1236,20 @@ def check_release(self) -> None: "release channel" ) + def update_available(self) -> bool: + """Check whether newer changes are available for odev in its remote repository. + + Based on the remote tracking branch as of the last time changes were fetched by the periodic + update check, this does not reach out to the network. + + :return: Whether newer changes are available + :rtype: bool + """ + if self.git.repository is None: + return False + + return self.__git_branch_behind(self.git.repository) + def switch_release_channel(self, branch: str) -> None: """Switch the release channel to the given branch.""" with progress.spinner(f"Switching odev to {branch!r} release channel"): @@ -1070,6 +1260,8 @@ def switch_release_channel(self, branch: str) -> None: self.__checkout_release_channel(GitConnector(plugin.name), branch) self.config.update.release = branch + self._set_version_after_update() + self._reconcile_recorded_version() logger.info(f"Switched release channel to {branch!r}") # --- Private methods ------------------------------------------------------ @@ -1121,6 +1313,9 @@ def __git_branch_behind(self, repository: Repo) -> bool: :return: Whether the branch is behind the remote tracking branch :rtype: bool """ + if repository.head.is_detached: + return False + remote_branch = repository.active_branch.tracking_branch() if remote_branch is None: diff --git a/odev/common/odoobin.py b/odev/common/odoobin.py index 8b0e881d0..ef4c6c4e6 100644 --- a/odev/common/odoobin.py +++ b/odev/common/odoobin.py @@ -2,6 +2,8 @@ import re import shlex +import shutil +import sys from ast import literal_eval from collections.abc import Callable, Generator, Mapping, Sequence from contextlib import nullcontext @@ -17,7 +19,7 @@ from packaging.version import Version -from odev.common import bash, string +from odev.common import bash, string, system from odev.common.cache import TTLCache from odev.common.connectors import GitConnector, GitWorktree from odev.common.databases import Branch, Repository @@ -29,6 +31,7 @@ from odev.common.progress import spinner from odev.common.python import PythonEnv from odev.common.signal_handling import capture_signals +from odev.common.system import SystemDependency from odev.common.version import OdooVersion @@ -51,6 +54,22 @@ ODOO_UPGRADE_REPOSITORY: str = "odoo/upgrade" +SETUPTOOLS_REQUIREMENT: str = "setuptools>=69.0.0,<82" +"""Version of setuptools to install in the virtual environments of Odoo installations. +At least 69 because setuptools 58 and 59 cannot serve as a PEP 517 backend for current pip, +which breaks installing gevent and other source distributions with `--no-build-isolation`. +Below 82 because it removed `pkg_resources`, which Odoo 15.0 and 16.0 import unconditionally. +""" + +DEBIAN_INSTALL_SCRIPT: str = "setup/debinstall.sh" +"""Path of the script listing and installing Odoo's system dependencies, relative to the Odoo sources.""" + +PROBED_DEPENDENCIES: Sequence[SystemDependency] = (system.C_COMPILER, system.POSTGRESQL_HEADERS) +"""System dependencies looked for on systems where Odoo does not list its own packages. Restricted +to the ones an executable on the `PATH` proves present, so that nothing is ever reported as missing +on a distribution that just names it differently. +""" + ODOO_PYTHON_VERSIONS: Mapping[int, str] = { 19: "3.12", @@ -148,8 +167,10 @@ def __init__( self.repository: GitConnector = GitConnector("odoo/odoo") """Github repository of Odoo.""" - self._additional_addons_paths: list[Path] = [] - """List of additional addons paths to use when starting the Odoo process.""" + self._additional_addons_paths: list[Path] | None = None + """List of additional addons paths to use when starting the Odoo process. + None until derived from the repository linked to the database, or assigned explicitly. + """ self._venv: PythonEnv | None = None """Cached python virtual environment used by the Odoo installation.""" @@ -282,12 +303,14 @@ def odoo_support_repository(self) -> GitConnector: @property def additional_addons_paths(self) -> list[Path]: - """Return the list of additional addons paths.""" - if not self._additional_addons_paths and self.database.repository: - repository = GitConnector(self.database.repository.full_name) - - if repository.path not in self._additional_addons_paths: - self._additional_addons_paths.append(repository.path) + """Return the list of additional addons paths. + Derived once from the repository linked to the database, unless assigned explicitly. + """ + if self._additional_addons_paths is None: + repository = self.database.repository + self._additional_addons_paths = ( + self.expand_addons_paths([GitConnector(repository.full_name).path]) if repository else [] + ) return self._additional_addons_paths @@ -301,7 +324,10 @@ def additional_repositories(self) -> Generator[GitConnector, None, None]: """Return the list of additional repositories linked to this database.""" for path in self.additional_addons_paths: if (path / ".git").exists() and self.check_addons_path(path): - yield GitConnector(f"{path.parent.name}/{path.name}") + # Pass the path so the name is read from the git remote: a repository cloned outside of + # the `//` convention would otherwise be named + # after the directories it lives in, and resolved back to a path that does not exist. + yield GitConnector(f"{path.parent.name}/{path.name}", path) @property def odoo_worktrees(self) -> Generator[GitWorktree, None, None]: @@ -711,6 +737,82 @@ def missing_npm_packages(self, packages: Sequence[str]) -> Generator[str, None, if f" {package}" not in installed_packages: yield package + def missing_debian_packages(self) -> list[str] | None: + """List the packages Odoo declares in `debian/control` and that are not installed. + + :return: The names of the missing packages, or None on a system where Odoo does not + describe them or where they cannot be queried. + :rtype: Optional[List[str]] + """ + script = self.odoo_path / DEBIAN_INSTALL_SCRIPT + + if not script.is_file() or not shutil.which("dpkg-query"): + return None + + # `--list` only prints the package names parsed out of `debian/control`, it needs no privileges + listed = bash.execute(f"sh {shlex.quote(script.as_posix())} --list", raise_on_error=False) + + if listed is None: + return None + + packages = listed.stdout.decode().split() + installed = bash.execute( + "dpkg-query --show --showformat '${Package} ${Status}\\n' " + " ".join(map(shlex.quote, packages)), + raise_on_error=False, + ) + satisfied = { + line.split(" ", 1)[0] + for line in (installed.stdout.decode().splitlines() if installed else []) + if line.endswith("install ok installed") + } + return [package for package in packages if package not in satisfied] + + def missing_system_dependencies(self) -> list[SystemDependency]: + """List what Odoo needs to build its python dependencies from source and that is missing. + + Odev runs on systems it does not control, so this never assumes a distribution: where Odoo + describes its own packages they are used, and anywhere else only what can be proven missing + is reported. + + :return: The missing dependencies. + :rtype: List[SystemDependency] + """ + if sys.platform not in {"linux", "darwin"}: + return [] + + debian_packages = self.missing_debian_packages() + + if debian_packages is None: + missing = [dependency for dependency in PROBED_DEPENDENCIES if not dependency.found] + else: + missing = [SystemDependency(name=package, packages={"apt-get": package}) for package in debian_packages] + + # Checked everywhere: `debian/control` asks for `python3-dev`, which is not necessarily the + # version of python the Odoo installation is built against + if self.venv.exists and not self.venv.has_development_headers: + missing.append(system.PYTHON_HEADERS) + + return missing + + def check_system_dependencies(self) -> None: + """Warn about what Odoo needs to build its python dependencies from source and that is + missing from the system. + + Building gevent, python-ldap or lxml from source fails with errors naming a missing header + rather than the package providing it, so this is checked when a virtual environment is + created. Nothing is installed: the machine odev runs on belongs to the user. + """ + missing = self.missing_system_dependencies() + + if not missing: + return + + logger.warning( + f"{len(missing)} system dependencies required by Odoo are missing, building its python " + "dependencies from source is likely to fail:\n" + + system.install_instructions(missing, version=self.venv.version) + ) + def prepare_venv(self): """Prepare the virtual environment of the Odoo installation.""" if not self.database.exists: @@ -718,7 +820,10 @@ def prepare_venv(self): if not self.venv.exists: self.venv.create() - self.venv.install_packages(["wheel", "setuptools", "pip", "cython<3.0.0"]) + # After creating the environment: the interpreter it was created from is the one whose + # development headers matter, and the warning still precedes the installs that need them + self.check_system_dependencies() + self.venv.install_packages(["wheel", SETUPTOOLS_REQUIREMENT, "pip", "cython<3.0.0"]) self.venv.install_packages(["pyyaml==5.4.1"], ["--no-build-isolation"]) for path in self.addons_requirements: @@ -728,6 +833,12 @@ def prepare_venv(self): ) if missing_gevent: + # `--no-build-isolation` builds against the setuptools of the virtual environment + # rather than a fresh one, so it has to be usable as a PEP 517 backend. Custom addons + # requirements are installed before odev's own, so this may still be the old one. + if not self.venv.satisfies(SETUPTOOLS_REQUIREMENT): + self.venv.install_packages([SETUPTOOLS_REQUIREMENT, "wheel"]) + self.venv.install_packages([missing_gevent.split(" ;")[0]], ["--no-build-isolation"]) if any(self.venv.missing_requirements(path)): @@ -821,6 +932,21 @@ def check_addons_path(cls, path: Path) -> bool: manifest.is_file() and (manifest.parent / "__init__.py").is_file() for glob in globs for manifest in glob ) + @classmethod + def expand_addons_paths(cls, paths: Sequence[Path]) -> list[Path]: + """Expand paths to the actual Odoo addons directories they contain. + + Modules are looked up recursively so that repositories keeping their modules in + subdirectories are handled the same way as those keeping them at their root. + + :param paths: Paths to expand. + :return: Sorted list of unique valid addons paths. + :rtype: List[Path] + """ + globs = (path.glob(f"**/__{manifest}__.py") for path in paths for manifest in ["manifest", "openerp"]) + candidates = {manifest.parents[1] for glob in globs for manifest in glob} + return sorted(path for path in candidates if cls.check_addons_path(path)) + @classmethod def check_addon_path(cls, path: Path) -> bool: """Return whether the given path is a valid Odoo addon. diff --git a/odev/common/postgres.py b/odev/common/postgres.py index 4f21f79b7..34495314e 100644 --- a/odev/common/postgres.py +++ b/odev/common/postgres.py @@ -1,6 +1,6 @@ """PostgreSQL database class.""" -from collections.abc import Mapping, MutableMapping +from collections.abc import Mapping, MutableMapping, Sequence from contextlib import nullcontext from psycopg2.errors import InvalidTableDefinition @@ -22,9 +22,6 @@ class PostgresDatabase(PostgresConnectorMixin): connector: PostgresConnector """Instance of the connector to the database engine.""" - tables: MutableMapping[str, "PostgresTable"] = {} - """Mapping of tables in the database.""" - def __init__(self, name: str): """Initialize the database.""" super().__init__() @@ -32,14 +29,25 @@ def __init__(self, name: str): self.name: str = name """The name of the database.""" + self.tables: MutableMapping[str, PostgresTable] = {} + """Mapping of tables in the database, keyed by table name.""" + self.prepare_database() def __enter__(self): - self.connector = self._connector_class(self.name).__enter__() + self._enter_connector(self.name) return self def __exit__(self, *args): - self._connector_class(self.name).__exit__(*args) + self._exit_connector(*args) + + def hold_connection(self) -> None: + """Keep the connection open for the lifetime of this object rather than for that of a block. + + Meant for a database read often enough that reconnecting for each operation is wasteful, and + long-lived enough that holding a backend for it is not. + """ + self._enter_connector(self.name) def __repr__(self): """Return the representation of the database.""" @@ -111,10 +119,15 @@ def create_column(self, table: str, column: str, definition: str): return self.connector.create_column(table, column, definition) @ensure_connected - def query(self, query: str, nocache: bool = False): - """Execute a query on the database.""" + def query(self, query: str, params: Sequence | None = None, nocache: bool = False): + """Execute a query on the database. + + :param query: The query to execute. + :param params: Values to bind to the placeholders of the query. + :param nocache: Whether to bypass the query cache. + """ with self.connector.nocache() if nocache else nullcontext(): - return self.connector.query(query) + return self.connector.query(query, params) @ensure_connected def constraint(self, table: str, name: str, definition: str): diff --git a/odev/common/progress.py b/odev/common/progress.py index e450d79a6..21bb5cb8f 100644 --- a/odev/common/progress.py +++ b/odev/common/progress.py @@ -20,7 +20,7 @@ from odev.common import string from odev.common.console import console -from odev.common.debug import DEBUG_MODE +from odev.common.debug import debug_mode, debuggers from odev.common.logging import OdevRichHandler, logging @@ -30,11 +30,31 @@ logger = logging.getLogger(__name__) -if DEBUG_MODE: - logger.warning( - "Disabling live status due to debugger usage:\n" - + string.join_bullet(["Progress bars will not be shown", "Spinners will be replaced with log messages"]) - ) +_debug_warning_shown: bool = False +"""Whether the user was already told that live statuses are disabled.""" + + +def live_status_disabled() -> bool: + """Whether live statuses must be degraded to plain log messages. + + An interactive debugger and a live display cannot share the terminal, so the presence of a call to a debugger + in odev's sources disables spinners and progress bars. The check is only performed when a live status is about + to be displayed: scanning the sources is far too expensive to pay on every odev invocation. + """ + global _debug_warning_shown # noqa: PLW0603 + + if not debug_mode(): + return False + + if not _debug_warning_shown: + _debug_warning_shown = True + logger.warning(f"Interactive debuggers detected:\n{string.join_bullet(debuggers())}") + logger.warning( + "Disabling live status due to debugger usage:\n" + + string.join_bullet(["Progress bars will not be shown", "Spinners will be replaced with log messages"]) + ) + + return True class Progress(RichProgress): @@ -111,7 +131,7 @@ def __enter__(self) -> "Status": if self.stack: self.stack[-1].stop() - if DEBUG_MODE or getattr(console, "headless", False): + if getattr(console, "headless", False) or live_status_disabled(): return self console.is_live = True @@ -176,7 +196,7 @@ def spinner(message: str) -> StackedStatus: :param message: The message to display. :type message: str """ - if not getattr(console, "headless", False) and (DEBUG_MODE or not console.is_interactive): + if not getattr(console, "headless", False) and (not console.is_interactive or live_status_disabled()): logger.info(message) status = StackedStatus(console.render_str(message), console=console, spinner="arc") diff --git a/odev/common/python.py b/odev/common/python.py index d67223f7b..94be1ab78 100644 --- a/odev/common/python.py +++ b/odev/common/python.py @@ -11,9 +11,11 @@ from typing import ClassVar import virtualenv +from packaging.markers import default_environment +from packaging.requirements import InvalidRequirement, Requirement from packaging.version import InvalidVersion, Version, parse as parse_version -from odev.common import bash, progress, string +from odev.common import bash, progress, string, system from odev.common.cache import TTLCache from odev.common.console import console from odev.common.errors import OdevError @@ -43,23 +45,11 @@ re.VERBOSE | re.IGNORECASE, ) -OS_PACKAGES = { - "dnf": [ - "gcc", - "libpq-devel", - "openldap-devel", - "python{version}-devel", - "python{version}", - ], - "apt": [ - "gcc", - "libldap2-dev", - "libpq-dev", - "libsasl2-dev", - "python{version}-dev", - "python{version}", - ], -} +INSTALLABLE_PACKAGE_MANAGERS: frozenset[str] = frozenset({"apt-get", "dnf"}) +"""Package managers odev installs packages with on its own. Anywhere else it only reports what is +missing and lets the user install it, as installing the wrong package under `sudo` on a system odev +has never been tested against is worse than saying nothing. +""" @lru_cache @@ -194,7 +184,11 @@ def create(self) -> None: ): raise OdevError("Failed to create virtual environment") from error - self.install_system_packages() + # Only retry if something was actually installed, otherwise the same question + # would be asked over and over on a system odev cannot install packages on + if not self.install_system_packages(): + raise OdevError("Failed to create virtual environment") from error + return self.create() raise OdevError("Failed to create virtual environment") from error @@ -213,21 +207,34 @@ def remove(self) -> None: return logger.info(f"Removed {venv_description}") - def install_system_packages(self) -> None: - """Install system packages for the current python version.""" + def install_system_packages(self) -> bool: + """Install the system packages needed to run and build against the current python version. + + Odev only installs packages on the systems it has been tested against; on any other one it + reports what is missing and how to install it, and leaves it to the user. + + :return: Whether packages were installed, so that callers know a retry is worth it. + :rtype: bool + """ if self._global: raise OdevError("Cannot install system packages for the global python interpreter") - with progress.spinner("Installing system packages"): - package_manager = next((pkg for pkg in OS_PACKAGES if shutil.which(pkg)), None) + package_manager = system.package_manager() - if not package_manager: - raise OdevError( - f"Neither {string.join_or(list(OS_PACKAGES.keys()))} package managers found on the system, " - "cannot install packages" - ) + if package_manager not in INSTALLABLE_PACKAGE_MANAGERS: + logger.warning( + f"Odev cannot install packages on this system, python {self.version} and the packages " + "needed to build Odoo's dependencies have to be installed manually:\n" + + system.install_instructions(system.ODOO_SYSTEM_DEPENDENCIES, version=self.version) + ) + return False - packages = " ".join([pkg.format(version=self.version) for pkg in OS_PACKAGES[package_manager]]) + with progress.spinner("Installing system packages"): + packages = " ".join( + package + for dependency in system.ODOO_SYSTEM_DEPENDENCIES + if (package := dependency.package(package_manager, version=self.version)) + ) logger.info( f"The following packages will be installed using {package_manager}:\n" + string.join_bullet(packages.split()) @@ -235,7 +242,7 @@ def install_system_packages(self) -> None: if not console.confirm("Continue?", default=True): logger.warning("Aborting system package installation") - return + return False try: bash.execute(f"{package_manager} install -y {packages}", sudo=True) @@ -255,6 +262,27 @@ def install_system_packages(self) -> None: bash.execute(f"ln -s {lldap} {lldap_r}", sudo=True) logger.info(f"Installed system packages for python {self.version}") + return True + + @property + def has_development_headers(self) -> bool: + """Whether the C headers needed to build python extensions are available for this interpreter. + + `INCLUDEPY` points to the headers of the interpreter this environment was created from, which + most distributions ship in a separate package (`python3.10-dev`, `python3.10-devel`). Building + `gevent` or `python-ldap` from source fails without them, with an error naming a missing + `Python.h` rather than the package that provides it. + """ + headers = bash.execute( + f"{self.python} -c 'import sysconfig; print(sysconfig.get_config_var(\"INCLUDEPY\"))'", + raise_on_error=False, + ) + + if headers is None: + # Never warn on a guess: an interpreter that cannot be questioned is assumed complete + return True + + return Path(headers.stdout.decode().strip(), "Python.h").is_file() def install_packages(self, packages: list[str], options: list[str] | None = None) -> None: """Install python packages. @@ -465,43 +493,32 @@ def missing_requirements(self, path: Path | str, raise_if_error: bool = True) -> continue - match = RE_PACKAGE.search(line) - - if match is None: + try: + requirement = Requirement(line) + except InvalidRequirement: + logger.debug(f"Ignoring unparsable requirement {line!r}") continue - if not self.__check_package_conditions(match.group("conditional")): + if requirement.marker is not None and not requirement.marker.evaluate(self.marker_environment): continue - installed_version = installed_packages.get(match.group("name").lower()) + installed_version = installed_packages.get(requirement.name.lower()) if installed_version is None: - logger.debug(f"Missing python package {match.group('name')}") + logger.debug(f"Missing python package {requirement.name}") yield line continue if not isinstance(installed_version, Version): - raise TypeError(f"Invalid version {installed_version!r} for python package {match.group('name')}") + raise TypeError(f"Invalid version {installed_version!r} for python package {requirement.name}") - if match.group("version") is None and match.group("op") is None: + if not requirement.specifier: continue - package_operator = match.group("op") - - if package_operator is None: - continue - - package_version = match.group("version").split("*", 1)[0].rstrip(".") - - version_locals = { - "installed_version": installed_version, - "package_version": parse_version(package_version), - } - - if not eval(f"installed_version {package_operator} package_version", version_locals): # noqa: S307 - known values + if not requirement.specifier.contains(installed_version, prereleases=True): logger.debug( - f"Incorrect python package version {match.group('name')} " - f"({installed_version} {package_operator} {package_version})" + f"Incorrect python package version {requirement.name} " + f"({installed_version} does not satisfy {requirement.specifier})" ) yield line @@ -516,24 +533,36 @@ def __check_requirements_path(self, path: Path | str) -> Path: return requirements_path - def __check_package_conditions(self, conditional: str | None) -> bool: - if conditional is None: - return True + def satisfies(self, specification: str) -> bool: + """Check whether a package installed in this environment satisfies a requirement. - if "python_version" in conditional: - conditional = re.sub( - r"(?:'|\")(3.\d+)(?:'|\")", - lambda m: m.group(1) and " {} ".format(int(m.group(1).replace(".", ""))), - conditional, - ) + :param specification: The requirement to check, e.g. `setuptools>=69.0.0,<82`. + :return: True if the package is installed and its version satisfies the requirement. + :rtype: bool + """ + requirement = Requirement(specification) + installed_version = self.installed_packages().get(requirement.name.lower()) - return eval( # noqa: S307 - known values and operations - conditional, - { - "sys_platform": sys.platform, - "python_version": int(self.version.replace(".", "")), - }, - ) + if not isinstance(installed_version, Version): + return False + + return requirement.specifier.contains(installed_version, prereleases=True) + + @property + def marker_environment(self) -> MutableMapping[str, str]: + """Environment against which the markers of a requirement are evaluated. + + Requirements are evaluated for the python of *this* environment, not for the one odev + itself runs under, so that a requirement conditioned on the python version is resolved + for the Odoo installation it is going to be installed in. + """ + version = self.version + return { + **default_environment(), + "python_version": version, + "python_full_version": version, + "implementation_version": version, + } def run_script( self, diff --git a/odev/common/ssh_crypt.py b/odev/common/ssh_crypt.py index ea684ef85..c1c33ddc4 100644 --- a/odev/common/ssh_crypt.py +++ b/odev/common/ssh_crypt.py @@ -8,11 +8,15 @@ import random from collections import deque from hashlib import sha3_256 +from typing import TYPE_CHECKING from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import padding from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes -from paramiko.agent import AgentKey + + +if TYPE_CHECKING: + from paramiko.agent import AgentKey VALID_SSH_NAME = ["ssh-rsa", "ssh-ed25519"] @@ -105,7 +109,7 @@ def decode(self, data: bytes) -> bytes: class Encryptor: """High-level encryptor using SSH agent.""" - def __init__(self, ssh_key: AgentKey, binary: bool = False): + def __init__(self, ssh_key: "AgentKey", binary: bool = False): """Initialize the encryptor. :param ssh_key: The SSH key to use for signing. @@ -143,7 +147,7 @@ def send(self, data: bytes) -> bytes: class Decryptor: """High-level decryptor using SSH agent.""" - def __init__(self, ssh_key: AgentKey, binary: bool = False): + def __init__(self, ssh_key: "AgentKey", binary: bool = False): """Initialize the decryptor. :param ssh_key: The SSH key to use for signing. @@ -200,7 +204,7 @@ def send(self, data: bytes) -> bytes: return self.decoder.decode(raw_data) -def encrypt(data: str | bytes, ssh_key: AgentKey, binary: bool = False) -> bytes: +def encrypt(data: str | bytes, ssh_key: "AgentKey", binary: bool = False) -> bytes: """Encrypt data using an SSH key. :param data: The data to encrypt. @@ -218,7 +222,7 @@ def encrypt(data: str | bytes, ssh_key: AgentKey, binary: bool = False) -> bytes class E: """A wrapper for decrypting data lazily or as a string.""" - def __init__(self, data: str | bytes, ssh_key: AgentKey, binary: bool = False): + def __init__(self, data: str | bytes, ssh_key: "AgentKey", binary: bool = False): """Initialize the decryptor wrapper. :param data: The encrypted data. diff --git a/odev/common/store/datastore.py b/odev/common/store/datastore.py index ab769ae8f..674b644c0 100644 --- a/odev/common/store/datastore.py +++ b/odev/common/store/datastore.py @@ -18,6 +18,12 @@ class DataStore(PostgresDatabase): def __init__(self, name: str = "odev"): super().__init__(name) + + # Every command reads the store, and it lives as long as the process does: its connection is + # opened once here and held, rather than reopened for each of the reads a single run makes. + # Holding it from this point also covers the tables prepared below. + self.hold_connection() + self.databases = DatabaseStore(self) self.history = HistoryStore(self) self.secrets = SecretStore(self) diff --git a/odev/common/store/tables/databases.py b/odev/common/store/tables/databases.py index b9cf26e78..b56749701 100644 --- a/odev/common/store/tables/databases.py +++ b/odev/common/store/tables/databases.py @@ -1,6 +1,6 @@ from collections.abc import Mapping from dataclasses import dataclass -from typing import Literal +from typing import Any, Literal from odev.common.databases import Database, LocalDatabase from odev.common.postgres import PostgresTable @@ -106,6 +106,30 @@ def set(self, database: Database, arguments: str | None = None): """ ) + def set_value(self, database: Database, key: str, value: Any): + """Set a single value for a database, leaving its other values untouched. + + Unlike :meth:`set`, this does not go through the database properties, which makes it the + only way to clear a value: the properties fall back to reading the data store when their + cached value is empty, and would write the cleared value straight back. + + :param database: The database to set the value for. + :param key: The name of the column to set. + :param value: The value to set, bound as a query parameter. + """ + if key not in self._columns: + raise ValueError(f"Unknown column {key!r} in table {self.name!r}") + + self.database.query( + f""" + UPDATE {self.name} + SET {key} = %s + WHERE name = %s + AND platform = %s + """, + (value, database.name, database.platform.name), + ) + def delete(self, database: Database): """Delete the saved values of a database.""" self.database.query( diff --git a/odev/common/store/tables/secrets.py b/odev/common/store/tables/secrets.py index eba6c5513..3be1bd2d4 100644 --- a/odev/common/store/tables/secrets.py +++ b/odev/common/store/tables/secrets.py @@ -2,17 +2,17 @@ from base64 import b64decode, b64encode from collections.abc import Sequence from dataclasses import dataclass -from typing import Literal - -from paramiko.agent import Agent as SSHAgent, AgentKey -from paramiko.ssh_exception import SSHException +from typing import TYPE_CHECKING, Literal from odev.common.config import Config from odev.common.console import console from odev.common.errors import OdevError from odev.common.logging import logging from odev.common.postgres import PostgresTable -from odev.common.ssh_crypt import E as ssh_decrypt, encrypt as ssh_encrypt # noqa: N811 + + +if TYPE_CHECKING: + from paramiko.agent import AgentKey logger = logging.getLogger(__name__) @@ -68,8 +68,10 @@ class SecretStore(PostgresTable): """Configuration parameters.""" @classmethod - def _list_ssh_keys(cls) -> list[AgentKey]: + def _list_ssh_keys(cls) -> list["AgentKey"]: """List all SSH keys available in the ssh-agent.""" + from paramiko.agent import Agent as SSHAgent # noqa: PLC0415 - importing paramiko is expensive + keys = list(SSHAgent().get_keys()) if not keys and not os.environ.get("ODEV_NO_SSH_AGENT"): @@ -93,6 +95,10 @@ def encrypt(cls, plaintext: str) -> str: :return: The encrypted string. :rtype: str """ + from paramiko.ssh_exception import SSHException # noqa: PLC0415 - importing paramiko is expensive + + from odev.common.ssh_crypt import encrypt as ssh_encrypt # noqa: PLC0415 + ciphered: str | None = None keys = cls._list_ssh_keys() @@ -121,6 +127,10 @@ def decrypt(cls, ciphertext: str) -> str: :return: The decrypted string. :rtype: str """ + from paramiko.ssh_exception import SSHException # noqa: PLC0415 - importing paramiko is expensive + + from odev.common.ssh_crypt import E as ssh_decrypt # noqa: N811, PLC0415 + deciphered: str | None = None keys = cls._list_ssh_keys() diff --git a/odev/common/string.py b/odev/common/string.py index 50f6dd607..247438957 100644 --- a/odev/common/string.py +++ b/odev/common/string.py @@ -93,11 +93,13 @@ def dedent(text: str, dedent: int = 0) -> str: def min_indent(text: str) -> int: """Return the smallest indentation in a text. + A text without any non-blank line has no indentation to speak of and returns zero. + :param text: The text to get the minimum indentation from. :return: The minimum indentation of the text. :rtype: int """ - return min(len(line) - len(line.lstrip()) for line in text.splitlines() if line.strip()) + return min((len(line) - len(line.lstrip()) for line in text.splitlines() if line.strip()), default=0) def bytes_size(size: int | float) -> str: @@ -281,18 +283,27 @@ def ago(date: datetime.datetime) -> str: def quote(string: str, dirty_only: bool = False, force_single: bool = False) -> str: """Quote a string. + The quote character is chosen so that it does not appear in the string: a string containing single + quotes is wrapped in double quotes, and the other way around. + + **Warning** This helper picks a delimiter, it does not escape. A string containing both quote + characters, or containing the delimiter imposed by `force_single`, cannot be represented and comes + back with an unbalanced delimiter. Do not use it to interpolate untrusted input into SQL or shell + commands; use `shlex.quote` or query parameters instead. + :param string: The string to quote. :param dirty_only: Do not quote strings that have no quotes to begin with. :param force_single: Force single quotes. :return: The quoted string. :rtype: str """ - index = max(string.find(char) for char in ("'", '"')) + contains_single = "'" in string + contains_double = '"' in string - if dirty_only and index == -1: + if dirty_only and not contains_single and not contains_double: return string - double = not force_single and (index == -1 or string[index] == "'") + double = not force_single and (contains_single or not contains_double) return f'"{string}"' if double else f"'{string}'" diff --git a/odev/common/system.py b/odev/common/system.py new file mode 100644 index 000000000..ea21de19b --- /dev/null +++ b/odev/common/system.py @@ -0,0 +1,174 @@ +"""Description of the operating system odev runs on and of the packages it can install.""" + +import shutil +from collections.abc import Mapping, Sequence +from dataclasses import dataclass, field + +from odev.common import string + + +__all__ = [ + "ODOO_SYSTEM_DEPENDENCIES", + "SystemDependency", + "install_instructions", + "package_manager", +] + + +PACKAGE_MANAGERS: Mapping[str, str] = { + "apt-get": "sudo apt-get install -y {packages}", + "dnf": "sudo dnf install -y {packages}", + "zypper": "sudo zypper install -y {packages}", + "pacman": "sudo pacman -S --needed {packages}", + "apk": "sudo apk add {packages}", + "brew": "brew install {packages}", +} +"""Command installing packages with each of the package managers odev knows about, keyed by the +executable to look for on the `PATH`. Package managers shipped by the distribution come first so +that a Linux machine on which Homebrew is also installed is told to use its own. +""" + + +@dataclass(frozen=True) +class SystemDependency: + """A part of the operating system Odoo needs to build its python dependencies from source.""" + + name: str + """Human-readable description, displayed as-is when no package can be named for the current + system. May contain placeholders, `{version}` being the version of python being installed. + """ + + packages: Mapping[str, str] = field(default_factory=dict) + """Package providing this dependency, per package manager. A package manager absent from this + mapping has nothing to install: Arch Linux ships the python headers within `python` itself and + the compiler of macOS comes from `xcode-select --install`, not from Homebrew. + """ + + commands: Sequence[str] = () + """Executables proving, if any of them is found on the `PATH`, that this dependency is + installed. Dependencies detected some other way leave this empty and are never found. + """ + + @property + def found(self) -> bool: + """Whether the dependency can be proven to be installed on the current system.""" + return any(shutil.which(command) for command in self.commands) + + def package(self, manager: str, **placeholders: str) -> str | None: + """Package to install to provide this dependency with a given package manager. + + :param manager: The package manager to install the dependency with. + :param placeholders: Values for the placeholders in the name of the package. + :return: The name of the package, or None if the package manager does not ship one. + :rtype: Optional[str] + """ + package = self.packages.get(manager) + return package.format(**placeholders) if package is not None else None + + +PYTHON_INTERPRETER = SystemDependency( + name="the python {version} interpreter", + packages={ + "apt-get": "python{version}", + "dnf": "python{version}", + "zypper": "python{version}", + "pacman": "python", + "apk": "python3", + "brew": "python@{version}", + }, +) + +PYTHON_HEADERS = SystemDependency( + name="the development headers of python {version} (Python.h)", + packages={ + "apt-get": "python{version}-dev", + "dnf": "python{version}-devel", + "zypper": "python{version}-devel", + "apk": "python3-dev", + }, +) + +C_COMPILER = SystemDependency( + name="a C compiler", + packages={ + "apt-get": "build-essential", + "dnf": "gcc", + "zypper": "gcc", + "pacman": "base-devel", + "apk": "build-base", + }, + commands=("cc", "gcc", "clang"), +) + +POSTGRESQL_HEADERS = SystemDependency( + name="the PostgreSQL client headers (pg_config)", + packages={ + "apt-get": "libpq-dev", + "dnf": "libpq-devel", + "zypper": "postgresql-devel", + "pacman": "postgresql-libs", + "apk": "postgresql-dev", + "brew": "libpq", + }, + commands=("pg_config",), +) + +LDAP_HEADERS = SystemDependency( + name="the OpenLDAP and SASL headers", + packages={ + "apt-get": "libldap2-dev libsasl2-dev", + "dnf": "openldap-devel", + "zypper": "openldap2-devel cyrus-sasl-devel", + "pacman": "libldap libsasl", + "apk": "openldap-dev", + "brew": "openldap", + }, +) + +ODOO_SYSTEM_DEPENDENCIES: Sequence[SystemDependency] = ( + PYTHON_INTERPRETER, + PYTHON_HEADERS, + C_COMPILER, + POSTGRESQL_HEADERS, + LDAP_HEADERS, +) +"""Everything Odoo needs to build its python dependencies from source, in the order in which it is +worth installing it. +""" + + +def package_manager() -> str | None: + """Find the package manager of the current system. + + :return: The name of the package manager, or None if odev does not know the one in use. + :rtype: Optional[str] + """ + return next((manager for manager in PACKAGE_MANAGERS if shutil.which(manager)), None) + + +def install_instructions(dependencies: Sequence[SystemDependency], **placeholders: str) -> str: + """Describe what is missing from the current system and how to install it. + + Odev runs on systems it does not control, and cannot name a package for all of them, so the + dependencies are always described in plain words; the command is only added on top when the + package manager in use is one odev knows. + + :param dependencies: The dependencies missing from the current system. + :param placeholders: Values for the placeholders in the names of the dependencies and of their + packages, `version` being the version of python being installed. + :return: A bullet list of the missing dependencies, followed by the command installing them. + :rtype: str + """ + instructions = string.join_bullet([dependency.name.format(**placeholders) for dependency in dependencies]) + manager = package_manager() + + if manager is None: + return instructions + + packages = [package for dependency in dependencies if (package := dependency.package(manager, **placeholders))] + + if not packages: + return instructions + + command = PACKAGE_MANAGERS[manager].format(packages=" ".join(packages)) + return f"{instructions}\n\nInstall them by running:\n{string.stylize(command, 'color.cyan')}" diff --git a/odev/common/telemetry.py b/odev/common/telemetry.py index 26f9c64ec..6fe2f1347 100644 --- a/odev/common/telemetry.py +++ b/odev/common/telemetry.py @@ -1,19 +1,28 @@ -"""Telemetry module for odev.""" +"""Telemetry module for odev. + +Reporting a command must never delay the CLI: waiting for the telemetry endpoint after a command printed its result +is directly perceptible to the user. Records are therefore written to a local spool file when a command completes, +and submitted in the background by a later odev run, which has the whole duration of its own command to do so. +""" import json import re import threading import uuid -from queue import Queue -from time import monotonic +from pathlib import Path +from typing import TYPE_CHECKING, Any from urllib.error import URLError from urllib.request import Request, urlopen -from odev.common.commands.base import Command +from odev.common.config import CONFIG_DIR from odev.common.logging import logging from odev.common.utils import EmployeeUtils +if TYPE_CHECKING: + from odev.common.commands.base import Command + + logger = logging.getLogger(__name__) TELEMETRY_ENDPOINT = "https://odev-telemetry.odoo.com" @@ -23,6 +32,44 @@ # This is not ideal but it will already prevent most automated bots from sending fake data. TELEMETRY_KEY = "xEGGxJLlTuRfGO8f5STWpehXKGRB8RbVpo3DgWYA7nJquh16I5Q59SU+ucyhcZoy" +REQUEST_TIMEOUT = 1 +"""Timeout in seconds for a single request to the telemetry endpoint.""" + +MAX_SPOOLED_RECORDS = 100 +"""Number of records kept in the spool file when the endpoint cannot be reached. + +Old records are dropped past this limit so that a long-lasting outage cannot grow the file indefinitely. +""" + + +class TelemetryRun: + """Handle on the telemetry record of a single command. + + The record is only complete once the command finished, since it carries its exit code and execution time. + Callers must therefore signal completion through :meth:`finish`, which spools the record for submission. + """ + + def __init__(self, telemetry: "Telemetry", payload: dict[str, Any]): + self.telemetry: Telemetry = telemetry + """Telemetry manager this record belongs to.""" + + self.payload: dict[str, Any] = payload + """Data describing the command being reported.""" + + def finish(self, exit_code: int = 0, execution_time: float = 0.0) -> None: + """Complete the record with the outcome of the command and spool it for submission. + + :param exit_code: Exit code of the command. + :param execution_time: Time the command took to run, in minutes. + """ + self.telemetry.spool( + { + "payload": self.payload, + "exit_code": exit_code, + "execution_time": execution_time, + } + ) + class Telemetry: """Telemetry manager.""" @@ -30,6 +77,11 @@ class Telemetry: def __init__(self, odev): self.odev = odev + @property + def spool_path(self) -> Path: + """Path to the file holding the telemetry records awaiting submission.""" + return CONFIG_DIR / f"{self.odev.name}-telemetry.jsonl" + def _get_client_id(self) -> str: """Get or generate the client ID.""" client_id = self.odev.config.telemetry.client_id @@ -55,7 +107,24 @@ def _prepare_request(self, path: str, payload: dict) -> Request: data=data, ) - def _sanitize_arguments(self, command: Command) -> tuple[str, str]: + def _send_request(self, path: str, payload: dict) -> dict[str, Any] | None: + """Send telemetry data to the given endpoint and return the decoded response. + + :param path: Path of the endpoint to send the data to. + :param payload: Data to send. + :return: The decoded response, or None if the data could not be sent. + """ + try: + with urlopen(self._prepare_request(path, payload), timeout=REQUEST_TIMEOUT) as response: # noqa: S310 + return json.loads(response.read()) + except (URLError, OSError) as error: + logger.debug(f"Telemetry failed: {error}") + except json.JSONDecodeError as error: + logger.debug(f"Telemetry returned an invalid response: {error}") + + return None + + def _sanitize_arguments(self, command: "Command") -> tuple[str, str]: """Sanitize arguments for telemetry so that sensitive data is not sent.""" arguments = " ".join(command._argv) if command._argv else "" additional_args = "" @@ -88,17 +157,25 @@ def _sanitize_arguments(self, command: Command) -> tuple[str, str]: return arguments, additional_args - def send(self, command: Command) -> tuple[threading.Thread, Queue] | None: - """Send telemetry data.""" + def send(self, command: "Command") -> TelemetryRun | None: + """Start recording the execution of a command. + + The returned handle must be completed through :meth:`TelemetryRun.finish` once the command is done, so that + its exit code and execution time are recorded as well. + + :param command: The command being run. + :return: A handle on the record, or None if this command must not be reported. + """ if len(self.odev._command_stack) != 1 or self.odev.in_test_mode: return None + enabled = self.odev.config.telemetry.enabled payload = { "client_id": self._get_client_id(), - "is_telemetry_agreed": self.odev.config.telemetry.enabled, + "is_telemetry_agreed": enabled, } - if self.odev.config.telemetry.enabled: + if enabled: args, additional_args = self._sanitize_arguments(command) payload.update( { @@ -114,42 +191,100 @@ def send(self, command: Command) -> tuple[threading.Thread, Queue] | None: } ) - def _send(_queue: Queue): - try: - request = self._prepare_request("odev/telemetry", payload) + return TelemetryRun(self, payload) - with urlopen(request, timeout=1) as response: # noqa: S310 - content = response.read() + def spool(self, record: dict[str, Any]) -> None: + """Append a record to the spool file, to be submitted by a later run. - result = json.loads(content).get("result", {}).get("id") - _queue.put(result) - except (URLError, OSError) as e: - logger.debug(f"Telemetry failed: {e}") - _queue.put(None) + :param record: The record to spool. + """ + try: + self.spool_path.parent.mkdir(parents=True, exist_ok=True) - queue = Queue(maxsize=1) - thread = threading.Thread(target=_send, args=(queue,)) - thread.start() - return thread, queue + with self.spool_path.open("a", encoding="utf-8") as spool: + spool.write(json.dumps(record) + "\n") + except OSError as error: + logger.debug(f"Failed to spool telemetry: {error}") - def update(self, line_id: int) -> None: - """Update a specific line in the telemetry data.""" - if len(self.odev._command_stack) != 1 or self.odev.in_test_mode: + def flush(self) -> None: + """Submit the records spooled by previous runs in a background thread. + + The thread is a daemon: whatever it did not manage to send stays in the spool and is retried by the next + run, so that exiting odev never waits on the telemetry endpoint. + """ + if self.odev.in_test_mode or not self.spool_path.is_file(): return - payload = { - "telemetry_id": line_id, - "exit_code": 0, - "execution_time": (monotonic() - self.odev.start_time) / 60, - } + threading.Thread(target=self._flush, name="odev-telemetry", daemon=True).start() + + def _flush(self) -> None: + """Submit every spooled record, keeping in the spool the ones that could not be sent.""" + records = self._claim_spooled_records() + + if not records: + return + + logger.debug(f"Submitting {len(records)} spooled telemetry records") + unsent = [record for record in records if not self._submit(record)] + + if unsent: + self._respool(unsent) + + def _claim_spooled_records(self) -> list[dict[str, Any]]: + """Read the spooled records and empty the spool file so that they are not submitted twice. + + :return: The records that were waiting in the spool. + """ + records: list[dict[str, Any]] = [] - def _update(): + try: + with self.spool_path.open("r+", encoding="utf-8") as spool: + lines = spool.readlines() + spool.seek(0) + spool.truncate() + except OSError as error: + logger.debug(f"Failed to read spooled telemetry: {error}") + return records + + for line in lines: try: - request = self._prepare_request("odev/telemetry/update", payload) + records.append(json.loads(line)) + except json.JSONDecodeError: + logger.debug(f"Discarding malformed telemetry record: {line.strip()!r}") + + return records + + def _respool(self, records: list[dict[str, Any]]) -> None: + """Put records that could not be submitted back into the spool. + + :param records: The records to keep for a later run. + """ + for record in records[-MAX_SPOOLED_RECORDS:]: + self.spool(record) + + def _submit(self, record: dict[str, Any]) -> bool: + """Submit a single spooled record to the telemetry endpoint. - with urlopen(request, timeout=1): # noqa: S310 - pass - except (URLError, OSError) as e: - logger.debug(f"Telemetry failed: {e}") + :param record: The record to submit. + :return: Whether the record was submitted successfully. + """ + response = self._send_request("odev/telemetry", record["payload"]) + + if response is None: + return False + + line_id = response.get("result", {}).get("id") + + if line_id is None or not record["payload"].get("is_telemetry_agreed"): + return True + + self._send_request( + "odev/telemetry/update", + { + "telemetry_id": line_id, + "exit_code": record["exit_code"], + "execution_time": record["execution_time"], + }, + ) - threading.Thread(target=_update).start() + return True diff --git a/odev/common/utils.py b/odev/common/utils.py index 595e467fa..5377c0277 100644 --- a/odev/common/utils.py +++ b/odev/common/utils.py @@ -1,6 +1,7 @@ """Utility classes and functions for odev.""" from odev.common import bash +from odev.common.config import XGRAM_UNKNOWN from odev.common.logging import logging @@ -16,6 +17,22 @@ def __init__(self, odev): def get_xgram(self) -> str | None: """Get the user's xgram from their Odoo email. + The result is cached in the configuration file: resolving it requires a vault lookup and a call to git, + which would otherwise be paid on every single odev invocation. + """ + cached = self.odev.config.user.xgram + + if cached != XGRAM_UNKNOWN: + return cached or None + + xgram = self._resolve_xgram() + self.odev.config.user.xgram = xgram or "" + + return xgram + + def _resolve_xgram(self) -> str | None: + """Resolve the user's xgram from their Odoo email. + Checks secrets first, then falls back to git configuration. """ # 1. Try from secrets diff --git a/odev/common/version.py b/odev/common/version.py index a5151d6f7..668ec9864 100644 --- a/odev/common/version.py +++ b/odev/common/version.py @@ -86,7 +86,9 @@ def __repr__(self) -> str: def __bool__(self) -> bool: """Return True if the version is not empty.""" - return bool(self.major or self.minor or self.module or self.master) + # `module` is padded to `MIN_VERSION_LENGTH` and is therefore never an empty tuple: + # it has to be tested on its values rather than on its own truthiness. + return bool(self.major or self.minor or any(self.module) or self.master) @property def major(self) -> int: @@ -130,7 +132,8 @@ def _cmpkey(master: bool, major: int, minor: int, module: tuple, enterprise: boo # Saas versions should sort after non-saas versions _saas = int(saas) - # Master versions should sort before non-master versions + # Master is the development version and therefore sorts after every numbered version, hence it comes + # first in the key so that it outweighs the major number. _master = int(master) return _master, major, minor, _module, enterprise, _saas diff --git a/odev/setup/symlink.py b/odev/setup/symlink.py index 7d2470e58..2ccdbf0ef 100644 --- a/odev/setup/symlink.py +++ b/odev/setup/symlink.py @@ -15,6 +15,27 @@ PRIORITY = 10 +LOCAL_BIN_PATH = Path("~/.local/bin").expanduser() +"""Directory holding the executables of the current user.""" + +SYSTEM_BIN_PATH = Path("/usr/local/bin") +"""Directory holding the executables of every user, used when the local one is not in `PATH`.""" + + +# --- Values ------------------------------------------------------------------- + + +def link_path() -> Path: + """Return the path the `odev` command should be linked from. + + :return: The link to create, in the local binaries directory of the user when it is in `PATH`, in the + system-wide one otherwise. + :rtype: Path + """ + known_env_paths = getenv("PATH", "").split(":") + + return (LOCAL_BIN_PATH if str(LOCAL_BIN_PATH) in known_env_paths else SYSTEM_BIN_PATH) / "odev" + # --- Setup -------------------------------------------------------------------- @@ -25,26 +46,24 @@ def setup(odev: Odev) -> None: :param config: Odev configuration """ main_path = Path(__file__).parents[2] / "odev.sh" - known_env_paths = getenv("PATH", "").split(":") - local_path = Path("~/.local/bin").expanduser() - link_path = (local_path if str(local_path) in known_env_paths else Path("/usr/local/bin")) / "odev" + command_path = link_path() - if not link_path.parent.exists(): - logger.debug(f"Directory {link_path.parent} does not exist, creating it") - link_path.parent.mkdir(parents=True) + if not command_path.parent.exists(): + logger.debug(f"Directory {command_path.parent} does not exist, creating it") + command_path.parent.mkdir(parents=True) - if link_path.exists() or link_path.is_symlink(): - logger.warning(f"Symlink path {link_path} already exists, this is used to run odev as a shell command") + if command_path.exists() or command_path.is_symlink(): + logger.warning(f"Symlink path {command_path} already exists, this is used to run odev as a shell command") if console.confirm("Would you like to overwrite it?"): - logger.debug(f"Removing symlink path {link_path}") - bash.execute(f"rm {link_path}", sudo=True) + logger.debug(f"Removing symlink path {command_path}") + bash.execute(f"rm {command_path}", sudo=True) - if not link_path.exists() and not link_path.is_symlink(): - logger.debug(f"Creating symlink from {link_path} to {main_path}") - bash.execute(f"ln -s {main_path} {link_path}", sudo=True) + if not command_path.exists() and not command_path.is_symlink(): + logger.debug(f"Creating symlink from {command_path} to {main_path}") + bash.execute(f"ln -s {main_path} {command_path}", sudo=True) logger.info("Symlink created") - for path in (main_path, link_path): + for path in (main_path, command_path): logger.debug(f"Checking execute permissions for {path}") path.chmod(path.stat().st_mode | stat.S_IXUSR) diff --git a/odev/static/requirements.txt b/odev/static/requirements.txt index c5bbc4f71..7c832434f 100644 --- a/odev/static/requirements.txt +++ b/odev/static/requirements.txt @@ -2,7 +2,9 @@ ipdb phonenumbers pudb pydevd-odoo +# setuptools 58-59 cannot serve as a PEP 517 backend for current pip, which breaks +# `pip install --no-build-isolation` for gevent and other source distributions. +# Capped below 82, which removed `pkg_resources`; Odoo 15.0 and 16.0 import it unconditionally. +setuptools>=69.0.0,<82; python_version >= '3.8' setuptools<58.0.0; python_version < '3.8' -setuptools>=58.0.0, <59.0.0; python_version >= '3.8' and python_version < '3.12' -setuptools>59.0.0; python_version >= '3.12' websocket-client diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 000000000..b877c8849 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,119 @@ +"""Session-wide setup for the test suite: give this run a private sandbox and make sure it is cleaned up. + +`pytest_sessionfinish` runs from a `finally` block in pytest's session wrapper, so the sandbox is removed +both when the suite completes and when it is interrupted. A `SIGTERM` and a `Ctrl+C` are both turned into +the same orderly exit, and anything that still escapes — a `SIGKILL`, a crashed interpreter — is picked up +by the sweep at the start of the next run. +""" + +import atexit +import logging +from collections.abc import Callable +from signal import SIGINT, SIGTERM, Signals, signal +from typing import Any +from unittest.mock import patch + +import pytest + +from odev.common.logging import OdevRichHandler + +from tests.fixtures import sandbox + + +TERMINATED_EXIT_CODE = 2 +"""Exit code reported when the suite is stopped before it could complete.""" + +INTERRUPT_MESSAGE = "interrupted" +"""Reason reported when the suite is stopped before it could complete.""" + +SIGNAL_INSTALLER = "odev.common.signal_handling.signal" +"""Where odev installs the signal handlers it uses to cancel the operation it is running.""" + + +class InterruptRecorder: + """Signal handler noting an interrupt for the session, then handing it over to odev. + + odev captures `SIGINT` around every query and every subprocess, and its handlers cancel that single + operation instead of propagating. A suite spends much of its time inside one of those blocks, so a + `Ctrl+C` landing in one would be swallowed and the run would carry on. Letting the interrupt through + instead abandons the PostgreSQL connection mid-statement, and a suite interrupted that way exhausts + the connection slots of the server; so it is recorded here and acted upon at the next test boundary, + once odev has closed what it had open. + """ + + interrupted: bool = False + """Whether an interrupt was received while odev was holding the signal handlers.""" + + def __init__(self, handler: Callable[..., Any]): + self.handler: Callable[..., Any] = handler + """The handler odev installed, called once the interrupt has been recorded.""" + + def __call__(self, *args) -> Any: + InterruptRecorder.interrupted = True + + return self.handler(*args) + + +def pytest_configure(config: pytest.Config) -> None: + """Make the suite stoppable, whichever way it is asked to stop, and leave the logs to pytest.""" + + def terminate(*args): + pytest.exit(INTERRUPT_MESSAGE, returncode=TERMINATED_EXIT_CODE) + + signal(SIGTERM, terminate) + + installer = patch(SIGNAL_INSTALLER, new=install_handler) + installer.start() + config.add_cleanup(installer.stop) + + detach_odev_log_handler() + + +def detach_odev_log_handler() -> None: + """Take odev's own logging handler off the root logger for the duration of the suite. + + `odev.common.logging` configures logging when it is imported, but `logging.basicConfig` is a no-op + once the root logger has handlers: whether odev's handler ends up installed depends on whether that + import happens before or after pytest sets its own up. Importing anything from odev in this module, + as the sandbox does, is enough to tip it one way. + + The handler renders through the console, so leaving it on makes every record reach the output twice — + once rendered by odev and once through whatever the test is capturing — and turns a plain `logger.info` + into a `console.print` that tests patching the console then have to account for. Records are left to + the handlers pytest and the test cases install, which is what the suite asserts on. + """ + root = logging.getLogger() + + for handler in [handler for handler in root.handlers if isinstance(handler, OdevRichHandler)]: + root.removeHandler(handler) + + +def pytest_sessionstart(session: pytest.Session) -> None: + """Claim a sandbox for this run, then clean up after the runs that no longer own theirs.""" + sandbox.acquire() + atexit.register(sandbox.release) + sandbox.sweep() + + +def pytest_runtest_setup(item: pytest.Item) -> None: + """Stop the session on an interrupt odev handled itself, now that it is between two tests.""" + if InterruptRecorder.interrupted: + pytest.exit(INTERRUPT_MESSAGE, returncode=TERMINATED_EXIT_CODE) + + +def pytest_sessionfinish(session: pytest.Session, exitstatus: int) -> None: + """Remove the sandbox of this run, whether the suite completed or was interrupted.""" + sandbox.release() + + +def install_handler(signal_number: Signals, handler: Any) -> Any: + """Install a signal handler for odev, recording the interrupts it would otherwise swallow. + + :param signal_number: The signal odev wants to handle. + :param handler: The handler it wants to install, or the one it restores once its block is over. + :return: The handler that was previously installed. + """ + if signal_number != SIGINT or not callable(handler) or isinstance(handler, InterruptRecorder): + return signal(signal_number, handler) + + return signal(signal_number, InterruptRecorder(handler)) diff --git a/tests/fixtures/capture.py b/tests/fixtures/capture.py index 37297bd22..b702949cd 100644 --- a/tests/fixtures/capture.py +++ b/tests/fixtures/capture.py @@ -24,6 +24,7 @@ def __init__(self): self._stderr_value = "" self._stdout_handler = None self._stderr_handler = None + self._root_level = logging.NOTSET def __enter__(self): self._stdout = StringIO() @@ -33,12 +34,18 @@ def __enter__(self): sys.stdout = self._stdout sys.stderr = self._stderr + # Capture on the root logger rather than on each existing one: command modules are only imported when their + # command runs, so their logger does not exist yet when the capture starts. + root = logging.getLogger() + self._root_level = root.level + root.setLevel(logging.INFO) + root.addHandler(self._stdout_handler) + root.addHandler(self._stderr_handler) + for logger in logging.Logger.manager.loggerDict.values(): if isinstance(logger, logging.Logger): logger.propagate = True logger.setLevel(logging.INFO) - logger.addHandler(self._stdout_handler) - logger.addHandler(self._stderr_handler) return self @@ -46,10 +53,10 @@ def __exit__(self, *args): if self._stdout_handler is None or self._stderr_handler is None: raise AssertionError("CaptureOutput not properly initialized") - for logger in logging.Logger.manager.loggerDict.values(): - if isinstance(logger, logging.Logger): - logger.removeHandler(self._stdout_handler) - logger.removeHandler(self._stderr_handler) + root = logging.getLogger() + root.removeHandler(self._stdout_handler) + root.removeHandler(self._stderr_handler) + root.setLevel(self._root_level) if self._stderr is None or self._stdout is None: raise AssertionError("CaptureOutput streams not properly initialized") diff --git a/tests/fixtures/case.py b/tests/fixtures/case.py index 5d17f7dc4..3765bf70a 100644 --- a/tests/fixtures/case.py +++ b/tests/fixtures/case.py @@ -16,7 +16,7 @@ from odev.common.config import Config from odev.common.string import suid -from tests.fixtures import CaptureOutput +from tests.fixtures import CaptureOutput, sandbox class OdevTestCase(TestCase): @@ -32,10 +32,14 @@ class OdevTestCase(TestCase): """Name of the test case run, used for environment preparation.""" run_path: ClassVar[Path] - """Path to the test case run directory under `/tmp`.""" + """Path to the test case run directory, inside the sandbox of the current suite run.""" - _patches: ClassVar[list[_patch]] = [] - """The patches applied to the test case.""" + _patches: ClassVar[list[_patch]] + """The patches applied to the test case. + + Assigned per class in `setUpClass`: a list defined here would be shared by every subclass through + `cls._patches.append(...)`, making each class tear down the patches of all the classes before it. + """ __config: str """Content of the configuration file to restore after each test case.""" @@ -50,11 +54,17 @@ def tearDown(self): @classmethod def setUpClass(cls): - Config.parser = ConfigParser() - cls.odev = odev.Odev(test=True) + cls._patches = [] cls.run_id = suid() - cls.run_name = f"{cls.odev.name}-{cls.run_id}" - cls.run_path = Path(f"/tmp/{cls.run_name}") # noqa: S108 + cls.run_path = sandbox.SESSION_PATH / cls.run_id + cls.run_name = f"{sandbox.SESSION_NAME}-{cls.run_id}" + + # The framework reads its name and its configuration directory while being constructed, so both + # have to point inside the sandbox before the instance exists. + cls.__patch_paths() + + Config.parser = ConfigParser() + cls.odev = odev.Odev(test=True, name=sandbox.SESSION_NAME) cls.res_path = cls.odev.tests_path / "resources" cls.replacer = Replacer() cls.__patch_cli() @@ -62,6 +72,7 @@ def setUpClass(cls): cls.__patch_framework() cls.addClassCleanup(cls.tearDownClass) cls.odev.start() + cls.__sandbox_config_paths() @classmethod def tearDownClass(cls): @@ -69,10 +80,11 @@ def tearDownClass(cls): cls.replacer.restore() cls.odev.commands.clear() cls.odev.store.drop() - cls.odev.config.path.unlink(missing_ok=True) - if cls.run_path.exists(): - shutil.rmtree(cls.run_path, ignore_errors=True) + # The configuration file lives in the run directory, and goes away with it. Whatever this misses, + # because the run was interrupted or because a test left a database behind, is picked up by the + # sandbox: either when the suite ends or at the start of the next one. + shutil.rmtree(cls.run_path, ignore_errors=True) odev.HOME_PATH = (Path.home() / ".local" / "share" / "odev").resolve() @@ -152,8 +164,10 @@ def _import_dotted_path(cls, path: str) -> Any: @classmethod def __unpatch_all(cls): - for patched in cls._patches: - patched.stop() + # `tearDownClass` runs twice, once through `addClassCleanup` and once through unittest itself, + # so the patches are dropped as they are stopped. + while cls._patches: + cls._patches.pop().stop() @classmethod def _patch_object( @@ -178,12 +192,35 @@ def _patch_object( cls._patches.append(patched) patched.start() + @classmethod + def __patch_paths(cls): + """Redirect the configuration directory into the run directory. + + The config file, and the plugin `config.py` modules `Config` discovers next to it, then come from + the sandbox instead of `~/.config/odev`: the suite writes nothing outside of it, and behaves the + same whether or not the developer running it has plugins installed. + """ + patched = patch("odev.common.config.CONFIG_DIR", cls.run_path) + cls._patches.append(patched) + patched.start() + + @classmethod + def __sandbox_config_paths(cls): + """Point the directories odev reads from its configuration at the sandbox. + + They default to `~/odoo`, where a test cloning a repository or downloading a dump would land in + the middle of the checkouts of the user — and in the way of a suite running alongside this one. + """ + cls.odev.config.paths.repositories = cls.run_path / "repositories" + cls.odev.config.paths.dumps = cls.run_path / "dumps" + cls.odev.config.paths.upgrade = cls.run_path / "repositories" / "odoo" / "upgrade" + @classmethod def __patch_cli(cls): """Patch interactions with the CLI to avoid waiting for user input or showing live status during tests.""" cls._patch_object("odev.common.console.Console", properties=[("bypass_prompt", True)]) - cls._patch_object("odev.common.debug", [("DEBUG_MODE", True)]) - cls._patch_object("odev.common.progress", [("DEBUG_MODE", True)]) + cls._patch_object("odev.common.debug", [("debug_mode", True)]) + cls._patch_object("odev.common.progress", [("live_status_disabled", True)]) @classmethod def __patch_framework(cls): @@ -208,7 +245,6 @@ def __patch_odev(cls): ("_update", False), ], [ - ("name", "odev-test"), ("upgrades_path", cls.odev.tests_path / "resources" / "upgrades"), ("setup_path", cls.odev.tests_path / "resources" / "setup"), ("scripts_path", cls.odev.tests_path / "resources" / "scripts"), diff --git a/tests/fixtures/sandbox.py b/tests/fixtures/sandbox.py new file mode 100644 index 000000000..a6dc526fe --- /dev/null +++ b/tests/fixtures/sandbox.py @@ -0,0 +1,251 @@ +"""Private namespace owned by a single run of the test suite. + +Every resource the suite touches — the datastore database, the databases created by the command tests, +the configuration file, the temporary directories — is named after `SESSION_NAME` or nested under +`SESSION_PATH`. Two suites running at the same time therefore never share anything, and whatever a run +leaves behind can be identified and removed by the next one. + +A run holds an exclusive `flock` on its sandbox for its whole lifetime. The kernel releases that lock +when the process dies, whichever way it dies, so a lock that can be taken is proof that its owner is +gone and its leftovers are safe to remove. This is what makes cleanup survive `SIGKILL`, where no +handler of ours can run. +""" + +import shutil +import tempfile +from collections.abc import Iterator +from contextlib import contextmanager +from fcntl import LOCK_EX, LOCK_NB, LOCK_UN, flock +from pathlib import Path +from typing import IO + +import psycopg2 +from psycopg2.extensions import ISOLATION_LEVEL_AUTOCOMMIT, cursor as Cursor + +from odev.common.config import CONFIG_DIR +from odev.common.logging import logging +from odev.common.string import suid + + +logger = logging.getLogger(__name__) + + +SANDBOX_PREFIX = "odev-test" +"""Prefix shared by every sandbox, and the only namespace this module is ever allowed to delete.""" + +SANDBOX_ROOT = Path(tempfile.gettempdir()) +"""Directory holding the sandbox of each run.""" + +SESSION_NAME = f"{SANDBOX_PREFIX}-{suid()}" +"""Name of the sandbox owned by this run, used as the name of the odev framework it drives.""" + +SESSION_PATH = SANDBOX_ROOT / SESSION_NAME +"""Directory holding every file this run creates.""" + +REAL_CONFIG_DIR = CONFIG_DIR +"""The user's actual configuration directory, captured before the test case redirects `CONFIG_DIR`. + +Runs predating this module wrote their `odev-test*` files there; the sweep still cleans them up. +""" + +LOCK_NAME = ".lock" +"""Name of the lock file marking a sandbox as owned by a live process.""" + +MAINTENANCE_DATABASE = "postgres" +"""Database to connect to when listing and dropping the databases of a sandbox.""" + +SESSION_PARTS = 3 +"""Number of dash-separated components in a sandbox name, as in `odev-test-4kq2z81a`.""" + + +_lock: IO[str] | None = None +"""Open handle on this run's lock file, kept for as long as the run lives.""" + + +def acquire() -> None: + """Create the sandbox of this run and hold its lock until the process exits.""" + global _lock # noqa: PLW0603 - the lock lives as long as the module does + + if _lock is not None: + return + + # The sandbox is locked before it is published under its final name, and the rename is atomic: a + # sweep running in another process must never come across a sandbox that does not hold its lock yet, + # or it would take it for the leftovers of a dead run and delete it. + staging = SANDBOX_ROOT / f".{SESSION_NAME}" + staging.mkdir(parents=True, exist_ok=True) + + _lock = (staging / LOCK_NAME).open("w") + flock(_lock, LOCK_EX | LOCK_NB) + staging.rename(SESSION_PATH) + + +def release() -> None: + """Remove everything this run created, then release its lock. + + Safe to call more than once: pytest calls it at the end of the session and `atexit` calls it again if + the interpreter goes down another way. + """ + global _lock + + if _lock is None: + return + + handle, _lock = _lock, None + + try: + discard(SESSION_NAME) + finally: + flock(handle, LOCK_UN) + handle.close() + + +def sweep() -> None: + """Remove the sandboxes of runs that no longer hold their lock. + + Covers whatever escaped `release()`: a suite killed with `SIGKILL`, a crashed interpreter, a machine + that went down mid-run. + """ + # The sandboxes are listed before the live ones are: a run publishing its own between the two + # snapshots is then simply absent from the list, rather than present in it and seemingly unowned. + known = _known_sessions() + live = _live_sessions() + + for session in sorted(known - live): + try: + discard(session) + except Exception as error: # noqa: BLE001 - a sandbox we cannot clean must not fail the suite + logger.warning(f"Could not remove the leftovers of test session {session!r}: {error}") + + +def discard(session: str) -> None: + """Remove every trace of a sandbox: its databases, its configuration files and its directory. + + :param session: The name of the sandbox to remove. + :raises ValueError: If the name falls outside the sandbox namespace. + """ + if session != SANDBOX_PREFIX and not session.startswith(f"{SANDBOX_PREFIX}-"): + raise ValueError(f"Refusing to remove {session!r}, which is not a test sandbox") + + # `odev-test` on its own is the sandbox of runs predating this module. Everything below it belongs to + # other sandboxes, possibly live ones, so only its own name is removed. + owns_children = session != SANDBOX_PREFIX + + for database in _databases(session, children=owns_children): + _drop_database(database) + + for path in REAL_CONFIG_DIR.glob(f"{session}*" if owns_children else f"{session}.*"): + path.unlink(missing_ok=True) + + shutil.rmtree(SANDBOX_ROOT / session, ignore_errors=True) + + +def _live_sessions() -> set[str]: + """Return the sandboxes still owned by a running process.""" + return {path.name for path in _sandbox_directories() if _is_locked(path / LOCK_NAME)} + + +def _known_sessions() -> set[str]: + """Return every sandbox that left a trace on this machine, live or not.""" + sessions = {path.name for path in _sandbox_directories()} + sessions |= {_session_of(path.name) for path in REAL_CONFIG_DIR.glob(f"{SANDBOX_PREFIX}*")} + sessions |= {_session_of(database) for database in _databases(SANDBOX_PREFIX, children=True)} + + return sessions + + +def _sandbox_directories() -> Iterator[Path]: + """Yield the sandbox directories present on this machine.""" + return (path for path in SANDBOX_ROOT.glob(f"{SANDBOX_PREFIX}-*") if path.is_dir()) + + +def _is_locked(path: Path) -> bool: + """Check whether a lock file is held by a live process. + + `flock` conflicts between separate open file descriptions, including within a single process, so this + also reports the sandbox of the current run as live. A missing lock file means the sandbox predates + this module or its owner died before taking the lock: either way nobody owns it. + """ + if not path.exists(): + return False + + try: + with path.open("r") as handle: + flock(handle, LOCK_EX | LOCK_NB) + flock(handle, LOCK_UN) + + except OSError: + return True + + return False + + +def _session_of(artifact: str) -> str: + """Return the sandbox an artifact belongs to. + + Artifacts are named after their sandbox with a suffix of their own, so the sandbox is the first three + components of the name: the database `odev-test-4kq2z81a-9zf1z0aa` and the file + `odev-test-4kq2z81a.cfg` both belong to `odev-test-4kq2z81a`. `suid` only ever emits lowercase letters + and digits, so no component contains a dash of its own. + + :param artifact: The name of a database, or the name of a file including its extension. + :return: The name of the sandbox owning it. + :rtype: str + """ + return "-".join(artifact.split(".")[0].split("-")[:SESSION_PARTS]) + + +def _databases(session: str, children: bool) -> list[str]: + """List the existing databases belonging to a sandbox. + + :param session: The name of the sandbox. + :param children: Whether to also return the databases named after a sandbox nested below it. + :return: The names of the matching databases. + :rtype: list[str] + """ + with _maintenance() as cursor: + if cursor is None: + return [] + + cursor.execute( + "SELECT datname FROM pg_database WHERE datname = %s OR datname LIKE %s", + (session, f"{session}-%" if children else session), + ) + + return [name for (name,) in cursor.fetchall()] + + +def _drop_database(database: str) -> None: + """Drop a database left behind by a dead run, disconnecting whatever still holds it open.""" + with _maintenance() as cursor: + if cursor is None: + return + + cursor.execute( + "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = %s AND pid <> pg_backend_pid()", + (database,), + ) + cursor.execute(f'DROP DATABASE IF EXISTS "{database}"') + + +@contextmanager +def _maintenance() -> Iterator[Cursor | None]: + """Yield a cursor on the maintenance database, or `None` if PostgreSQL cannot be reached. + + Cleaning up is best-effort: a developer without a running PostgreSQL gets a warning rather than a + suite that refuses to start. + """ + try: + connection = psycopg2.connect(database=MAINTENANCE_DATABASE) + except psycopg2.Error as error: + logger.warning(f"Could not connect to PostgreSQL to clean up test sandboxes: {error}") + yield None + + return + + connection.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT) + + try: + yield connection.cursor() + finally: + connection.close() diff --git a/tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql b/tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql new file mode 100644 index 000000000..2ff641944 --- /dev/null +++ b/tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql @@ -0,0 +1,2 @@ +-- Neutralization script shipped by a custom module, used by tests. +SELECT 1; diff --git a/tests/tests/commands/test_database.py b/tests/tests/commands/test_database.py index ade716e42..fe2ca3b57 100644 --- a/tests/tests/commands/test_database.py +++ b/tests/tests/commands/test_database.py @@ -568,11 +568,13 @@ def test_99_delete_expression(self): """Command `odev delete` should delete databases matching a regular expression.""" self.assertDatabaseExist(self.database_name) + # The expression runs against the real PostgreSQL instance, so it has to be scoped to the sandbox + # of this run: a broader one would delete the databases of a suite running alongside this one. with self.patch(self.odev.console, "confirm", return_value=True): stdout, _ = self.dispatch_command( "delete", "--expression", - "^odev-test-[a-z0-9]{8}", + f"^{self.odev.name}-[a-z0-9]{{8}}", "--include-whitelisted", ) diff --git a/tests/tests/commands/test_database_command.py b/tests/tests/commands/test_database_command.py new file mode 100644 index 000000000..3b07ef5da --- /dev/null +++ b/tests/tests/commands/test_database_command.py @@ -0,0 +1,131 @@ +"""Tests for the `odev database` command, which edits a database's parameters without starting it.""" + +from odev.common.databases import LocalDatabase, Repository + +from tests.fixtures import OdevCommandTestCase + + +class TestDatabaseParametersCommand(OdevCommandTestCase): + """`odev database` must go through the database model so that its parameters round-trip.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.database_name = f"{cls.run_name}-params" + cls.database = LocalDatabase(cls.database_name) + cls.database.create() + + @classmethod + def tearDownClass(cls): + # `OdevTestCase` registers its teardown as a class cleanup on top of unittest calling it, + # so this runs twice; by the second time the framework the database needs is already gone. + if cls.database is not None: + cls.database.drop() + cls.database = None + super().tearDownClass() + + def setUp(self): + super().setUp() + self.addCleanup(self.odev.store.databases.delete, self.database) + + def stored(self): + """Return the values saved in the data store for the database under test.""" + return self.odev.store.databases.get(self.database) + + def test_set_repository_persists_the_link(self): + self.dispatch_command("database", self.database_name, "--set-repo", "odoo-ps/psbe-project") + self.assertEqual(self.stored().repository, "odoo-ps/psbe-project") + + def test_set_repository_normalizes_urls(self): + """A URL must be stored as `organization/repository`: the stored value is split on the + first slash when read back, so storing a URL yields a nonsensical repository. + """ + self.dispatch_command("database", self.database_name, "--set-repo", "https://github.com/odoo-ps/psbe-project") + self.assertEqual(self.stored().repository, "odoo-ps/psbe-project") + + def test_set_repository_creates_the_row_when_missing(self): + """The database has never been run under odev, so it has no row in the data store yet.""" + self.assertIsNone(self.stored()) + self.dispatch_command("database", self.database_name, "--set-repo", "odoo-ps/psbe-project") + self.assertIsNotNone(self.stored()) + + def test_remove_repository_clears_the_link(self): + self.dispatch_command("database", self.database_name, "--set-repo", "odoo-ps/psbe-project") + self.dispatch_command("database", self.database_name, "--remove-repo") + self.assertIsNone(self.stored().repository) + + def test_set_and_remove_repository_are_exclusive(self): + _, stderr = self.dispatch_command( + "database", self.database_name, "--set-repo", "odoo-ps/psbe-project", "--remove-repo" + ) + self.assertIn("cannot be used together", stderr) + self.assertIsNone(self.stored()) + + def test_relinking_clears_the_previous_branch(self): + """The branch belongs to the repository, keeping it would attribute it to the new one.""" + self.database.repository = Repository("psbe-project", "odoo-ps") + self.odev.store.databases.set_value(self.database, "branch", "17.0-fix") + self.assertEqual(self.stored().branch, "17.0-fix") + + self.dispatch_command("database", self.database_name, "--set-repo", "odoo-ps/psbe-other") + self.assertEqual(self.stored().repository, "odoo-ps/psbe-other") + self.assertIsNone(self.stored().branch) + + def test_whitelist_and_unwhitelist(self): + self.dispatch_command("database", self.database_name, "--set-repo", "odoo-ps/psbe-project") + self.assertFalse(self.stored().whitelisted) + + self.dispatch_command("database", self.database_name, "--whitelist") + self.assertTrue(self.stored().whitelisted) + + self.dispatch_command("database", self.database_name, "--no-whitelist") + self.assertFalse(self.stored().whitelisted) + + def test_unknown_worktree_is_rejected(self): + """Exercises `GitCommand.worktrees`, which reads an argument this command removes.""" + _, stderr = self.dispatch_command("database", self.database_name, "--set-worktree", "does-not-exist") + self.assertIn("not found", stderr) + + def test_without_arguments_the_parameters_are_printed(self): + self.dispatch_command("database", self.database_name, "--set-repo", "odoo-ps/psbe-project") + stdout, _ = self.dispatch_command("database", self.database_name) + self.assertIn("odoo-ps/psbe-project", stdout) + self.assertIn("Whitelisted", stdout) + + +class TestDatabaseLinkRepository(OdevCommandTestCase): + """`LocalDatabase.link_repository` is the shared entry point for linking a repository.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.database = LocalDatabase(f"{cls.run_name}-link") + cls.database.create() + + @classmethod + def tearDownClass(cls): + # `OdevTestCase` registers its teardown as a class cleanup on top of unittest calling it, + # so this runs twice; by the second time the framework the database needs is already gone. + if cls.database is not None: + cls.database.drop() + cls.database = None + super().tearDownClass() + + def setUp(self): + super().setUp() + self.addCleanup(self.odev.store.databases.delete, self.database) + + def test_accepts_a_repository_name(self): + self.assertEqual(self.database.link_repository("odoo-ps/psbe-project"), Repository("psbe-project", "odoo-ps")) + + def test_accepts_an_ssh_url(self): + linked = self.database.link_repository("git@github.com:odoo-ps/psbe-project.git") + self.assertEqual(linked, Repository("psbe-project", "odoo-ps")) + + def test_accepts_a_repository_instance(self): + linked = self.database.link_repository(Repository("psbe-project", "odoo-ps")) + self.assertEqual(linked, Repository("psbe-project", "odoo-ps")) + + def test_none_unlinks(self): + self.database.link_repository("odoo-ps/psbe-project") + self.assertIsNone(self.database.link_repository(None)) diff --git a/tests/tests/commands/test_git_and_scripts.py b/tests/tests/commands/test_git_and_scripts.py index d3d25630e..5b8feda39 100644 --- a/tests/tests/commands/test_git_and_scripts.py +++ b/tests/tests/commands/test_git_and_scripts.py @@ -1,6 +1,7 @@ from argparse import Namespace from unittest.mock import MagicMock +from odev.commands.git.pull import PullCommand from odev.commands.scripts.assets import PathfinderCommand as AssetsCommand from odev.commands.scripts.pathfinder import PathfinderCommand from odev.common.connectors.git import GitConnector @@ -33,6 +34,28 @@ def test_04_worktree_name_required_for_create(self): _, stderr = self.dispatch_command("worktree", "--create") self.assertIn("provide a name for the worktree", stderr) + def test_05_pull_output_ends_with_blank_line(self): + """`FetchCommand.run` clears the line trailing the last worktree, so `run_hook` must end on a blank one.""" + worktree = MagicMock(name="17.0", detached=False, branch="17.0") + worktree.name = "17.0" + worktree.connector.name = "odoo/odoo" + + command = PullCommand.__new__(PullCommand) + printed: list[str] = [] + titled: list[str] = [] + + with ( + self.patch_property(PullCommand, "worktrees", [worktree]), + self.patch( + self.odev.console, "print", side_effect=lambda renderable="", *_, **__: printed.append(renderable) + ), + self.patch(self.odev.console, "title_rule", side_effect=titled.append), + ): + command.run_hook("17.0", [("odoo/odoo", 0, 0)]) + + self.assertEqual(titled, ["17.0"]) + self.assertEqual(printed, [""]) + class TestScriptCommands(OdevTestCase): def test_01_assets_script_run_after(self): diff --git a/tests/tests/commands/test_quickstart.py b/tests/tests/commands/test_quickstart.py new file mode 100644 index 000000000..ff79fc4ea --- /dev/null +++ b/tests/tests/commands/test_quickstart.py @@ -0,0 +1,96 @@ +from argparse import Namespace +from pathlib import Path +from unittest.mock import MagicMock + +from odev.commands.database.quickstart import QuickStartCommand +from odev.common.databases import LocalDatabase, Repository + +from tests.fixtures import OdevTestCase + + +class TestQuickStartLinksRepository(OdevTestCase): + """The repository must be linked before the dump is restored, and still be linked afterwards.""" + + def make_command(self, repository: Repository | None) -> QuickStartCommand: + command = QuickStartCommand.__new__(QuickStartCommand) + command._framework = self.odev + command.args = Namespace(branch=None, version=None, name="quickstart-target", filestore=False) + + source = MagicMock() + source.name = "quickstart-source" + source.repository = repository + source.platform.name = "remote" + source._get_dump_filename.return_value = "dump.zip" + command._database = source + return command + + def make_dump_file(self) -> Path: + dump_file = self.run_path / "dump.zip" + dump_file.parent.mkdir(parents=True, exist_ok=True) + dump_file.touch() + return dump_file + + def test_repository_is_linked_before_restore(self): + """Regression for #98: neutralization runs from within `restore`, so a repository linked + only after the restore call is invisible to it. + """ + repository = Repository("psbe-project", "odoo-ps") + command = self.make_command(repository) + target = MagicMock(spec=LocalDatabase) + target.repository = None + seen: list[Repository | None] = [] + + def fake_run_command(name, *_args, database=None, **_kwargs): + if name == "restore": + seen.append(database.repository) + return True + + dump_file = self.make_dump_file() + + with ( + self.patch(self.odev, "run_command", side_effect=fake_run_command), + self.patch_property(type(self.odev), "dumps_path", dump_file.parent), + self.patch("odev.commands.database.quickstart", "LocalDatabase", return_value=target), + ): + command.run() + + self.assertEqual(seen, [Repository("psbe-project", "odoo-ps")]) + + def test_repository_is_still_linked_after_restore(self): + """`restore` drops and recreates the database, which clears its entry in the data store.""" + repository = Repository("psbe-project", "odoo-ps") + command = self.make_command(repository) + target = MagicMock(spec=LocalDatabase) + target.repository = None + + def fake_run_command(name, *_args, database=None, **_kwargs): + if name == "restore": + database.repository = None # `restore` wipes the store entry + return True + + dump_file = self.make_dump_file() + + with ( + self.patch(self.odev, "run_command", side_effect=fake_run_command), + self.patch_property(type(self.odev), "dumps_path", dump_file.parent), + self.patch("odev.commands.database.quickstart", "LocalDatabase", return_value=target), + ): + command.run() + + self.assertEqual(target.repository, Repository("psbe-project", "odoo-ps")) + + def test_no_repository_on_source_links_nothing(self): + command = self.make_command(None) + target = MagicMock(spec=LocalDatabase) + target.repository = None + + dump_file = self.make_dump_file() + + with ( + self.patch(self.odev, "run_command", return_value=True), + self.patch_property(type(self.odev), "dumps_path", dump_file.parent), + self.patch("odev.commands.database.quickstart", "LocalDatabase", return_value=target), + ): + command.run() + + self.assertIsNone(target.repository) diff --git a/tests/tests/commands/test_utilities.py b/tests/tests/commands/test_utilities.py index 7974c2a77..bad4f871f 100644 --- a/tests/tests/commands/test_utilities.py +++ b/tests/tests/commands/test_utilities.py @@ -1,5 +1,8 @@ +import os import shutil from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch from odev._version import __version__ from odev.common.python import PythonEnv @@ -9,13 +12,28 @@ POSTGRES_PATH = "odev.common.connectors.PostgresConnector" GIT_PATH = "odev.common.connectors.git.GitConnector" +GITHUB_PATH = "odev.common.connectors.git.GithubConnector" class TestCommandUtilities(OdevCommandTestCase): def test_version_01_no_argument(self): - """Command `odev version` should print the version of the application.""" - stdout, _ = self.dispatch_command("version") - self.assertIn(f"Odev-test version {__version__}", stdout) + """Command `odev version` should print the version of the application and stay silent when there is + nothing new to pull. + """ + with self.patch(self.odev, "update_available", return_value=False): + stdout, stderr = self.dispatch_command("version") + + self.assertIn(f"{self.odev.name.capitalize()} version {__version__}", stdout) + self.assertNotIn("A newer version is available", stderr) + + def test_version_02_update_available(self): + """Command `odev version` should warn about a newer version only when the repository has incoming + changes. + """ + with self.patch(self.odev, "update_available", return_value=True): + _, stderr = self.dispatch_command("version") + + self.assertIn("A newer version is available", stderr) def test_config_01_no_argument(self): """Run the command without arguments.""" @@ -169,7 +187,10 @@ def test_update_01_no_argument(self): def upgrade(): self.odev.config.update.version = __version__ - with self.patch(self.odev, "upgrade", side_effect=upgrade): + with ( + self.patch_property(type(self.odev), "version", "3.0.0"), + self.patch(self.odev, "upgrade", side_effect=upgrade), + ): stdout, _ = self.dispatch_command("update") self.assertEqual(self.odev.config.update.version, __version__) @@ -240,6 +261,221 @@ def test_plugin_03_enable_dependencies(self): self.assertFalse(self.odev._plugin_is_installed(plugin)) self.assertFalse(self.odev._plugin_is_installed(dependent)) + def test_plugin_04_list(self): + """Run the command to list plugins, showing enabled and downloaded ones alike.""" + self.__enable_test_plugin() + stdout, _ = self.__dispatch_plugin("--list") + + self.assertRegex(stdout, r"Plugin\s+Version\s+Branch\s+State\s+Depends") + self.assertRegex(stdout, r"test/test-plugin\s+1\.0\.0\s+enabled") + self.assertRegex(stdout, r"test/test-plugin-dep\s+1\.0\.0\s+disabled\s+test-plugin") + self.assertNotIn("test/test-addons", stdout) + + def test_plugin_05_list_shadowed(self): + """Run the command to list plugins when two enabled plugins share the same module name.""" + self.__enable_test_plugin() + self.odev.config.plugins.enabled = [*self.odev.config.plugins.enabled, "other/test-plugin"] + + stdout, _ = self.__dispatch_plugin("--list") + + self.assertRegex(stdout, r"test/test-plugin\s+1\.0\.0\s+enabled") + self.assertRegex(stdout, r"other/test-plugin\s+shadowed") + self.assertIn("other/test-plugin (shadowed by test/test-plugin)", stdout) + + def test_plugin_06_search(self): + """Run the command to search plugins on GitHub, keeping only valid plugin repositories.""" + self.odev.config.paths.repositories = self.res_path / "repositories" + manifest = (self.res_path / "repositories" / "test" / "test-plugin" / "__manifest__.py").read_text() + repositories = [ + self.__github_repository("test/archived-plugin", archived=True), + self.__github_repository("odoo-odev/odev-plugin-template"), + self.__github_repository("test/test-plugin", stars=3), + self.__github_repository("other/remote-plugin"), + self.__github_repository("other/not-a-plugin", description="Not a plugin at all"), + ] + + with ( + self.patch(GITHUB_PATH, "search_repositories", return_value=repositories) as search, + self.patch(GITHUB_PATH, "get_repository_file", side_effect=[manifest, manifest, None]), + ): + stdout, _ = self.__dispatch_plugin("--search") + + self.assertRegex(stdout, r"Plugin\s+Version\s+Stars\s+State\s+Description") + self.assertRegex(stdout, r"test/test-plugin\s+1\.0\.0\s+3\s+disabled") + self.assertRegex(stdout, r"other/remote-plugin\s+1\.0\.0\s+0\s+not downloaded") + self.assertNotIn("other/not-a-plugin", stdout) + self.assertNotIn("test/archived-plugin", stdout) + self.assertNotIn("odev-plugin-template", stdout) + self.assertTrue(search.call_args.args[0].startswith("odev plugin")) + + def test_plugin_07_search_terms(self): + """Run the command to search plugins with additional terms, showing the state of enabled plugins.""" + self.__enable_test_plugin() + manifest = (self.res_path / "repositories" / "test" / "test-plugin" / "__manifest__.py").read_text() + + with ( + self.patch( + GITHUB_PATH, + "search_repositories", + return_value=[self.__github_repository("test/test-plugin")], + ) as search, + self.patch(GITHUB_PATH, "get_repository_file", return_value=manifest), + ): + stdout, _ = self.__dispatch_plugin("--search", "editor") + + self.assertRegex(stdout, r"test/test-plugin\s+1\.0\.0\s+0\s+enabled") + self.assertIn("editor", search.call_args.args[0]) + + def test_plugin_08_search_no_result(self): + """Run the command to search plugins when no repository exposes a valid manifest.""" + self.odev.config.paths.repositories = self.res_path / "repositories" + + with ( + self.patch( + GITHUB_PATH, + "search_repositories", + return_value=[self.__github_repository("other/not-a-plugin")], + ), + self.patch(GITHUB_PATH, "get_repository_file", return_value=None), + ): + _, stderr = self.dispatch_command("plugin", "--search", "unknown") + + self.assertIn("No odev plugin found matching 'unknown'", stderr) + + def test_plugin_09_show_qualified_name(self): + """Run the command to show a plugin using its fully qualified name.""" + self.__enable_test_plugin() + + stdout, _ = self.dispatch_command("plugin", "--show", "test/test-plugin") + self.assertIn("Plugin 'test/test-plugin' is enabled", stdout) + + stdout, _ = self.dispatch_command("plugin", "--show", "test-plugin") + self.assertIn("Plugin 'test/test-plugin' is enabled", stdout) + + def test_plugin_10_show_downloaded(self): + """Run the command to show a plugin available locally but not enabled, without querying GitHub.""" + self.__enable_test_plugin() + + with self.patch(GITHUB_PATH, "get_repository", return_value=None) as get_repository: + stdout, _ = self.__dispatch_plugin("--show", "test/test-plugin-dep") + + self.assertIn("Plugin 'test/test-plugin-dep' is disabled", stdout) + self.assertRegex(stdout, r"Version:\s+1\.0\.0") + self.assertRegex(stdout, r"Depends:\s+test/test-plugin") + self.assertRegex(stdout, r"Path:\s+.*test-plugin-dep") + self.assertRegex(stdout, r"URL:\s+https://github\.com/test/test-plugin-dep") + get_repository.assert_not_called() + + def test_plugin_11_show_not_downloaded(self): + """Run the command to show a plugin that is not available locally, fetching its manifest from GitHub.""" + self.odev.config.paths.repositories = self.res_path / "repositories" + manifest = (self.res_path / "repositories" / "test" / "test-plugin" / "__manifest__.py").read_text() + + with ( + self.patch( + GITHUB_PATH, + "get_repository", + return_value=self.__github_repository("other/remote-plugin", stars=42), + ) as get_repository, + self.patch(GITHUB_PATH, "get_repository_file", return_value=manifest) as get_repository_file, + ): + stdout, _ = self.__dispatch_plugin("--show", "other/remote-plugin") + + self.assertIn("Plugin 'other/remote-plugin' is not downloaded", stdout) + self.assertRegex(stdout, r"Version:\s+1\.0\.0") + self.assertRegex(stdout, r"Branch:\s+main") + self.assertRegex(stdout, r"Stars:\s+42") + self.assertNotIn("Path:", stdout) + self.assertIn("Run 'odev plugin --enable other/remote-plugin' to install this plugin", stdout) + get_repository.assert_called_once_with("other/remote-plugin") + self.assertEqual(get_repository_file.call_args.args[1], "__manifest__.py") + + def test_plugin_12_show_archived_and_excluded(self): + """Run the command to show plugins that can be found on GitHub but should not be installed.""" + self.odev.config.paths.repositories = self.res_path / "repositories" + manifest = (self.res_path / "repositories" / "test" / "test-plugin" / "__manifest__.py").read_text() + + with ( + self.patch( + GITHUB_PATH, + "get_repository", + return_value=self.__github_repository("other/archived-plugin", archived=True), + ), + self.patch(GITHUB_PATH, "get_repository_file", return_value=manifest), + ): + stdout, _ = self.__dispatch_plugin("--show", "other/archived-plugin") + + self.assertIn("Repository 'other/archived-plugin' is archived", stdout) + + with ( + self.patch( + GITHUB_PATH, + "get_repository", + return_value=self.__github_repository("odoo-odev/odev-plugin-template"), + ), + self.patch(GITHUB_PATH, "get_repository_file", return_value=manifest), + ): + stdout, _ = self.__dispatch_plugin("--show", "odoo-odev/odev-plugin-template") + + self.assertIn("is a template used to create new plugins and cannot be installed", stdout) + self.assertNotIn("--enable odoo-odev/odev-plugin-template", stdout) + + def test_plugin_13_show_unknown(self): + """Run the command to show a plugin that is neither available locally nor on GitHub.""" + self.odev.config.paths.repositories = self.res_path / "repositories" + + with self.patch(GITHUB_PATH, "get_repository", return_value=None): + stdout, _ = self.__dispatch_plugin("--show", "other/unknown-plugin") + + self.assertIn("Plugin 'other/unknown-plugin' is not downloaded", stdout) + self.assertNotIn("Version:", stdout) + + with self.patch(GITHUB_PATH, "get_repository", return_value=None) as get_repository: + stdout, _ = self.__dispatch_plugin("--show", "unknown-plugin") + + self.assertIn("Plugin 'unknown-plugin' is not downloaded", stdout) + self.assertIn("Use the full name of the plugin", stdout) + get_repository.assert_not_called() + + def test_plugin_14_show_all(self): + """Run the command to show all plugins available locally, enabled or not.""" + self.__enable_test_plugin() + + with self.patch(GITHUB_PATH, "get_repository", return_value=None) as get_repository: + stdout, _ = self.__dispatch_plugin("--show") + + self.assertIn("Plugin 'test/test-plugin' is enabled", stdout) + self.assertIn("Plugin 'test/test-plugin-dep' is disabled", stdout) + get_repository.assert_not_called() + + def __dispatch_plugin(self, *arguments: str) -> tuple[str, str]: + """Run the plugin command on a wide terminal so table columns are not cropped.""" + with patch.dict(os.environ, {"COLUMNS": "200"}): + return self.dispatch_command("plugin", *arguments) + + def __enable_test_plugin(self): + """Enable the test plugin from the local test resources and unlink it after the test.""" + self.odev.config.paths.repositories = self.res_path / "repositories" + plugin_link = Path(self.odev.plugins_path) / "test_plugin" + self.addCleanup(plugin_link.unlink, missing_ok=True) + + with ( + self.patch_property(GIT_PATH, "exists", value=True), + self.patch(GIT_PATH, "update"), + ): + self.dispatch_command("plugin", "--enable", "test/test-plugin") + + def __github_repository(self, full_name: str, description: str = "", stars: int = 0, archived: bool = False): + """Build a stand-in for a repository as returned by the GitHub API.""" + return SimpleNamespace( + full_name=full_name, + html_url=f"https://github.com/{full_name}", + description=description, + stargazers_count=stars, + archived=archived, + default_branch="main", + ) + class TestCommandUtilitiesVenv(OdevCommandTestCase): @classmethod diff --git a/tests/tests/common/test_bash.py b/tests/tests/common/test_bash.py index 8c2ecda36..2f5f66105 100644 --- a/tests/tests/common/test_bash.py +++ b/tests/tests/common/test_bash.py @@ -7,6 +7,9 @@ from tests.fixtures import OdevTestCase +PRIVILEGED_COMMAND = "cat /etc/shadow" + + class TestCommonBash(OdevTestCase): """Test the execution of system commands.""" @@ -26,37 +29,143 @@ def test_03_invalid_command_no_raise(self): exec_result = bash.execute("notacommand", raise_on_error=False) self.assertIsNone(exec_result) - def test_04_sudo_no_password(self): - """A command that fails should be re-executed with sudo and fail if the password is not set.""" - with self.assertRaises(CalledProcessError), self.patch(console, "secret") as mock_secret: - mock_secret.return_value = None - bash.execute("cat /etc/shadow", sudo=True) + def test_04_detached(self): + """A command that is run in detached mode should not block the program.""" + start = monotonic() + bash.detached("sleep 1") + self.assertLess(monotonic() - start, 1) + + +class TestCommonBashSudo(OdevTestCase): + """Elevation should be attempted once a command fails, and only with a password at hand. + + The subprocess and the effective user are both simulated: shelling out to `sudo` would make the result + depend on the local sudoers policy, and asking for a real elevation from a test suite is not something + a developer should have to accept. A machine granting passwordless sudo would run the privileged + command for real, and running the suite as root skips elevation entirely. + """ + + def setUp(self): + super().setUp() + self.addCleanup(self.restore_sudo_password) + self.commands: list[str] = [] + + def restore_sudo_password(self): + """Clear the session password cached in the module by the sudo tests.""" + bash.sudo_password = None - def test_05_sudo_no_password_no_raise(self): - """A command that fails should be re-executed with sudo and return None if the password is not set - and raise_on_error is False. + def failure(self, command: str) -> CalledProcessError: + """Build the error raised by a command the user is not allowed to run.""" + return CalledProcessError(1, command, output=b"", stderr=b"Permission denied") + + def patch_subprocess(self, sudo_succeeds: bool = False): + """Patch the subprocess call, recording commands and failing until sudo is used. + + :param sudo_succeeds: Whether the elevated command should succeed instead of failing again. """ - with self.patch(console, "secret", None): - exec_result = bash.execute("cat /etc/shadow", sudo=True, raise_on_error=False) + + def run(command: str, **kwargs): + self.commands.append(command) + + if sudo_succeeds and command.startswith("sudo "): + return CompletedProcess(command, 0, stdout=b"elevated", stderr=b"") + + raise self.failure(command) + + return self.patch(bash, "run_subprocess", side_effect=run) + + def patch_unprivileged_user(self): + """Pretend the suite runs as a regular user, so the elevation path is taken.""" + return self.patch(bash.os, "geteuid", return_value=1000) + + def test_01_password_is_asked_once_the_command_failed(self): + """The session password should only be requested after a first, unprivileged attempt.""" + with ( + self.patch_subprocess(), + self.patch_unprivileged_user(), + self.patch(console, "secret", return_value="secret") as mock_secret, + self.assertRaises(CalledProcessError), + ): + bash.execute(PRIVILEGED_COMMAND, sudo=True) + + mock_secret.assert_called_once() + self.assertEqual(self.commands, [PRIVILEGED_COMMAND, f"sudo -Sks {PRIVILEGED_COMMAND}"]) + + def test_02_no_password_does_not_elevate(self): + """Without a password there is nothing to elevate with, and the original error should surface.""" + with ( + self.patch_subprocess(), + self.patch_unprivileged_user(), + self.patch(console, "secret", return_value=None), + self.assertRaises(CalledProcessError), + ): + bash.execute(PRIVILEGED_COMMAND, sudo=True) + + self.assertEqual(self.commands, [PRIVILEGED_COMMAND]) + + def test_03_no_password_no_raise(self): + """A command failing without a password should return None when not raising.""" + with ( + self.patch_subprocess(), + self.patch_unprivileged_user(), + self.patch(console, "secret", return_value=None), + ): + exec_result = bash.execute(PRIVILEGED_COMMAND, sudo=True, raise_on_error=False) + self.assertIsNone(exec_result) - def test_06_sudo_wrong_password(self): - """A command that fails should be re-executed with sudo and fail again if the password is wrong.""" + def test_04_cached_password_is_reused(self): + """A password from an earlier command should be reused rather than asked again.""" + bash.sudo_password = "cached" # noqa: S105 + + with ( + self.patch_subprocess(sudo_succeeds=True), + self.patch_unprivileged_user(), + self.patch(console, "secret") as mock_secret, + ): + exec_result = bash.execute(PRIVILEGED_COMMAND, sudo=True) + + mock_secret.assert_not_called() + + if exec_result is None: + self.fail("the elevated command should have returned a result") + + self.assertEqual(exec_result.stdout, b"elevated") + + def test_05_wrong_password_raises(self): + """A password rejected by sudo should let the second failure surface.""" bash.sudo_password = "wrongpassword" # noqa: S105 - with self.assertRaises(CalledProcessError): - bash.execute("cat >> /etc/shadow", sudo=True) + with self.patch_subprocess(), self.patch_unprivileged_user(), self.assertRaises(CalledProcessError): + bash.execute(PRIVILEGED_COMMAND, sudo=True) - def test_07_sudo_wrong_password_no_raise(self): - """A command that fails should be re-executed with sudo and return None if the password is wrong - and raise_on_error is False. - """ + def test_06_wrong_password_no_raise(self): + """A password rejected by sudo should return None when not raising.""" bash.sudo_password = "wrongpassword" # noqa: S105 - exec_result = bash.execute("cat >> /etc/shadow", sudo=True, raise_on_error=False) + + with self.patch_subprocess(), self.patch_unprivileged_user(): + exec_result = bash.execute(PRIVILEGED_COMMAND, sudo=True, raise_on_error=False) + self.assertIsNone(exec_result) - def test_08_detached(self): - """A command that is run in detached mode should not block the program.""" - start = monotonic() - bash.detached("sleep 1") - self.assertLess(monotonic() - start, 1) + def test_07_wrong_password_is_forgotten(self): + """A rejected password should not be kept and asked again on the next command.""" + bash.sudo_password = "wrongpassword" # noqa: S105 + + with self.patch_subprocess(), self.patch_unprivileged_user(): + bash.execute(PRIVILEGED_COMMAND, sudo=True, raise_on_error=False) + + self.assertIsNone(bash.sudo_password) + + def test_08_root_does_not_elevate(self): + """Running as root already has the privileges, sudo would add nothing.""" + with ( + self.patch_subprocess(), + self.patch(bash.os, "geteuid", return_value=0), + self.patch(console, "secret") as mock_secret, + ): + exec_result = bash.execute(PRIVILEGED_COMMAND, sudo=True, raise_on_error=False) + + self.assertIsNone(exec_result) + mock_secret.assert_not_called() + self.assertEqual(self.commands, [PRIVILEGED_COMMAND]) diff --git a/tests/tests/common/test_connectors.py b/tests/tests/common/test_connectors.py index c9a3d5d41..db5f9b8c8 100644 --- a/tests/tests/common/test_connectors.py +++ b/tests/tests/common/test_connectors.py @@ -5,6 +5,7 @@ from odev.common.connectors.postgres import Cursor, PostgresConnector from odev.common.connectors.rest import RestConnector +from odev.common.postgres import PostgresDatabase from tests.fixtures import OdevTestCase @@ -106,3 +107,69 @@ def execute(self, statement): raise RuntimeError("boom") self.assertEqual(cursor.calls, ["BEGIN", "ROLLBACK"]) + + +class TestPostgresConnectionLifecycle(OdevTestCase): + """A block has to close the connection it opened, and nested blocks have to share one. + + `ensure_connected` wraps every method of a database in `with self:`, so anything less means a fresh + PostgreSQL backend per call: enough of them at once and the server runs out of connection slots. + """ + + def setUp(self): + super().setUp() + self.database = PostgresDatabase(self.odev.name) + """A handle on the datastore database, connected and disconnected by the tests.""" + + def backends(self, name: str) -> int: + """Count the backends PostgreSQL currently holds for a database.""" + with PostgresConnector() as psql, psql.nocache(): + result = psql.query(f"SELECT count(*) FROM pg_stat_activity WHERE datname = '{name}'") + + return result[0][0] if isinstance(result, list) else 0 + + def test_01_block_closes_what_it_opened(self): + """Leaving a block should disconnect the connector the block connected.""" + with self.database: + self.assertTrue(self.database.connector.connected) + + self.assertFalse(self.database.connector.connected) + + def test_02_nested_blocks_share_one_connection(self): + """An inner block should join the connection of the outer one instead of opening its own.""" + with self.database: + connector = self.database.connector + + with self.database: + self.assertIs(self.database.connector, connector, "the inner block should reuse the connector") + + self.assertTrue(connector.connected, "the inner block should not close what the outer one uses") + + self.assertFalse(connector.connected, "the outermost block should close it") + + def test_03_repeated_calls_do_not_pile_up_backends(self): + """Decorated methods each open a block, and those should not accumulate connections.""" + baseline = self.backends(self.database.name) + + for _ in range(20): + self.database.table_exists("history") + + self.assertLessEqual( + self.backends(self.database.name), + baseline, + "connections opened by the calls should have been closed again", + ) + + def test_04_store_keeps_a_single_connection(self): + """The datastore is read by every command and holds its connection rather than reopening it.""" + connector = self.odev.store.connector + self.assertTrue(connector.connected, "the store should be connected as soon as it exists") + + backends = self.backends(self.odev.store.name) + + for _ in range(20): + self.odev.store.table_exists("history") + + self.assertIs(self.odev.store.connector, connector, "the store should keep the same connector") + self.assertTrue(connector.connected, "a block should not close the connection the store holds") + self.assertEqual(self.backends(self.odev.store.name), backends, "the store should hold a single backend") diff --git a/tests/tests/common/test_git_connector.py b/tests/tests/common/test_git_connector.py index f91e7b9fc..e59b35366 100644 --- a/tests/tests/common/test_git_connector.py +++ b/tests/tests/common/test_git_connector.py @@ -1,9 +1,39 @@ -from odev.common.connectors.git import GitConnector +import shutil +import tempfile +from pathlib import Path +from types import SimpleNamespace + +from git import Repo +from github import GithubException, UnknownObjectException + +from odev.common.connectors.git import GitConnector, GithubConnector from odev.common.errors import ConnectorError +from odev.common.odoobin import OdoobinProcess from tests.fixtures import OdevTestCase +class GitRepositoryMixin: + """Build throwaway git repositories on disk to exercise the connector against real remotes.""" + + def make_repository(self, *parts: str, remote: str | None = "git@github.com:acme/myrepo.git") -> Path: + """Create a git repository in a temporary directory. + :param parts: The directories to nest the repository into, relative to the temporary directory. + :param remote: The URL of the remote to configure, or None to leave the repository without one. + :return: The path to the repository. + """ + root = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + path = root.joinpath(*parts) + path.mkdir(parents=True) + repository = Repo.init(path) + + if remote is not None: + repository.create_remote("origin", remote) + + return path + + class TestGitConnectorInit(OdevTestCase): def test_https_github_url(self): g = GitConnector("https://github.com/acme/myrepo") @@ -25,3 +55,112 @@ def test_invalid_repo_format_raises(self): with self.assertRaises(ConnectorError) as ctx: GitConnector("onlyonepart") self.assertIn("Invalid repository format", str(ctx.exception)) + + +class TestGithubConnectorRepositories(OdevTestCase): + def __connector(self, get_repo) -> GithubConnector: + """Build a connector already connected to a stand-in of the Github API.""" + connector = GithubConnector() + connector._connection = SimpleNamespace(get_repo=get_repo) # type: ignore [assignment] + + for attribute in ("connect", "disconnect"): + patcher = self.patch(connector, attribute) + patcher.start() + self.addCleanup(patcher.stop) + + return connector + + def test_get_repository(self): + repository = SimpleNamespace(full_name="acme/myrepo") + connector = self.__connector(lambda name: repository) + self.assertIs(connector.get_repository("acme/myrepo"), repository) + + def test_get_repository_missing(self): + def get_repo(name: str): + raise UnknownObjectException(404, None, None) + + connector = self.__connector(get_repo) + self.assertIsNone(connector.get_repository("acme/missing")) + + def test_get_repository_error(self): + def get_repo(name: str): + raise GithubException(500, None, None) + + connector = self.__connector(get_repo) + self.assertIsNone(connector.get_repository("acme/myrepo")) + + +class TestGitConnectorName(GitRepositoryMixin, OdevTestCase): + """The repository name must come from the git remote whenever one is available.""" + + def test_name_from_remote_overrides_directory_names(self): + """Regression for #92: a repository cloned outside of the `/` + convention must be named after its remote, not after the directories it lives in. + """ + path = self.make_repository("dev", "tutorials", remote="git@github.com:jlom/tutorials.git") + connector = GitConnector("dev/tutorials", path) + self.assertEqual(connector.name, "jlom/tutorials") + self.assertEqual(connector.path, path) + + def test_name_from_https_remote(self): + path = self.make_repository("myrepo", remote="https://github.com/acme/myrepo.git") + self.assertEqual(GitConnector("whatever/myrepo", path).name, "acme/myrepo") + + def test_name_from_non_origin_remote(self): + path = self.make_repository("myrepo", remote=None) + Repo(path).create_remote("upstream", "git@github.com:acme/upstreamed.git") + self.assertEqual(GitConnector("whatever/myrepo", path).name, "acme/upstreamed") + + def test_origin_remote_wins_over_others(self): + path = self.make_repository("myrepo", remote="git@github.com:acme/origin-repo.git") + Repo(path).create_remote("upstream", "git@github.com:other/upstream-repo.git") + self.assertEqual(GitConnector("whatever/myrepo", path).name, "acme/origin-repo") + + def test_fallback_to_repo_argument_without_remote(self): + path = self.make_repository("myrepo", remote=None) + self.assertEqual(GitConnector("acme/myrepo", path).name, "acme/myrepo") + + def test_fallback_to_repo_argument_on_unparseable_remote(self): + path = self.make_repository("myrepo", remote="myrepo") + self.assertEqual(GitConnector("acme/myrepo", path).name, "acme/myrepo") + + def test_absolute_path_without_remote_uses_directory_names(self): + path = self.make_repository("acme", "myrepo", remote=None) + self.assertEqual(GitConnector(path.as_posix()).name, "acme/myrepo") + + def test_path_that_is_not_a_repository_parses_the_name(self): + root = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + connector = GitConnector("acme/myrepo", root) + self.assertEqual(connector.name, "acme/myrepo") + self.assertEqual(connector.path, root) + + +class TestGitConnectorPath(GitRepositoryMixin, OdevTestCase): + def test_path_defaults_to_the_configured_repositories_directory(self): + connector = GitConnector("acme/myrepo") + self.assertEqual(connector.path, self.odev.config.paths.repositories / "acme/myrepo") + + def test_explicit_path_is_used_as_is(self): + path = self.make_repository("myrepo") + self.assertEqual(GitConnector("acme/myrepo", path).path, path) + + +class TestOdoobinAdditionalRepositories(GitRepositoryMixin, OdevTestCase): + """End-to-end guard for #92 at the level that persists the repository name.""" + + def test_additional_repositories_resolve_to_the_real_directory(self): + path = self.make_repository("dev", "tutorials", remote="git@github.com:jlom/tutorials.git") + module = path / "my_module" + module.mkdir() + (module / "__init__.py").touch() + (module / "__manifest__.py").write_text("{'name': 'My Module'}", encoding="utf-8") + + self.odev.config.paths.repositories = path.parent + + process = OdoobinProcess.__new__(OdoobinProcess) + process._additional_addons_paths = [path] + + repository = next(process.additional_repositories) + self.assertEqual(repository.name, "jlom/tutorials") + self.assertEqual(repository.path, path, "the repositories root must not be duplicated in the path") diff --git a/tests/tests/common/test_git_worktree.py b/tests/tests/common/test_git_worktree.py new file mode 100644 index 000000000..e74fa76ce --- /dev/null +++ b/tests/tests/common/test_git_worktree.py @@ -0,0 +1,251 @@ +from pathlib import Path +from unittest.mock import MagicMock + +from git.exc import GitCommandError + +from odev.common.connectors.git import GitWorktree + +from tests.fixtures import OdevTestCase + + +WORKTREES_PATH = Path("/home/user/.local/share/odev/worktrees") +COMMIT = "0123456789abcdef0123456789abcdef01234567" + + +def porcelain(worktree: str, commit: str = COMMIT, *attributes: str) -> str: + """Build an entry as emitted by `git worktree list --porcelain`. + + :param worktree: The path of the worktree. + :param commit: The commit the worktree points to. + :param attributes: The trailing attribute lines, such as `branch refs/heads/17.0` or `detached`. + :return: The porcelain entry. + :rtype: str + """ + return "\n".join([f"worktree {worktree}", f"HEAD {commit}", *attributes]) + "\n" + + +class TestGitWorktreeParse(OdevTestCase): + """`git worktree list --porcelain` entries should be parsed into worktree objects.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.connector = MagicMock() + cls.connector.name = "odoo/odoo" + cls.connector.odev.worktrees_path = WORKTREES_PATH + + def parse(self, *arguments: str) -> GitWorktree: + """Parse a porcelain entry against the mocked connector.""" + return GitWorktree.parse(self.connector, porcelain(*arguments)) + + def test_01_branch(self): + """A worktree checked out on a branch should expose its path, commit and branch.""" + worktree = self.parse(f"{WORKTREES_PATH}/17.0/odoo", COMMIT, "branch refs/heads/17.0") + + self.assertEqual(worktree.path, WORKTREES_PATH / "17.0" / "odoo") + self.assertEqual(worktree.commit, COMMIT) + self.assertEqual(worktree.ref, "refs/heads/17.0") + self.assertEqual(worktree.branch, "17.0") + self.assertFalse(worktree.detached) + self.assertFalse(worktree.bare) + self.assertFalse(worktree.locked) + self.assertFalse(worktree.prunable) + + def test_02_detached(self): + """A detached worktree should be flagged as such and have no branch.""" + worktree = self.parse(f"{WORKTREES_PATH}/16.0/odoo", COMMIT, "detached") + + self.assertTrue(worktree.detached) + self.assertIsNone(worktree.branch) + self.assertIsNone(worktree.ref) + + def test_03_bare(self): + """A bare repository should be flagged as such.""" + worktree = self.parse("/home/user/repositories/odoo/odoo", COMMIT, "bare") + + self.assertTrue(worktree.bare) + self.assertFalse(worktree.detached) + + def test_04_locked_with_reason(self): + """A locked worktree should keep the reason given to `git worktree lock`.""" + worktree = self.parse(f"{WORKTREES_PATH}/15.0/odoo", COMMIT, "branch refs/heads/15.0", "locked on a usb drive") + + self.assertTrue(worktree.locked) + self.assertEqual(worktree.locked_reason, "on a usb drive") + + def test_05_locked_without_reason(self): + """A worktree may be locked without an explanation.""" + worktree = self.parse(f"{WORKTREES_PATH}/15.0/odoo", COMMIT, "branch refs/heads/15.0", "locked") + + self.assertTrue(worktree.locked) + self.assertIsNone(worktree.locked_reason) + + def test_06_prunable_with_reason(self): + """A prunable worktree should keep the reason reported by git.""" + worktree = self.parse( + f"{WORKTREES_PATH}/14.0/odoo", + COMMIT, + "branch refs/heads/14.0", + "prunable gitdir file points to non-existent location", + ) + + self.assertTrue(worktree.prunable) + self.assertEqual(worktree.prunable_reason, "gitdir file points to non-existent location") + + def test_07_flags_are_booleans(self): + """Flags should be coerced to booleans, not left as the matched text.""" + worktree = self.parse(f"{WORKTREES_PATH}/16.0/odoo", COMMIT, "detached") + + for flag in (worktree.bare, worktree.detached, worktree.locked, worktree.prunable): + self.assertIsInstance(flag, bool) + + +class TestGitWorktreeBranch(OdevTestCase): + """Worktrees created by odev carry a local branch suffixed with the worktree name.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.connector = MagicMock() + cls.connector.name = "odoo/odoo" + cls.connector.odev.worktrees_path = WORKTREES_PATH + + def test_01_odev_suffix_is_stripped(self): + """`GitConnector.create_worktree` names local branches `-odev-`. + + The upstream revision is what commands display and compare, the local branch is what git needs. + """ + worktree = GitWorktree.parse( + self.connector, + porcelain(f"{WORKTREES_PATH}/mydb/odoo", COMMIT, "branch refs/heads/17.0-odev-mydb"), + ) + + self.assertEqual(worktree.local_branch, "17.0-odev-mydb") + self.assertEqual(worktree.branch, "17.0") + + def test_02_branch_without_suffix_is_kept(self): + """A branch not created by odev should be reported unchanged.""" + worktree = GitWorktree.parse( + self.connector, + porcelain(f"{WORKTREES_PATH}/17.0/odoo", COMMIT, "branch refs/heads/17.0"), + ) + + self.assertEqual(worktree.local_branch, "17.0") + self.assertEqual(worktree.branch, "17.0") + + def test_03_only_the_first_suffix_is_split(self): + """A branch name containing the separator more than once should split on the first occurrence.""" + worktree = GitWorktree.parse( + self.connector, + porcelain(f"{WORKTREES_PATH}/mydb/odoo", COMMIT, "branch refs/heads/17.0-odev-my-odev-db"), + ) + + self.assertEqual(worktree.branch, "17.0") + + +class TestGitWorktreeIdentity(OdevTestCase): + """Worktrees are named after their directory and identified by their path.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.connector = MagicMock() + cls.connector.name = "odoo/odoo" + cls.connector.odev.worktrees_path = WORKTREES_PATH + + def worktree(self, path: str, branch: str = "17.0") -> GitWorktree: + """Build a worktree on a branch at the given path.""" + return GitWorktree.parse(self.connector, porcelain(path, COMMIT, f"branch refs/heads/{branch}")) + + def test_01_name_is_the_parent_directory(self): + """A worktree under the odev worktrees directory is named after the directory holding it.""" + self.assertEqual(self.worktree(f"{WORKTREES_PATH}/17.0/odoo").name, "17.0") + self.assertEqual(self.worktree(f"{WORKTREES_PATH}/mydb/enterprise").name, "mydb") + + def test_02_name_outside_worktrees_path(self): + """A checkout managed outside of odev is reported as the master worktree.""" + self.assertEqual(self.worktree("/home/user/repositories/odoo/odoo").name, "master") + + def test_03_equality_on_path(self): + """Two worktrees at the same path are the same worktree, whatever their revision.""" + self.assertEqual(self.worktree(f"{WORKTREES_PATH}/17.0/odoo"), self.worktree(f"{WORKTREES_PATH}/17.0/odoo")) + self.assertNotEqual(self.worktree(f"{WORKTREES_PATH}/17.0/odoo"), self.worktree(f"{WORKTREES_PATH}/16.0/odoo")) + self.assertNotEqual(self.worktree(f"{WORKTREES_PATH}/17.0/odoo"), f"{WORKTREES_PATH}/17.0/odoo") + + def test_04_hash_on_path(self): + """Worktrees should deduplicate on their path when collected in a set.""" + worktrees = { + self.worktree(f"{WORKTREES_PATH}/17.0/odoo", "17.0"), + self.worktree(f"{WORKTREES_PATH}/17.0/odoo", "saas-17.2"), + self.worktree(f"{WORKTREES_PATH}/16.0/odoo"), + } + + self.assertEqual(len(worktrees), 2) + + def test_05_repr(self): + """The representation should identify the worktree by name, repository and revision.""" + self.assertEqual( + repr(self.worktree(f"{WORKTREES_PATH}/17.0/odoo")), + "GitWorktree(name='17.0', repository='odoo/odoo', revision='17.0')", + ) + + +class TestGitWorktreePendingChanges(OdevTestCase): + """Pending changes are counted from the revision list against the tracked upstream branch.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.connector = MagicMock() + cls.connector.name = "odoo/odoo" + cls.connector.odev.worktrees_path = WORKTREES_PATH + + def worktree(self, *attributes: str) -> GitWorktree: + """Build a worktree with the given porcelain attributes.""" + return GitWorktree.parse(self.connector, porcelain(f"{WORKTREES_PATH}/17.0/odoo", COMMIT, *attributes)) + + def patch_rev_list(self, result: str | Exception): + """Patch `git.Repo` as used by `pending_changes` to return or raise on `rev_list`.""" + repository = MagicMock() + + if isinstance(result, Exception): + repository.git.rev_list.side_effect = result + else: + repository.git.rev_list.return_value = result + + return self.patch("odev.common.connectors.git", "Repo", return_value=repository) + + def test_01_detached_has_no_upstream(self): + """A detached worktree has nothing to compare against and reports no pending changes.""" + self.assertEqual(self.worktree("detached").pending_changes(), (0, 0)) + + def test_02_counts_behind_and_ahead(self): + """The counts should be read from `git rev-list --left-right --count`.""" + with self.patch_rev_list("12\t3"): + self.assertEqual(self.worktree("branch refs/heads/17.0").pending_changes(), (12, 3)) + + def test_03_up_to_date(self): + """A worktree in sync with its upstream should report no pending changes.""" + with self.patch_rev_list("0\t0"): + self.assertEqual(self.worktree("branch refs/heads/17.0").pending_changes(), (0, 0)) + + def test_04_no_upstream_configured(self): + """A branch without an upstream cannot be compared and reports no pending changes.""" + error = GitCommandError("rev-list", 128, b"fatal: no upstream configured for branch '17.0'") + + with self.patch_rev_list(error): + self.assertEqual(self.worktree("branch refs/heads/17.0").pending_changes(), (0, 0)) + + def test_05_head_does_not_point_to_a_branch(self): + """A HEAD not pointing to a branch cannot be compared and reports no pending changes.""" + error = GitCommandError("rev-list", 128, b"fatal: HEAD does not point to a branch") + + with self.patch_rev_list(error): + self.assertEqual(self.worktree("branch refs/heads/17.0").pending_changes(), (0, 0)) + + def test_06_unexpected_git_error_is_raised(self): + """Any other git failure should surface instead of being reported as no pending changes.""" + error = GitCommandError("rev-list", 128, b"fatal: bad revision") + + with self.patch_rev_list(error), self.assertRaises(GitCommandError): + self.worktree("branch refs/heads/17.0").pending_changes() diff --git a/tests/tests/common/test_interrupts.py b/tests/tests/common/test_interrupts.py new file mode 100644 index 000000000..6baf1cbbc --- /dev/null +++ b/tests/tests/common/test_interrupts.py @@ -0,0 +1,56 @@ +import os +from signal import SIGINT, getsignal + +from odev.common.signal_handling import capture_signals + +from tests.conftest import InterruptRecorder +from tests.fixtures import OdevTestCase + + +class TestSuiteInterrupts(OdevTestCase): + """The suite has to stay stoppable while odev is holding the signal handlers. + + odev captures `SIGINT` around every query and every subprocess to cancel that operation rather than + let the interrupt through. Without the recorder `conftest` installs, a `Ctrl+C` landing inside one of + those blocks would be swallowed and the run would carry on. + """ + + def setUp(self): + super().setUp() + self.addCleanup(self.clear_interrupt) + + def clear_interrupt(self): + """Forget the interrupt recorded by a test. + + Left set, the flag would stop the session before the next test rather than at the end of this one, + taking the rest of the suite with it. + """ + InterruptRecorder.interrupted = False + + def test_01_odev_handlers_are_wrapped(self): + """The handlers odev installs should be the ones recording interrupts.""" + with capture_signals(handler=lambda *args: None): + self.assertIsInstance(getsignal(SIGINT), InterruptRecorder) + + def test_02_interrupt_is_recorded_and_handled(self): + """An interrupt captured by odev should reach its handler and be noted for the session.""" + handled: list[int] = [] + + def handler(signal_number, *args): + handled.append(signal_number) + + with capture_signals(handler=handler): + os.kill(os.getpid(), SIGINT) + + self.assertEqual(handled, [SIGINT], "odev should still get a chance to cancel what it was doing") + self.assertTrue(InterruptRecorder.interrupted, "the session should be stopped at the next test boundary") + + def test_03_handlers_are_wrapped_once(self): + """Restoring a wrapped handler should not wrap it again, however many blocks are nested.""" + with capture_signals(handler=lambda *args: None): + outer = getsignal(SIGINT) + + with capture_signals(handler=lambda *args: None): + pass + + self.assertIs(getsignal(SIGINT), outer, "the outer handler should be restored as it was") diff --git a/tests/tests/common/test_local_database.py b/tests/tests/common/test_local_database.py new file mode 100644 index 000000000..d7e3b93b9 --- /dev/null +++ b/tests/tests/common/test_local_database.py @@ -0,0 +1,79 @@ +from pathlib import Path +from unittest.mock import MagicMock + +from odev.common.databases import LocalDatabase +from odev.common.version import OdooVersion + +from tests.fixtures import OdevTestCase + + +class TestNeutralizeScripts(OdevTestCase): + """Neutralization must pick up the `data/neutralize.sql` scripts shipped by custom modules.""" + + def setUp(self): + super().setUp() + self.addons_path = self.res_path / "repositories" / "test" / "test-addons" + + def make_database(self, addons_paths: list[Path] | None = None) -> LocalDatabase: + database = LocalDatabase.__new__(LocalDatabase) + database._framework = self.odev + database._process = MagicMock() + database._process.additional_addons_paths = [self.addons_path] if addons_paths is None else addons_paths + return database + + def neutralize_scripts( + self, + installed_modules: list[str], + addons_paths: list[Path] | None = None, + version: str = "18.0", + ) -> list[Path]: + database = self.make_database(addons_paths) + + with ( + self.patch_property(LocalDatabase, "installed_modules", installed_modules), + self.patch_property(LocalDatabase, "process", database._process), + self.patch_property(LocalDatabase, "version", OdooVersion(version)), + ): + return database._neutralize_scripts() + + def test_custom_module_script_is_collected(self): + """Regression for #98: the module scripts were filtered against the *addons directory* + names rather than the module names, so no custom script was ever collected. + """ + scripts = self.neutralize_scripts(["base", "addon_01"]) + self.assertEqual( + scripts, + [ + self.odev.static_path / "neutralize-pre.sql", + self.addons_path / "addon_01" / "data" / "neutralize.sql", + self.odev.static_path / "neutralize-post.sql", + ], + ) + + def test_module_without_script_is_skipped(self): + scripts = self.neutralize_scripts(["addon_02"], addons_paths=[self.addons_path / "submodule"]) + self.assertEqual( + scripts, + [ + self.odev.static_path / "neutralize-pre.sql", + self.odev.static_path / "neutralize-post.sql", + ], + ) + + def test_module_not_installed_is_skipped(self): + scripts = self.neutralize_scripts(["base"]) + self.assertNotIn(self.addons_path / "addon_01" / "data" / "neutralize.sql", scripts) + + def test_no_addons_paths_yields_static_scripts_only(self): + scripts = self.neutralize_scripts(["addon_01"], addons_paths=[]) + self.assertEqual( + scripts, + [ + self.odev.static_path / "neutralize-pre.sql", + self.odev.static_path / "neutralize-post.sql", + ], + ) + + def test_older_versions_get_the_legacy_script(self): + scripts = self.neutralize_scripts(["base"], version="14.0") + self.assertIn(self.odev.static_path / "neutralize-post-before-15.0.sql", scripts) diff --git a/tests/tests/common/test_odev.py b/tests/tests/common/test_odev.py index 146abed80..fb5a551bb 100644 --- a/tests/tests/common/test_odev.py +++ b/tests/tests/common/test_odev.py @@ -2,22 +2,36 @@ import sys from pathlib import Path from types import ModuleType -from unittest.mock import patch +from unittest.mock import MagicMock, patch from odev._version import __version__ from odev.common.commands import Command -from odev.common.odev import Manifest, Plugin, logger +from odev.common.odev import Manifest, Odev, Plugin, logger, parse_plugin_manifest, plugin_module_name from tests.fixtures import CaptureOutput, OdevTestCase +REAL_UPDATE = Odev._update +"""Reference to the real implementation of `Odev._update`, taken before the test fixtures patch it away to +prevent tests from running git operations on the odev repository. +""" + + class TestCommonOdev(OdevTestCase): """Global sanity check of the odev framework.""" + def setUp(self): + super().setUp() + # Dispatching a command reads `sys.argv`; leaving a command line behind would feed it to whichever + # test runs next. + argv = sys.argv + self.addCleanup(setattr, sys, "argv", argv) + def test_01_config_file(self): """Config file should have been created in the correct directory.""" - self.assertEqual(self.odev.config.name, "odev-test") - self.assertEqual(self.odev.config.path, Path.home() / ".config/odev/odev-test.cfg") + self.assertEqual(self.odev.config.name, self.odev.name) + self.assertEqual(self.odev.config.path, self.run_path / f"{self.odev.name}.cfg") + self.assertTrue(self.odev.config.path.exists()) def test_02_config_get_set_reset_delete(self): """Config manager should be able to get, set and reset values, as well as delete a key or a section. @@ -81,9 +95,15 @@ class FirstCommand(Command): class SecondCommand(Command): _name = "duplicate" + module_path = Path(__file__) + with ( self.assertRaises(ValueError) as error, - self.patch(self.odev, "import_commands", return_value=[FirstCommand, SecondCommand]), + self.patch( + self.odev, + "import_commands", + return_value=[(FirstCommand, module_path), (SecondCommand, module_path)], + ), ): self.odev.register_commands() @@ -256,3 +276,124 @@ def test_20_load_plugins_repoints_preexisting_plugins_module(self): self.odev.load_plugins() self.assertEqual(sys.modules["odev.plugins"].__path__, [str(self.odev.plugins_path)]) + + def test_21_plugin_module_name(self): + """The module name of a plugin should drop the organization and use underscores.""" + self.assertEqual(plugin_module_name("odoo-odev/odev-plugin-editor-base"), "odev_plugin_editor_base") + self.assertEqual(plugin_module_name("odev-plugin-ai"), "odev_plugin_ai") + + def test_22_parse_plugin_manifest(self): + """The manifest of a plugin should be parsed into its name, version, description and dependencies.""" + manifest = parse_plugin_manifest( + '"""Some plugin."""\n\n__version__ = "1.2.3"\n\ndepends = ["test/test-plugin", 42]\n', + "test/test-plugin-dep", + ) + + self.assertEqual( + manifest, + { + "name": "test/test-plugin-dep", + "version": "1.2.3", + "description": "Some plugin.", + "depends": ["test/test-plugin"], + }, + ) + + def test_23_parse_plugin_manifest_invalid(self): + """A source that is not a valid plugin manifest should be rejected.""" + self.assertIsNone(parse_plugin_manifest('"""No version."""\n\ndepends = []\n', "test/test-plugin")) + self.assertIsNone(parse_plugin_manifest("def invalid(:\n", "test/test-plugin")) + self.assertIsNone(parse_plugin_manifest("__version__ = 1.0\n", "test/test-plugin")) + self.assertIsNone(parse_plugin_manifest('{"name": "Sales", "version": "17.0"}\n', "test/test-addons")) + + def test_24_parse_plugin_manifest_does_not_execute_code(self): + """Parsing the manifest of an untrusted repository should never execute its content.""" + with self.patch("odev.common.odev.logger", "warning") as logger_warning: + manifest = parse_plugin_manifest( + '"""Malicious plugin."""\n' + "import odev.common.odev as target\n" + 'target.logger.warning("executed")\n' + "raise SystemExit(1)\n" + '__version__ = "6.6.6"\n', + "evil/plugin", + ) + + self.assertEqual( + manifest, + { + "name": "evil/plugin", + "version": "6.6.6", + "description": "Malicious plugin.", + "depends": [], + }, + ) + logger_warning.assert_not_called() + + def test_25_update_skipped_when_up_to_date(self): + """Nothing should be pulled, and the user should not be prompted, when the local branch has no incoming + changes left after fetching. + """ + manifest = Manifest(name="odev", description="Odev", version=__version__, depends=[]) + + with ( + self.patch("odev.common.odev", "Repo", return_value=MagicMock()), + self.patch("odev.common.odev", "GitConnector", return_value=MagicMock()) as git_connector, + self.patch(self.odev, "_load_plugin_manifest", return_value=manifest), + self.patch(self.odev, "_Odev__git_branch_behind", return_value=False), + self.patch(self.odev, "_Odev__update_prompt", return_value=True) as update_prompt, + ): + self.assertFalse(REAL_UPDATE(self.odev, self.odev.path)) + git_connector.return_value.fetch.assert_called_once_with(detached=False) + + update_prompt.assert_not_called() + + def test_26_update_records_check_date_when_up_to_date(self): + """The date of the last update check should be recorded even when there was nothing to update, so that + checks are not run again on every single command. + """ + self.odev.config.update.date = "1995-12-21 00:00:00" + + with self.patch(self.odev, "_update", return_value=False): + self.assertFalse(self.odev.update(restart=False)) + + self.assertGreater(self.odev.config.update.date.year, 1995) + + def test_27_update_available(self): + """An update should be reported only when the repository has incoming commits and none of its own.""" + for rev_list, expected in [("2\t0", True), ("0\t0", False), ("1\t3", False)]: + repository = MagicMock(working_dir=str(self.odev.path)) + repository.head.is_detached = False + repository.git.rev_list.return_value = rev_list + + with ( + self.subTest(rev_list=rev_list), + self.patch_property(type(self.odev), "git", MagicMock(repository=repository)), + ): + self.assertEqual(self.odev.update_available(), expected) + + def test_28_update_available_without_repository(self): + """No update should be reported when odev does not run from a git repository.""" + with self.patch_property(type(self.odev), "git", MagicMock(repository=None)): + self.assertFalse(self.odev.update_available()) + + def test_29_update_available_detached_head(self): + """No update should be reported on a detached HEAD, which has no branch to compare with its remote.""" + repository = MagicMock(working_dir=str(self.odev.path)) + repository.head.is_detached = True + + with self.patch_property(type(self.odev), "git", MagicMock(repository=repository)): + self.assertFalse(self.odev.update_available()) + + repository.active_branch.tracking_branch.assert_not_called() + + def test_30_upgrade_version_ahead_of_current(self): + """A recorded version ahead of the running one, as left over by a switch back from the 'beta' release + channel, should be reset instead of being reported as a newer version forever. + """ + self.odev.config.update.version = "999.0.0" + + with CaptureOutput() as output: + self.odev.upgrade() + + self.assertEqual(output.stdout, "") + self.assertEqual(self.odev.config.update.version, __version__) diff --git a/tests/tests/common/test_odoobin.py b/tests/tests/common/test_odoobin.py new file mode 100644 index 000000000..b6c5cce73 --- /dev/null +++ b/tests/tests/common/test_odoobin.py @@ -0,0 +1,84 @@ +import shutil +import tempfile +from pathlib import Path + +from odev.common.databases import Repository +from odev.common.odoobin import OdoobinProcess + +from tests.fixtures import OdevTestCase + + +class FakeDatabase: + """Minimal stand-in for a database, exposing only what the addons paths derivation reads.""" + + def __init__(self, repository: Repository | None = None): + self.repository = repository + + +class TestExpandAddonsPaths(OdevTestCase): + """`expand_addons_paths` resolves a directory to the addons directories it contains.""" + + @property + def addons_path(self) -> Path: + return self.res_path / "repositories" / "test" / "test-addons" + + def test_finds_modules_nested_in_subdirectories(self): + expanded = OdoobinProcess.expand_addons_paths([self.addons_path]) + self.assertEqual(expanded, [self.addons_path, self.addons_path / "submodule"]) + + def test_ignores_directories_without_modules(self): + empty = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, empty, ignore_errors=True) + self.assertEqual(OdoobinProcess.expand_addons_paths([empty]), []) + + def test_deduplicates_overlapping_inputs(self): + expanded = OdoobinProcess.expand_addons_paths([self.addons_path, self.addons_path / "submodule"]) + self.assertEqual(expanded, [self.addons_path, self.addons_path / "submodule"]) + + def test_no_paths_yields_no_addons_paths(self): + self.assertEqual(OdoobinProcess.expand_addons_paths([]), []) + + +class TestAdditionalAddonsPaths(OdevTestCase): + """The addons paths derived from the linked repository must be expanded, and derived only once.""" + + def setUp(self): + super().setUp() + self.odev.config.paths.repositories = self.res_path / "repositories" + self.addons_path = self.res_path / "repositories" / "test" / "test-addons" + + def make_process(self, repository: Repository | None = None) -> OdoobinProcess: + process = OdoobinProcess.__new__(OdoobinProcess) + process.database = FakeDatabase(repository) # type: ignore [assignment] + process._additional_addons_paths = None + return process + + def test_repository_is_expanded_to_its_addons_directories(self): + """Regression for #98: the bare repository root is not a valid addons path when the + modules live in subdirectories, so it must be expanded before being used. + """ + process = self.make_process(Repository("test-addons", "test")) + self.assertEqual(process.additional_addons_paths, [self.addons_path, self.addons_path / "submodule"]) + + def test_without_repository_no_addons_paths(self): + self.assertEqual(self.make_process().additional_addons_paths, []) + + def test_derived_only_once(self): + process = self.make_process(Repository("test-addons", "test")) + calls: list[object] = [] + original = OdoobinProcess.expand_addons_paths + + def counting_expand(paths): + calls.append(paths) + return original(paths) + + with self.patch(OdoobinProcess, "expand_addons_paths", side_effect=counting_expand): + for _ in range(3): + _ = process.additional_addons_paths + + self.assertEqual(len(calls), 1) + + def test_explicit_assignment_is_not_overridden(self): + process = self.make_process(Repository("test-addons", "test")) + process.additional_addons_paths = [] + self.assertEqual(process.additional_addons_paths, []) diff --git a/tests/tests/common/test_odoobin_prepare_venv.py b/tests/tests/common/test_odoobin_prepare_venv.py new file mode 100644 index 000000000..501b46330 --- /dev/null +++ b/tests/tests/common/test_odoobin_prepare_venv.py @@ -0,0 +1,233 @@ +"""Tests for the python and system packages odev needs to prepare an Odoo installation.""" + +from collections.abc import Sequence +from pathlib import Path +from unittest.mock import MagicMock, patch + +from packaging.requirements import Requirement +from packaging.version import Version + +from odev.common import system +from odev.common.odoobin import DEBIAN_INSTALL_SCRIPT, SETUPTOOLS_REQUIREMENT, OdoobinProcess +from odev.common.python import PythonEnv + +from tests.fixtures import OdevTestCase + + +class TestSetuptoolsRequirement(OdevTestCase): + """Guardrails for #93 and for the Odoo versions that cannot run on a recent setuptools.""" + + def setuptools_requirement(self, python_version: str) -> Requirement: + """Return the setuptools requirement that applies to a given python version.""" + text = (self.odev.static_path / "requirements.txt").read_text(encoding="utf-8") + requirements = [ + Requirement(stripped) + for line in text.splitlines() + if (stripped := line.split("#", 1)[0].strip()) and Requirement(stripped).name == "setuptools" + ] + return next( + requirement + for requirement in requirements + if requirement.marker is None or requirement.marker.evaluate({"python_version": python_version}) + ) + + def test_modern_python_requires_a_usable_pep517_backend(self): + """Setuptools 58 and 59 cannot load `setuptools.build_meta` for current pip, which is what + breaks installing gevent with `--no-build-isolation`. + """ + specifier = self.setuptools_requirement("3.10").specifier + self.assertFalse(specifier.contains(Version("58.0.0")), "regression of #93") + self.assertFalse(specifier.contains(Version("59.0.0")), "regression of #93") + self.assertTrue(specifier.contains(Version("69.0.0"))) + + def test_modern_python_excludes_setuptools_without_pkg_resources(self): + """Setuptools 82 removed `pkg_resources`, which Odoo 15.0 and 16.0 import unconditionally + in `odoo/modules/module.py`; both run on python 3.10. + """ + specifier = self.setuptools_requirement("3.10").specifier + self.assertTrue(specifier.contains(Version("81.2.0"))) + self.assertFalse(specifier.contains(Version("82.0.0"))) + self.assertFalse(specifier.contains(Version("83.0.0"))) + + def test_legacy_python_keeps_the_legacy_cap(self): + """Odoo 13.0 and older run on python 3.7 or 2.7, where setuptools must stay old.""" + specifier = self.setuptools_requirement("3.7").specifier + self.assertTrue(specifier.contains(Version("57.5.0"))) + self.assertFalse(specifier.contains(Version("69.0.0"))) + + def test_constant_matches_the_static_requirements(self): + self.assertEqual( + Requirement(SETUPTOOLS_REQUIREMENT).specifier, + self.setuptools_requirement("3.10").specifier, + ) + + +class TestMissingRequirements(OdevTestCase): + """`missing_requirements` must honour the full specifier and the markers of a requirement.""" + + def write_requirements(self, *lines: str) -> Path: + path = self.run_path / "requirements.txt" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("\n".join(lines) + "\n", encoding="utf-8") + return path + + def missing(self, requirement: str, installed: dict[str, Version], python_version: str = "3.10") -> list[str]: + venv = PythonEnv(str(self.run_path / "venv")) + path = self.write_requirements(requirement) + + with ( + self.patch(PythonEnv, "installed_packages", return_value=installed), + self.patch_property(PythonEnv, "version", python_version), + ): + return list(venv.missing_requirements(path)) + + def test_upper_bound_is_honoured(self): + """The requirement parser only kept the first operator of a specifier, so an upper bound + was silently dropped and an over-new package was considered satisfied. + """ + requirement = "setuptools>=69.0.0,<82; python_version >= '3.8'" + self.assertEqual(self.missing(requirement, {"setuptools": Version("83.0.0")}), [requirement]) + self.assertEqual(self.missing(requirement, {"setuptools": Version("75.0.0")}), []) + + def test_lower_bound_is_honoured(self): + requirement = "setuptools>=69.0.0,<82; python_version >= '3.8'" + self.assertEqual(self.missing(requirement, {"setuptools": Version("58.0.0")}), [requirement]) + + def test_marker_is_evaluated_for_the_environment_python(self): + """Markers apply to the python of the virtual environment, not to the one odev runs under.""" + requirement = "setuptools<58.0.0; python_version < '3.8'" + self.assertEqual(self.missing(requirement, {"setuptools": Version("69.0.0")}, python_version="3.10"), []) + self.assertEqual( + self.missing(requirement, {"setuptools": Version("69.0.0")}, python_version="3.7"), + [requirement], + ) + + def test_missing_package_is_reported(self): + self.assertEqual(self.missing("phonenumbers", {}), ["phonenumbers"]) + + def test_unpinned_installed_package_is_satisfied(self): + self.assertEqual(self.missing("phonenumbers", {"phonenumbers": Version("8.13.0")}), []) + + def test_comments_and_blank_lines_are_skipped(self): + self.assertEqual(self.missing("# just a comment", {}), []) + + +class TestSatisfies(OdevTestCase): + """`PythonEnv.satisfies` answers whether an installed package matches a specification.""" + + def satisfies(self, specification: str, installed: dict[str, Version]) -> bool: + venv = PythonEnv(str(self.run_path / "venv")) + + with self.patch(PythonEnv, "installed_packages", return_value=installed): + return venv.satisfies(specification) + + def test_within_bounds(self): + self.assertTrue(self.satisfies(SETUPTOOLS_REQUIREMENT, {"setuptools": Version("75.0.0")})) + + def test_below_lower_bound(self): + self.assertFalse(self.satisfies(SETUPTOOLS_REQUIREMENT, {"setuptools": Version("58.0.0")})) + + def test_above_upper_bound(self): + self.assertFalse(self.satisfies(SETUPTOOLS_REQUIREMENT, {"setuptools": Version("83.0.0")})) + + def test_not_installed(self): + self.assertFalse(self.satisfies(SETUPTOOLS_REQUIREMENT, {})) + + +class TestSystemDependencies(OdevTestCase): + """The system dependencies check must work on any unix and never install anything itself.""" + + def setUp(self): + super().setUp() + self.odoo_path = self.run_path / "odoo" + (self.odoo_path / "setup").mkdir(parents=True, exist_ok=True) + self.process = OdoobinProcess.__new__(OdoobinProcess) + self.process._framework = self.odev + + def write_script(self): + (self.odoo_path / DEBIAN_INSTALL_SCRIPT).write_text("#!/bin/sh\n", encoding="utf-8") + + def missing( + self, + platform: str = "linux", + on_path: Sequence[str] = (), + debian_packages: list[str] | None = None, + headers: bool = True, + ) -> list[str]: + """Return the names of the dependencies reported as missing on a simulated system. + + :param platform: The value of `sys.platform` to simulate. + :param on_path: The executables available on the `PATH`. + :param debian_packages: The packages Odoo declares and that are missing, None on a system + where they cannot be listed. + :param headers: Whether the interpreter ships its development headers. + """ + venv = MagicMock(exists=True, version="3.10", has_development_headers=headers) + + with ( + self.patch_property(OdoobinProcess, "odoo_path", self.odoo_path), + self.patch_property(OdoobinProcess, "venv", venv), + self.patch(OdoobinProcess, "missing_debian_packages", return_value=debian_packages), + patch("odev.common.odoobin.sys.platform", platform), + patch("odev.common.system.shutil.which", side_effect=lambda command: command in on_path or None), + ): + return [dependency.name for dependency in self.process.missing_system_dependencies()] + + def test_probes_run_where_odoo_lists_no_package(self): + """Regression guard: a system that is not Debian-based used to be reported as complete, and + the user only found out when the build of gevent failed in the compiler. + """ + self.assertEqual( + self.missing(platform="darwin"), + [system.C_COMPILER.name, system.POSTGRESQL_HEADERS.name], + ) + + def test_probes_are_satisfied_by_any_of_their_commands(self): + self.assertEqual(self.missing(platform="darwin", on_path=["clang"]), [system.POSTGRESQL_HEADERS.name]) + self.assertEqual(self.missing(platform="darwin", on_path=["clang", "pg_config"]), []) + + def test_packages_declared_by_odoo_take_over_the_probes(self): + self.assertEqual(self.missing(debian_packages=["libpq-dev", "libsasl2-dev"]), ["libpq-dev", "libsasl2-dev"]) + + def test_headers_are_checked_even_where_odoo_lists_its_packages(self): + """`debian/control` asks for `python3-dev`, not for the headers of the version of python the + Odoo installation is actually built against. + """ + self.assertEqual(self.missing(debian_packages=[], headers=False), [system.PYTHON_HEADERS.name]) + + def test_no_check_outside_unix(self): + self.assertEqual(self.missing(platform="win32", headers=False), []) + + def test_nothing_reported_on_a_complete_system(self): + self.assertEqual(self.missing(debian_packages=[]), []) + + def test_never_installs_anything(self): + """The machine odev runs on belongs to the user: the check reports and never escalates.""" + run, execute = MagicMock(), MagicMock() + venv = MagicMock(exists=True, version="3.10", has_development_headers=False) + + with ( + self.patch_property(OdoobinProcess, "venv", venv), + self.patch( + OdoobinProcess, + "missing_system_dependencies", + return_value=[system.C_COMPILER, system.POSTGRESQL_HEADERS], + ), + patch("odev.common.odoobin.bash.run", run), + patch("odev.common.odoobin.bash.execute", execute), + ): + self.process.check_system_dependencies() + + run.assert_not_called() + execute.assert_not_called() + + def test_nothing_happens_when_no_dependency_is_missing(self): + run = MagicMock() + + with ( + self.patch(OdoobinProcess, "missing_system_dependencies", return_value=[]), + patch("odev.common.odoobin.bash.run", run), + ): + self.process.check_system_dependencies() + + run.assert_not_called() diff --git a/tests/tests/common/test_postgres_table.py b/tests/tests/common/test_postgres_table.py new file mode 100644 index 000000000..1ccda851d --- /dev/null +++ b/tests/tests/common/test_postgres_table.py @@ -0,0 +1,240 @@ +from unittest.mock import MagicMock + +from psycopg2.errors import InvalidTableDefinition + +from odev.common.connectors import PostgresConnector +from odev.common.postgres import PostgresDatabase, PostgresTable + +from tests.fixtures import OdevTestCase + + +class TestPostgresColumnsExist(OdevTestCase): + """`columns_exist` reports the columns missing from a table, it does not need a live database.""" + + def columns_exist(self, existing: list[str], requested: list[str]) -> list[str]: + """Run `columns_exist` against a connector whose query returns the given existing columns.""" + connector = PostgresConnector.__new__(PostgresConnector) + connector.query = lambda _: [(column,) for column in existing] # type: ignore [method-assign] + return connector.columns_exist("table", requested) + + def test_01_some_columns_missing(self): + """Only the requested columns absent from the table should be returned.""" + self.assertEqual(self.columns_exist(["id"], ["id", "name", "date"]), ["name", "date"]) + + def test_02_no_column_exists(self): + """An empty result means none of the requested columns exist, so all of them are missing. + + A table created from an older definition holds none of the new columns; reporting nothing missing + would leave it unmigrated. + """ + self.assertEqual(self.columns_exist([], ["id", "name"]), ["id", "name"]) + + def test_03_all_columns_exist(self): + """A table holding every requested column should report nothing missing.""" + self.assertEqual(self.columns_exist(["id", "name"], ["id", "name"]), []) + + def test_04_order_is_preserved(self): + """Missing columns should be reported in the order they were requested.""" + self.assertEqual(self.columns_exist(["b"], ["a", "b", "c"]), ["a", "c"]) + + def test_05_no_column_requested(self): + """Asking for no column should report nothing missing without querying the database.""" + connector = PostgresConnector.__new__(PostgresConnector) + + def fail_on_query(_): + raise AssertionError("no query should be issued when no column is requested") + + connector.query = fail_on_query # type: ignore [method-assign] + self.assertEqual(connector.columns_exist("table", []), []) + + def test_06_columns_must_be_a_list(self): + """Passing a bare string would build a query over its characters and is rejected.""" + connector = PostgresConnector.__new__(PostgresConnector) + + with self.assertRaises(TypeError): + connector.columns_exist("table", "id") # type: ignore [arg-type] + + +class TestPostgresTable(OdevTestCase): + """Tables should be brought in line with the definition declared on their subclass.""" + + table_name = "odev_test_table" + + def setUp(self): + super().setUp() + self.database = MagicMock(spec=PostgresDatabase) + """The database the tables are built against, kept aside to assert the queries it received.""" + + self.database.name = "odev-test" + self.database.tables = {} + self.database.columns_exist.return_value = [] + + def build_table( + self, + columns: dict[str, str] | None, + constraints: dict[str, str] | None = None, + missing: list[str] | None = None, + ) -> PostgresTable: + """Build a table against the mocked database, reporting the given columns as missing. + + :param columns: The columns declared on the table subclass. + :param constraints: The constraints declared on the table subclass. + :param missing: The columns `columns_exist` should report as absent from the table. + """ + self.database.columns_exist.return_value = missing or [] + table_name = self.table_name + + class TestTable(PostgresTable): + name = table_name + _columns = columns + _constraints = constraints + + return TestTable(self.database) + + def test_01_registers_itself_on_the_database(self): + """A table should be reachable from the database it was built against.""" + table = self.build_table({"id": "SERIAL PRIMARY KEY"}) + + self.assertIs(self.database.tables[self.table_name], table) + + def test_02_prepare_creates_the_table(self): + """Preparing a table should create it from the columns of its definition.""" + columns = {"id": "SERIAL PRIMARY KEY", "name": "VARCHAR"} + self.build_table(columns).prepare_database_table() + + self.database.create_table.assert_called_once_with(self.table_name, columns) + + def test_03_prepare_without_columns_does_nothing(self): + """A table whose subclass declares no column has nothing to create.""" + self.build_table(None).prepare_database_table() + + self.database.create_table.assert_not_called() + self.database.columns_exist.assert_not_called() + + def test_04_prepare_adds_missing_columns(self): + """Columns absent from an existing table should be added to it. + + `CREATE TABLE IF NOT EXISTS` leaves an existing table alone, so a column added to the definition + can only reach the table through this pass. + """ + table = self.build_table({"id": "SERIAL PRIMARY KEY", "comment": "VARCHAR"}, missing=["comment"]) + table.prepare_database_table() + + self.database.create_column.assert_called_once_with(self.table_name, "comment", "VARCHAR") + + def test_05_prepare_without_missing_columns(self): + """A table already matching its definition should not be altered.""" + self.build_table({"id": "SERIAL PRIMARY KEY"}).prepare_database_table() + + self.database.create_column.assert_not_called() + + def test_06_prepare_applies_constraints(self): + """Constraints declared on the table should be applied when it is prepared.""" + table = self.build_table( + {"id": "SERIAL PRIMARY KEY", "name": "VARCHAR"}, + {"name_unique": "UNIQUE (name)"}, + ) + table.prepare_database_table() + + self.database.constraint.assert_called_once_with(self.table_name, "name_unique", "UNIQUE (name)") + + def test_07_constraints_need_columns(self): + """Constraints are applied from within the columns pass and are skipped without a definition.""" + self.build_table(None, {"name_unique": "UNIQUE (name)"}).prepare_database_table() + + self.database.constraint.assert_not_called() + + def test_08_clear_empties_the_table(self): + """Clearing a table should delete its rows, not the table itself.""" + self.build_table({"id": "SERIAL PRIMARY KEY"}).clear() + + self.database.query.assert_called_once_with(f"DELETE FROM {self.table_name}") + + +class TestPostgresTableMissingColumnErrors(OdevTestCase): + """Renaming a primary key column requires dropping the constraint left by the previous definition.""" + + table_name = "odev_test_table" + + def setUp(self): + super().setUp() + self.database = MagicMock(spec=PostgresDatabase) + """The database the table is built against, kept aside to assert the queries it received.""" + + self.database.name = "odev-test" + self.database.tables = {} + self.database.columns_exist.return_value = ["identifier"] + + def build_table(self, error: Exception) -> PostgresTable: + """Build a table whose first `create_column` call raises the given error.""" + self.database.create_column.side_effect = [error, None] + table_name = self.table_name + + class TestTable(PostgresTable): + name = table_name + _columns = {"identifier": "SERIAL PRIMARY KEY"} + + return TestTable(self.database) + + def runtime_error(self, cause: Exception) -> RuntimeError: + """Build the error raised by the connector thread when a query fails.""" + error = RuntimeError(f"Exception in thread: {cause}") + error.__cause__ = cause + + return error + + def test_01_drops_the_stale_primary_key(self): + """A leftover primary key should be dropped so the renamed column can be created.""" + cause = InvalidTableDefinition('multiple primary keys for table "odev_test_table" are not allowed') + + self.build_table(self.runtime_error(cause)).prepare_database_table() + + self.database.query.assert_called_once_with( + f"ALTER TABLE {self.table_name} DROP CONSTRAINT IF EXISTS {self.table_name}_pkey" + ) + self.assertEqual(self.database.create_column.call_count, 2) + + def test_02_other_table_definition_errors_are_swallowed(self): + """Another invalid definition should be logged without retrying nor dropping a constraint.""" + cause = InvalidTableDefinition("column is of type integer but default expression is of type text") + + self.build_table(self.runtime_error(cause)).prepare_database_table() + + self.database.query.assert_not_called() + self.assertEqual(self.database.create_column.call_count, 1) + + def test_03_unrelated_runtime_errors_are_raised(self): + """A failure unrelated to the table definition should surface to the caller.""" + table = self.build_table(self.runtime_error(ValueError("boom"))) + + with self.assertRaises(RuntimeError): + table.prepare_database_table() + + +class TestPostgresDatabaseTables(OdevTestCase): + """Each database should own the mapping of the tables registered against it.""" + + def test_01_tables_are_not_shared_between_databases(self): + """Registering a table on one database should not make it visible on another. + + The mapping used to be a class attribute, so every database instance shared a single registry and + tables collided across databases on their name alone. + """ + first = PostgresDatabase.__new__(PostgresDatabase) + first.tables = {} + second = PostgresDatabase.__new__(PostgresDatabase) + second.tables = {} + + first.tables["history"] = MagicMock(spec=PostgresTable) + + self.assertEqual(second.tables, {}) + self.assertIsNot(first.tables, second.tables) + + def test_02_tables_are_instance_attributes(self): + """The registry should not live on the class, where every database would share it.""" + self.assertNotIn("tables", PostgresDatabase.__dict__) + self.assertIn("tables", vars(self.odev.store)) + + def test_03_datastore_registers_its_tables(self): + """The odev datastore should hold the three tables it is made of.""" + self.assertEqual(set(self.odev.store.tables), {"databases", "history", "secrets"}) diff --git a/tests/tests/common/test_python_env.py b/tests/tests/common/test_python_env.py index 0ca92fd12..f6a7d8348 100644 --- a/tests/tests/common/test_python_env.py +++ b/tests/tests/common/test_python_env.py @@ -3,6 +3,7 @@ from unittest.mock import MagicMock, patch from odev.common import bash +from odev.common.errors import OdevError from odev.common.python import PythonEnv from tests.fixtures import OdevTestCase @@ -55,3 +56,75 @@ def streaming_failure(command, **_kwargs): self.assertIsInstance(result, CompletedProcess) self.assertEqual(result.returncode, 1) stream_filter_mock.assert_called_once_with("line 1") + + +class TestSystemPackages(OdevTestCase): + """Odev runs on systems whose package manager it does not know, and must say what is missing + rather than dead-end on them. + """ + + @classmethod + def setUpClass(cls): + with patch("odev.common.odev.Odev.start", return_value=None): + super().setUpClass() + + def setUp(self): + super().setUp() + self.env = PythonEnv(path=self.run_path / "venv", version="3.10") + + def on_path(self, *commands: str): + return patch("odev.common.system.shutil.which", side_effect=lambda command: command in commands or None) + + def test_reports_without_a_known_package_manager(self): + """Used to raise `Neither dnf or apt package managers found on the system`, which named + neither the packages to install nor a way to move forward. + """ + with self.on_path(): + self.assertFalse(self.env.install_system_packages()) + + def test_reports_on_a_package_manager_odev_does_not_install_with(self): + """Arch is understood well enough to name its packages, not well enough to run `sudo` on.""" + with self.on_path("pacman"): + self.assertFalse(self.env.install_system_packages()) + + def test_creation_gives_up_once_when_nothing_can_be_installed(self): + """`create` used to retry unconditionally, asking the same question again on a system where + the answer could not change anything. + """ + error = RuntimeError("failed to find interpreter for Builtin discover of python_spec='3.10'") + + with ( + patch("odev.common.python.virtualenv.cli_run", side_effect=error), + patch("odev.common.python.console.confirm", return_value=True), + self.patch(PythonEnv, "install_system_packages", return_value=False) as install, + self.assertRaises(OdevError), + ): + self.env.create() + + install.assert_called_once() + + +class TestDevelopmentHeaders(OdevTestCase): + """Missing python headers are the most common reason for a source build to fail.""" + + def has_headers(self, include_path: Path | None) -> bool: + env = PythonEnv(path=self.run_path / "venv", version="3.10") + result = None if include_path is None else CompletedProcess("", 0, stdout=f"{include_path}\n".encode()) + + with self.patch(bash, "execute", return_value=result): + return env.has_development_headers + + def test_headers_present(self): + include_path = self.run_path / "include" / "python3.10" + include_path.mkdir(parents=True, exist_ok=True) + (include_path / "Python.h").write_text("", encoding="utf-8") + self.assertTrue(self.has_headers(include_path)) + + def test_headers_missing(self): + self.assertFalse(self.has_headers(self.run_path / "include" / "python3.10")) + + def test_never_warns_on_a_guess(self): + """An interpreter that cannot be questioned is assumed complete: a false alarm on every + `odev run` would be worse than staying silent. + """ + self.assertTrue(self.has_headers(None)) diff --git a/tests/tests/common/test_startup_performance.py b/tests/tests/common/test_startup_performance.py new file mode 100644 index 000000000..ec443aaea --- /dev/null +++ b/tests/tests/common/test_startup_performance.py @@ -0,0 +1,129 @@ +"""Guard the cost odev pays before running the command it was asked to run. + +Startup is paid by every single invocation, so an import creeping back into the framework's module graph is a +regression the whole tool feels. Timings are too noisy to assert on, so these tests check the structural cause +instead: what got imported, and when. +""" + +import subprocess +import sys +from pathlib import Path +from unittest import TestCase + + +REPOSITORY_PATH = Path(__file__).parents[3] +"""Path to the odev repository, from which the probed process is started.""" + +PROBE_MARKER = "ODEV_PROBE:" +"""Prefix identifying the result of a probe among everything odev itself prints.""" + +PROBE_PREAMBLE = f""" +PROBE = {PROBE_MARKER!r} +import sys +sys.argv = ["odev", "version"] +from odev.common import init_framework +framework = init_framework() +""" +"""Source prepended to every probe, leaving it an initialized but not yet started framework.""" + +HEAVY_MODULES = ( + "black", + "copier", + "github", + "InquirerPy", + "networkx", + "paramiko", + "prompt_toolkit", +) +"""Third-party modules that are expensive to import and that the framework must not need in order to start. + +Each is only useful to a fraction of odev's commands: a GitHub API client, an SSH agent client, a code formatter, +a project scaffolder, a graph library and an interactive prompt toolkit. They belong at their point of use. +""" + + +class TestStartupPerformance(TestCase): + """Check that odev does not import what it does not need in order to start.""" + + def probe(self, source: str) -> str: + """Run a snippet in a fresh interpreter and return what it reported. + + A subprocess is required: whatever the test suite itself imported would otherwise pollute the measurement. + + :param source: Python source to run, on top of :data:`PROBE_PREAMBLE`. + :return: The line the snippet printed, without its marker. + :rtype: str + """ + process = subprocess.run( # noqa: S603 + [sys.executable, "-c", source], + cwd=REPOSITORY_PATH, + capture_output=True, + text=True, + check=False, + ) + + self.assertEqual(process.returncode, 0, f"Probe failed:\n{process.stderr}") + + # Odev writes to stdout as well, only the marked line holds the result. + reported = next((line for line in process.stdout.splitlines() if line.startswith(PROBE_MARKER)), None) + self.assertIsNotNone(reported, f"Probe did not report anything:\n{process.stdout}\n{process.stderr}") + + return str(reported).removeprefix(PROBE_MARKER) + + def warm_up_command_index(self) -> None: + """Make sure the commands were already discovered once before measuring. + + The first run of a new version has no index yet and legitimately imports every command to build one. What + must stay free is every run after it, so give the index a chance to exist first. + """ + self.probe(f"{PROBE_PREAMBLE}\nframework.start()\nprint(PROBE + 'warmed')\n") + + def test_01_importing_the_framework_stays_lean(self): + """Importing odev must not pull in the dependencies only a few of its commands need.""" + reported = self.probe( + f"import odev.common\nimport sys\nprint({PROBE_MARKER!r} + ' '.join(" + f"name for name in {HEAVY_MODULES!r} if name in sys.modules))" + ) + imported = set(reported.split()) + + self.assertEqual( + imported, + set(), + f"Importing odev.common pulled in {', '.join(sorted(imported))}. " + "Import those where they are used, so that commands that do not need them do not pay for them.", + ) + + def test_02_starting_the_framework_imports_no_command(self): + """Starting the framework must know every command without executing any of their modules. + + A command module imports whatever its command needs at module level, so importing all of them to discover + their names makes every invocation pay for every command, plugins included. + """ + self.warm_up_command_index() + + reported = self.probe( + f"{PROBE_PREAMBLE}\n" + "framework.start()\n" + "print(PROBE + f'{len(framework.commands.entries)} {len(framework.commands.classes)}')\n" + ) + known, imported = (int(value) for value in reported.split()) + + self.assertGreater(known, 0, "The framework did not register any command") + self.assertEqual(imported, 0, f"Starting the framework imported {imported} command modules, expected none") + + def test_03_running_a_command_imports_only_that_command(self): + """Resolving a command must import that command alone, not the ones registered alongside it.""" + self.warm_up_command_index() + + reported = self.probe( + f"{PROBE_PREAMBLE}\n" + "framework.start()\n" + "framework.commands['version']\n" + "print(PROBE + ' '.join(sorted(framework.commands.classes)))\n" + ) + + self.assertEqual( + reported.split(), + ["version"], + f"Resolving the 'version' command imported '{reported.strip()}', expected only 'version'", + ) diff --git a/tests/tests/common/test_string.py b/tests/tests/common/test_string.py new file mode 100644 index 000000000..40d409b3b --- /dev/null +++ b/tests/tests/common/test_string.py @@ -0,0 +1,250 @@ +import datetime + +from odev.common import string + +from tests.fixtures import OdevTestCase + + +class TestCommonStringSizes(OdevTestCase): + """Byte sizes should be formatted and parsed back consistently.""" + + def test_01_bytes_size_units(self): + """Sizes should be scaled down to the largest unit under the 1024 factor.""" + self.assertEqual(string.bytes_size(0), "0.0 B") + self.assertEqual(string.bytes_size(512), "512.0 B") + self.assertEqual(string.bytes_size(1024), "1.0 KB") + self.assertEqual(string.bytes_size(1024**2), "1.0 MB") + self.assertEqual(string.bytes_size(1536**2), "2.2 MB") + self.assertEqual(string.bytes_size(1024**3), "1.0 GB") + + def test_02_bytes_size_largest_unit(self): + """Sizes above the largest known unit should fall back to yottabytes.""" + self.assertEqual(string.bytes_size(1024**8), "1.0 YB") + self.assertEqual(string.bytes_size(1024**9), "1024.0 YB") + + def test_03_bytes_size_negative(self): + """Negative sizes should be scaled on their absolute value and keep their sign.""" + self.assertEqual(string.bytes_size(-1024), "-1.0 KB") + + def test_04_bytes_from_string(self): + """Human readable sizes should be converted back to a number of bytes.""" + self.assertEqual(string.bytes_from_string("512"), 512) + self.assertEqual(string.bytes_from_string("512 B"), 512) + self.assertEqual(string.bytes_from_string("1 KB"), 1024) + self.assertEqual(string.bytes_from_string("1.5 MB"), 1572864) + self.assertEqual(string.bytes_from_string("2GB"), 2 * 1024**3) + + def test_05_bytes_from_string_invalid(self): + """A representation not starting with a number cannot be parsed.""" + with self.assertRaises(ValueError): + string.bytes_from_string("not a size") + + def test_06_bytes_size_roundtrip(self): + """Formatting a size and parsing it back should return the original value.""" + for size in (1024, 4 * 1024**2, 3 * 1024**3): + self.assertEqual(string.bytes_from_string(string.bytes_size(size)), size) + + +class TestCommonStringIndent(OdevTestCase): + """Indentation helpers back the layout of the `help` command output.""" + + text = " first line\n nested line\n last line" + + def test_01_min_indent(self): + """The smallest indentation of all non-blank lines should be returned.""" + self.assertEqual(string.min_indent(self.text), 4) + self.assertEqual(string.min_indent("no indent"), 0) + + def test_02_min_indent_without_content(self): + """A text without any non-blank line has no indentation to measure.""" + self.assertEqual(string.min_indent(""), 0) + self.assertEqual(string.min_indent("\n\n"), 0) + self.assertEqual(string.min_indent(" \n\t\n "), 0) + + def test_03_indent(self): + """Indenting should prefix every line with the requested number of spaces.""" + self.assertEqual(string.indent("one\ntwo", 2), " one\n two") + self.assertEqual(string.indent("one\ntwo"), "one\ntwo") + + def test_04_dedent(self): + """Dedenting by zero should keep the text as-is, relative indentation included.""" + self.assertEqual(string.dedent(self.text), self.text) + + def test_05_dedent_removes_indentation(self): + """Dedenting should remove the requested number of spaces from every line.""" + self.assertEqual(string.dedent(self.text, 4), "first line\n nested line\nlast line") + + def test_06_dedent_without_content(self): + """Dedenting a blank text should not fail on the absence of a minimum indentation.""" + self.assertEqual(string.dedent(""), "") + self.assertEqual(string.dedent("\n\n"), "\n\n") + + def test_07_normalize_indent(self): + """Normalizing should clean up a docstring-like text and strip its surrounding blanks.""" + self.assertEqual(string.normalize_indent("\n first line\n second line\n "), "first line\nsecond line") + self.assertEqual(string.normalize_indent(""), "") + + +class TestCommonStringJoin(OdevTestCase): + """Parts should be joined with the delimiters expected in user-facing messages.""" + + def test_01_join(self): + """Parts should be joined with commas when no last delimiter is given.""" + self.assertEqual(string.join([]), "") + self.assertEqual(string.join(["one"]), "one") + self.assertEqual(string.join(["one", "two", "three"]), "one, two, three") + + def test_02_join_and(self): + """The last two parts should be separated by "and".""" + self.assertEqual(string.join_and([]), "") + self.assertEqual(string.join_and(["one"]), "one") + self.assertEqual(string.join_and(["one", "two"]), "one and two") + self.assertEqual(string.join_and(["one", "two", "three"]), "one, two and three") + + def test_03_join_or(self): + """The last two parts should be separated by "or".""" + self.assertEqual(string.join_or(["one", "two", "three"]), "one, two or three") + + def test_04_join_bullet(self): + """Parts should be listed as bullets, without a leading blank line.""" + self.assertEqual(string.join_bullet([]), "") + self.assertEqual(string.join_bullet(["one"]), "• one") + self.assertEqual(string.join_bullet(["one", "two"]), "• one\n• two") + + +class TestCommonStringQuote(OdevTestCase): + """Quoting picks a delimiter absent from the string, it never escapes.""" + + def test_01_quote_default(self): + """A string without quotes should be wrapped in double quotes.""" + self.assertEqual(string.quote("plain"), '"plain"') + + def test_02_quote_containing_single(self): + """A string containing single quotes should be wrapped in double quotes.""" + self.assertEqual(string.quote("it's"), '"it\'s"') + + def test_03_quote_containing_double(self): + """A string containing double quotes should be wrapped in single quotes.""" + self.assertEqual(string.quote('say "hi"'), "'say \"hi\"'") + + def test_04_quote_force_single(self): + """Forcing single quotes should win over the automatic delimiter choice.""" + self.assertEqual(string.quote("plain", force_single=True), "'plain'") + self.assertEqual(string.quote('say "hi"', force_single=True), "'say \"hi\"'") + + def test_05_quote_dirty_only(self): + """Strings without any quote should be left untouched in `dirty_only` mode.""" + self.assertEqual(string.quote("plain", dirty_only=True), "plain") + self.assertEqual(string.quote("plain", dirty_only=True, force_single=True), "plain") + self.assertEqual(string.quote("it's", dirty_only=True), '"it\'s"') + + def test_06_quote_both_quote_characters(self): + """A string containing both delimiters cannot be represented and falls back to double quotes. + + Documented in `quote`: the helper selects a delimiter and never escapes, so callers must not + feed it untrusted input. + """ + self.assertEqual(string.quote("""a'b"c"""), '"a\'b"c"') + + +class TestCommonStringMarkup(OdevTestCase): + """Rich markup helpers should produce tags the console can render.""" + + def test_01_stylize_resolves_theme_styles(self): + """Aliased theme styles should be replaced by the value Rich understands.""" + self.assertEqual(string.stylize("text", "bold"), "[bold]text[/bold]") + self.assertNotIn("color.cyan", string.stylize("text", "color.cyan")) + + def test_02_list_styles(self): + """Opening tags should be listed in their order of appearance, closing ones ignored.""" + self.assertEqual(string.list_styles("[bold]one[/bold] [color.cyan]two[/color.cyan]"), ["bold", "color.cyan"]) + self.assertEqual(string.list_styles("[bold red]one[/bold red]"), ["bold red"]) + self.assertEqual(string.list_styles("no markup here"), []) + + def test_03_strip_styles(self): + """Markup tags should be removed, keeping the text they wrap.""" + self.assertEqual(string.strip_styles("[bold]text[/bold]"), "text") + self.assertEqual(string.strip_styles("plain text"), "plain text") + + def test_04_strip_styles_keeps_nested_tags(self): + """Only the outermost tag pair is removed, nested markup survives. + + `strip_styles` runs a single non-greedy substitution pass. Nothing in odev calls it today, so the + limitation is asserted rather than fixed. + """ + self.assertEqual( + string.strip_styles("[bold]one [color.cyan]two[/color.cyan] three[/bold]"), + "one [color.cyan]two[/color.cyan] three", + ) + + def test_05_resolve_styles(self): + """Aliased styles inside a text should be resolved to their theme value.""" + resolved = string.resolve_styles("[color.cyan]text[/color.cyan]") + self.assertNotIn("color.cyan", resolved) + self.assertIn("text", resolved) + + def test_06_strip_ansi_colors(self): + """ANSI color codes should be removed, leaving the text untouched.""" + self.assertEqual(string.strip_ansi_colors("\x1b[31mred\x1b[0m"), "red") + self.assertEqual(string.strip_ansi_colors("no colors"), "no colors") + + def test_07_link(self): + """Links should be rendered with the Rich link markup.""" + self.assertEqual( + string.link("odev", "https://github.com/odoo-odev"), "[link=https://github.com/odoo-odev]odev[/link]" + ) + + +class TestCommonStringHelpFormatting(OdevTestCase): + """Help formatting keeps the descriptions of the `help` command aligned in a column.""" + + def test_01_short_help(self): + """The name should be emphasized and the description aligned after the indentation.""" + self.assertEqual(string.short_help("run", "Run a database"), "[bold]run[/bold] Run a database") + self.assertEqual(string.short_help("run", "Run a database", 4), "[bold]run[/bold] Run a database") + + def test_02_format_options_list_aligns_descriptions(self): + """Descriptions should all start at the same column, driven by the longest name.""" + formatted = string.format_options_list([("run", "Run a database"), ("shell", "Open a shell")]) + descriptions = [line.index("Run a database") for line in formatted.splitlines() if "Run a database" in line] + descriptions += [line.index("Open a shell") for line in formatted.splitlines() if "Open a shell" in line] + + self.assertEqual(len(set(descriptions)), 1, "descriptions should be aligned on a single column") + + def test_03_format_options_list_blank_lines(self): + """Blank lines should be inserted between the elements of the list.""" + formatted = string.format_options_list([("run", "Run"), ("shell", "Shell")], blanks=1) + self.assertEqual(len(formatted.splitlines()), 3) + + +class TestCommonStringMisc(OdevTestCase): + """Remaining formatting helpers.""" + + def test_01_suid(self): + """Unique identifiers should be lowercase alphanumeric strings of a fixed length.""" + identifiers = {string.suid() for _ in range(100)} + + for identifier in identifiers: + self.assertRegex(identifier, r"^[a-z0-9]{8}$") + + self.assertGreater(len(identifiers), 1, "identifiers should not be constant") + + def test_02_seconds_to_time(self): + """Seconds should be rendered as a hours:minutes:seconds duration.""" + self.assertEqual(string.seconds_to_time(0), "0:00:00") + self.assertEqual(string.seconds_to_time(3661), "1:01:01") + + def test_03_ago(self): + """Past datetimes should be rendered relative to now.""" + self.assertEqual(string.ago(datetime.datetime.now() - datetime.timedelta(hours=2)), "2 hours ago") + + def test_04_float_to_hours_drops_minutes(self): + """Fractions of an hour are lost, minutes always come out as zero. + + `int(value - hours) * 60` truncates the fraction before scaling it, so it can only ever yield 0. + Nothing in odev nor in the plugins calls this helper, so the behaviour is asserted as-is rather + than fixed; correcting it would be `int((value - hours) * 60)`. + """ + self.assertEqual(string.float_to_hours(2.0), "2:00") + self.assertEqual(string.float_to_hours(1.5), "1:00") + self.assertEqual(string.float_to_hours(2.25), "2:00") diff --git a/tests/tests/common/test_system.py b/tests/tests/common/test_system.py new file mode 100644 index 000000000..6e85f2456 --- /dev/null +++ b/tests/tests/common/test_system.py @@ -0,0 +1,87 @@ +"""Tests for the description of the operating system odev runs on.""" + +from collections.abc import Sequence +from unittest.mock import patch + +from odev.common import system +from odev.common.system import SystemDependency + +from tests.fixtures import OdevTestCase + + +class TestPackageManager(OdevTestCase): + """Odev has to recognize the package manager of the system it runs on, whichever it is.""" + + def package_manager(self, on_path: Sequence[str]) -> str | None: + with patch("odev.common.system.shutil.which", side_effect=lambda command: command in on_path or None): + return system.package_manager() + + def test_every_known_package_manager_is_detected(self): + for manager in system.PACKAGE_MANAGERS: + self.assertEqual(self.package_manager([manager]), manager) + + def test_the_package_manager_of_the_distribution_wins_over_homebrew(self): + """Homebrew installs on Linux too, but a Fedora machine has to be told about `dnf`.""" + self.assertEqual(self.package_manager(["dnf", "brew"]), "dnf") + + def test_unknown_package_manager(self): + self.assertIsNone(self.package_manager([])) + + +class TestInstallInstructions(OdevTestCase): + """Whatever the system, the user must be told what is missing; the command is a bonus.""" + + def instructions(self, dependencies: Sequence[SystemDependency], on_path: Sequence[str] = ()) -> str: + with patch("odev.common.system.shutil.which", side_effect=lambda command: command in on_path or None): + return system.install_instructions(dependencies, version="3.10") + + def test_names_and_command(self): + instructions = self.instructions([system.C_COMPILER, system.POSTGRESQL_HEADERS], on_path=["dnf"]) + self.assertIn("• a C compiler", instructions) + self.assertIn("• the PostgreSQL client headers (pg_config)", instructions) + self.assertIn("sudo dnf install -y gcc libpq-devel", instructions) + + def test_placeholders_are_expanded(self): + instructions = self.instructions([system.PYTHON_HEADERS], on_path=["apt-get"]) + self.assertIn("• the development headers of python 3.10 (Python.h)", instructions) + self.assertIn("sudo apt-get install -y python3.10-dev", instructions) + + def test_names_only_without_a_known_package_manager(self): + """On a system odev knows nothing about, saying what is missing is still useful.""" + instructions = self.instructions([system.C_COMPILER]) + self.assertEqual(instructions, "• a C compiler") + + def test_dependencies_without_a_package_are_still_named(self): + """Arch Linux ships the python headers within `python` itself: there is nothing to install, + but the user still has to know they are what is missing. + """ + instructions = self.instructions([system.PYTHON_HEADERS, system.C_COMPILER], on_path=["pacman"]) + self.assertIn("• the development headers of python 3.10 (Python.h)", instructions) + self.assertIn("sudo pacman -S --needed base-devel", instructions) + self.assertNotIn("python3.10", instructions.rsplit("\n", 1)[-1]) + + def test_no_command_when_nothing_can_be_named(self): + instructions = self.instructions([system.PYTHON_HEADERS], on_path=["pacman"]) + self.assertEqual(instructions, "• the development headers of python 3.10 (Python.h)") + + +class TestSystemDependencyProbes(OdevTestCase): + """A dependency is only ever reported as missing when it can be proven to be.""" + + def found(self, dependency: SystemDependency, on_path: Sequence[str]) -> bool: + with patch("odev.common.system.shutil.which", side_effect=lambda command: command in on_path or None): + return dependency.found + + def test_any_command_proves_the_dependency(self): + self.assertTrue(self.found(system.C_COMPILER, ["clang"])) + self.assertTrue(self.found(system.C_COMPILER, ["gcc"])) + self.assertFalse(self.found(system.C_COMPILER, ["pg_config"])) + + def test_dependencies_without_a_command_are_never_found(self): + """Those are detected some other way and must not be probed on the `PATH` by mistake.""" + self.assertFalse(self.found(system.PYTHON_HEADERS, ["python3.10", "cc"])) + + def test_odoo_dependencies_name_a_package_for_every_manager_that_ships_one(self): + for dependency in system.ODOO_SYSTEM_DEPENDENCIES: + for manager in dependency.packages: + self.assertIn(manager, system.PACKAGE_MANAGERS, f"{dependency.name} names an unknown {manager!r}") diff --git a/tests/tests/common/test_version.py b/tests/tests/common/test_version.py index c4ee8bfab..18b7829a0 100644 --- a/tests/tests/common/test_version.py +++ b/tests/tests/common/test_version.py @@ -47,3 +47,92 @@ def test_04_master(self): def test_05_invalid(self): with self.assertRaises(InvalidVersion): OdooVersion("invalid") + + def test_06_major_only(self): + """A version without a minor number should default it to zero.""" + parsed = OdooVersion("17") + self.assertEqual(parsed.major, 17) + self.assertEqual(parsed.minor, 0) + self.assertEqual(str(parsed), "17.0") + + def test_07_enterprise(self): + """The enterprise marker should be parsed but left out of the string representation.""" + parsed = OdooVersion("17.0+e") + self.assertTrue(parsed.enterprise) + self.assertEqual(str(parsed), "17.0") + self.assertFalse(OdooVersion("17.0").enterprise) + + def test_08_repr(self): + """The representation should wrap the string version.""" + self.assertEqual(repr(OdooVersion("saas~16.4")), "OdooVersion(saas-16.4)") + + +class TestCommonVersionBool(OdevTestCase): + """A version should be falsy only when it carries no version information at all.""" + + def test_01_empty(self): + """An empty version has nothing set and should be falsy. + + `module` is padded to `MIN_VERSION_LENGTH`, so it is never an empty tuple and cannot be tested on + its own truthiness. + """ + self.assertFalse(OdooVersion("")) + self.assertFalse(OdooVersion("0.0")) + + def test_02_not_empty(self): + """Any version component being set should make the version truthy.""" + self.assertTrue(OdooVersion("17.0")) + self.assertTrue(OdooVersion("0.1")) + self.assertTrue(OdooVersion("master")) + self.assertTrue(OdooVersion("0.0.1.0.0")) + + +class TestCommonVersionOrdering(OdevTestCase): + """Versions should sort the way Odoo releases succeed each other. + + Ordering is what picks a revision when odev has several to choose from, so it matters as much as + parsing does. + """ + + def test_01_major_versions(self): + """Newer major versions should sort after older ones.""" + self.assertLess(OdooVersion("15.0"), OdooVersion("16.0")) + self.assertGreater(OdooVersion("17.0"), OdooVersion("16.0")) + + def test_02_saas_between_majors(self): + """A SaaS version should sort after the major it branches off, and before the next one.""" + self.assertGreater(OdooVersion("saas~16.4"), OdooVersion("16.0")) + self.assertLess(OdooVersion("saas~16.4"), OdooVersion("17.0")) + self.assertGreater(OdooVersion("saas~16.4"), OdooVersion("saas~16.2")) + + def test_03_saas_after_same_numbered_version(self): + """At equal numbers, a SaaS version should sort after the stable one.""" + self.assertGreater(OdooVersion("saas~16.0"), OdooVersion("16.0")) + + def test_04_master_is_the_newest(self): + """Master is the development version and should sort after every numbered version.""" + self.assertGreater(OdooVersion("master"), OdooVersion("17.0")) + self.assertGreater(OdooVersion("master"), OdooVersion("saas~17.4")) + + def test_05_enterprise_after_community(self): + """At equal versions, the enterprise edition should sort after the community one.""" + self.assertGreater(OdooVersion("17.0+e"), OdooVersion("17.0")) + + def test_06_module_versions(self): + """Module versions should be compared component by component, ignoring trailing zeros.""" + self.assertLess(OdooVersion("17.0.1.0.0"), OdooVersion("17.0.1.1.0")) + self.assertEqual(OdooVersion("17.0.1.0.0"), OdooVersion("17.0.1")) + + def test_07_equality_and_hash(self): + """Equal versions should compare equal and hash alike, whatever their notation.""" + self.assertEqual(OdooVersion("17.0"), OdooVersion("17.0")) + self.assertEqual(hash(OdooVersion("17.0")), hash(OdooVersion("17.0"))) + self.assertEqual(OdooVersion("saas~16.4"), OdooVersion("saas-16.4")) + self.assertNotEqual(OdooVersion("17.0"), OdooVersion("16.0")) + + def test_08_sorting(self): + """Sorting a set of versions should yield the chronological order of the releases.""" + versions = ["master", "16.0", "saas~16.4", "17.0", "15.0", "saas~17.2"] + expected = ["15.0", "16.0", "saas-16.4", "17.0", "saas-17.2", "master"] + + self.assertEqual([str(version) for version in sorted(map(OdooVersion, versions))], expected) diff --git a/tests/tests/setup/test_setup.py b/tests/tests/setup/test_setup.py index 148f7051e..0bf37ecc3 100644 --- a/tests/tests/setup/test_setup.py +++ b/tests/tests/setup/test_setup.py @@ -1,5 +1,5 @@ import shutil -from pathlib import Path +from unittest.mock import patch from odev.setup import completion, directories, symlink, update @@ -7,24 +7,44 @@ class TestSetup(OdevTestCase): + """Test the setup scripts run when installing odev. + + Both scripts link odev into the shell of the user, at `~/.local/bin/odev` and in the bash completion + directory. Left to their real destinations they would repoint the `odev` command of the developer at + whichever checkout the suite happens to run from, and two suites running at once would fight over the + same two links, so they are redirected into the sandbox of the run. + """ + def test_completion_01_completion(self): """Test the setup script responsible of creating a symlink to the bash completion script of odev. A symlink should be created on the file system. """ - with self.patch(completion.console, "confirm", return_value=True): + completion_path = self.run_path / "completions" / "complete_odev.sh" + + with ( + patch.object(completion, "comp_path", completion_path), + self.patch(completion.console, "confirm", return_value=True), + ): completion.setup(self.odev) - self.assertTrue(Path("~/.local/share/bash-completion/completions/complete_odev.sh").expanduser().is_symlink()) + self.assertTrue(completion_path.is_symlink()) + self.assertEqual(completion_path.resolve(), self.odev.path / "complete_odev.sh") def test_symlink_01_symlink(self): """Test the setup script responsible of creating a symlink to odev. A symlink should be created to map the "odev" command to the main file of this application. """ - with self.patch(symlink.console, "confirm", return_value=True): + command_path = self.run_path / "bin" / "odev" + + with ( + self.patch(symlink, "link_path", return_value=command_path), + self.patch(symlink.console, "confirm", return_value=True), + ): symlink.setup(self.odev) - self.assertTrue(Path("~/.local/bin/odev").expanduser().is_symlink()) + self.assertTrue(command_path.is_symlink()) + self.assertEqual(command_path.resolve(), self.odev.path / "odev.sh") def test_update_01_update(self): """Test the setup script responsible of setting the auto-update values for odev. diff --git a/tools/benchmark_startup.py b/tools/benchmark_startup.py new file mode 100755 index 000000000..64cf27ff6 --- /dev/null +++ b/tools/benchmark_startup.py @@ -0,0 +1,140 @@ +#!/usr/bin/env python3 +"""Measure how long odev takes to start up and shut down. + +Every odev invocation pays the framework's startup cost before the command it was asked to run even begins, and +pays an exit cost after that command printed its result. Both are directly perceptible for a tool run dozens of +times a day, so this script reports them separately instead of a single wall-clock number. + +Usage: + ~/.config/odev/venv/bin/python tools/benchmark_startup.py [--runs N] [--command NAME] +""" + +import argparse +import statistics +import subprocess +import sys +import time +from pathlib import Path + + +REPOSITORY_PATH = Path(__file__).parents[1] +"""Path to the odev repository, from which the measured process is started.""" + +HEAVY_MODULES = ("copier", "networkx", "black", "github", "paramiko", "InquirerPy", "prompt_toolkit") +"""Third-party modules that are expensive to import and that a trivial command has no reason to load.""" + +PHASES_PROBE = """ +import sys +from time import monotonic + +start = monotonic() +from odev.common import init_framework +imported = monotonic() + +odev = init_framework() +odev.start(start) +started = monotonic() + +odev.dispatch() +dispatched = monotonic() + +heavy = [module for module in {heavy!r} if module in sys.modules] +print( + f"PROBE {{imported - start}} {{started - imported}} {{dispatched - started}} {{len(sys.modules)}} {{','.join(heavy)}}", + file=sys.stderr, +) +""" + + +def measure_process(command: str) -> tuple[float, float]: + """Run odev in a subprocess and measure its total duration and its exit tail. + + The exit tail is the time between the last byte the command wrote and the moment the process actually died: it + covers everything odev still does once the user can already read the result. + + :param command: Name of the odev command to run. + :return: The total duration and the exit tail, both in seconds. + :rtype: Tuple[float, float] + """ + start = time.monotonic() + process = subprocess.Popen( # noqa: S603 + [sys.executable, "main.py", command], + cwd=REPOSITORY_PATH, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + ) + + last_output = start + + for _ in process.stdout: # type: ignore [union-attr] + last_output = time.monotonic() + + process.wait() + end = time.monotonic() + + return end - start, end - last_output + + +def measure_phases(command: str) -> tuple[list[float], int, list[str]]: + """Measure the duration of each startup phase from inside the process. + + :param command: Name of the odev command to run. + :return: The duration of the import, start and dispatch phases, the number of imported modules and the heavy + modules that were loaded. + :rtype: Tuple[List[float], int, List[str]] + """ + probe = PHASES_PROBE.format(heavy=list(HEAVY_MODULES)) + process = subprocess.run( # noqa: S603 + [sys.executable, "-c", probe, command], + cwd=REPOSITORY_PATH, + capture_output=True, + text=True, + check=False, + ) + + line = next((line for line in process.stderr.splitlines() if line.startswith("PROBE ")), None) + + if line is None: + raise RuntimeError(f"Probe did not report any timing:\n{process.stderr}") + + _, imports, start, dispatch, modules, heavy = line.split(" ") + + return [float(imports), float(start), float(dispatch)], int(modules), [name for name in heavy.split(",") if name] + + +def main() -> int: + """Run the benchmark and print its report.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--runs", type=int, default=10, help="Number of times the command is run (default: 10)") + parser.add_argument("--command", default="version", help="Odev command to measure (default: version)") + arguments = parser.parse_args() + + print(f"Measuring 'odev {arguments.command}' over {arguments.runs} runs...\n") + + totals: list[float] = [] + tails: list[float] = [] + + for _ in range(arguments.runs): + total, tail = measure_process(arguments.command) + totals.append(total) + tails.append(tail) + + phases, modules, heavy = measure_phases(arguments.command) + + print(f"{'Phase':<28} {'Median':>9} {'Min':>9}") + print("-" * 48) + print(f"{'import odev.common':<28} {phases[0]:>8.3f}s {'':>9}") + print(f"{'init_framework + start':<28} {phases[1]:>8.3f}s {'':>9}") + print(f"{'dispatch (command)':<28} {phases[2]:>8.3f}s {'':>9}") + print(f"{'exit tail':<28} {statistics.median(tails):>8.3f}s {min(tails):>8.3f}s") + print("-" * 48) + print(f"{'total wall clock':<28} {statistics.median(totals):>8.3f}s {min(totals):>8.3f}s") + + print(f"\nImported modules: {modules}") + print(f"Heavy modules loaded: {', '.join(heavy) if heavy else 'none'}") + + return 0 + + +if __name__ == "__main__": + sys.exit(main())