-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix "deps check all" command #388
Changes from 3 commits
d42df24
3306bc1
fca22c9
45375a9
57cace9
11f0f0d
1ece80a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
import logging | ||
from typing import Any | ||
from typing import Any, List, Tuple | ||
|
||
from multiversx_sdk_cli import cli_shared, config, dependencies, errors | ||
from multiversx_sdk_cli.dependencies.install import get_deps_dict | ||
from multiversx_sdk_cli.dependencies.modules import DependencyModule | ||
from multiversx_sdk_cli.myprocess import run_process | ||
|
||
logger = logging.getLogger("cli.deps") | ||
|
||
|
@@ -34,16 +36,63 @@ def install(args: Any): | |
|
||
def check(args: Any): | ||
name: str = args.name | ||
module = dependencies.get_module_by_key(name) | ||
tag_to_check: str = config.get_dependency_tag(module.key) | ||
|
||
if name == "all": | ||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tested and it seems to work now 👍 |
||
all_dependencies = dependencies.get_all_deps() | ||
missing_dependencies: List[Tuple[str, str]] = [] | ||
|
||
for dependency in all_dependencies: | ||
tag_to_check: str = config.get_dependency_tag(dependency.key) | ||
is_installed = check_module_is_installed(dependency, tag_to_check) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
if not is_installed: | ||
missing_dependencies.append((dependency.key, tag_to_check)) | ||
|
||
if len(missing_dependencies): | ||
raise errors.DependenciesMissing(missing_dependencies) | ||
return | ||
if name == "rust": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice for having the check done in this new, better way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now refactored. |
||
module = dependencies.get_module_by_key(name) | ||
tag_to_check: str = config.get_dependency_tag(module.key) | ||
|
||
is_installed = check_module_is_installed(module, tag_to_check) | ||
if is_installed: | ||
actual_rust = _get_actual_installed_rust_version() | ||
|
||
if tag_to_check in actual_rust: | ||
logger.info(f"[{module.key} {tag_to_check}] is installed.") | ||
return | ||
if "command not found" in actual_rust: | ||
logger.warning("You have installed Rust without using `rustup`.") | ||
return | ||
else: | ||
logger.warning(f"The Rust version you have installed does not match the recommended version.\nInstalled [{actual_rust}], expected [{tag_to_check}].") | ||
return | ||
|
||
raise errors.DependencyMissing(module.key, tag_to_check) | ||
else: | ||
module = dependencies.get_module_by_key(name) | ||
tag_to_check: str = config.get_dependency_tag(module.key) | ||
|
||
is_installed = check_module_is_installed(module, tag_to_check) | ||
if is_installed: | ||
logger.info(f"[{module.key} {tag_to_check}] is installed.") | ||
return | ||
|
||
raise errors.DependencyMissing(module.key, tag_to_check) | ||
|
||
|
||
def check_module_is_installed(module: DependencyModule, tag_to_check: str) -> bool: | ||
resolution: str = config.get_dependency_resolution(module.key) | ||
resolution = resolution if resolution else "HOST" | ||
|
||
logger.info(f"Checking dependency: module = {module.key}, tag = {tag_to_check}, resolution = {resolution}") | ||
|
||
installed = module.is_installed(tag_to_check) | ||
if installed: | ||
logger.info(f"[{name} {tag_to_check}] is installed.") | ||
return | ||
return installed | ||
|
||
|
||
raise errors.DependencyMissing(name, tag_to_check) | ||
def _get_actual_installed_rust_version() -> str: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have it directly in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved it in the |
||
args = ["rustup", "default"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will throw if rust is not installed beforehand, and the error is not caught (sorry if I'm mistaken). Perhaps do a try / catch here and return empty string if missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It didn't trow any error as it was called only if |
||
output = run_process(args, dump_to_stdout=False) | ||
return output.rstrip("\n") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now using |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
from multiversx_sdk_cli.dependencies.install import install_module, get_module_directory, get_module_by_key, get_golang | ||
from multiversx_sdk_cli.dependencies.install import (get_all_deps, get_golang, | ||
get_module_by_key, | ||
get_module_directory, | ||
install_module) | ||
|
||
__all__ = ["install_module", "get_module_directory", "get_module_by_key", "get_golang"] | ||
__all__ = ["install_module", "get_module_directory", "get_module_by_key", "get_golang", "get_all_deps"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,9 +26,6 @@ def install(self, overwrite: bool) -> None: | |
# We install the default tag | ||
tag = config.get_dependency_tag(self.key) | ||
|
||
if tag == 'latest': | ||
tag = self.get_latest_release() | ||
|
||
logger.info(f"install: key={self.key}, tag={tag}, overwrite={overwrite}") | ||
|
||
if self._should_skip(tag, overwrite): | ||
|
@@ -60,9 +57,6 @@ def is_installed(self, tag: str) -> bool: | |
def get_env(self) -> Dict[str, str]: | ||
raise NotImplementedError() | ||
|
||
def get_latest_release(self) -> str: | ||
raise NotImplementedError() | ||
|
||
def get_resolution(self) -> DependencyResolution: | ||
return get_dependency_resolution(self.key) | ||
|
||
|
@@ -135,14 +129,6 @@ def _get_download_url(self, tag: str) -> str: | |
url = url.replace("{TAG}", tag) | ||
return url | ||
|
||
def get_latest_release(self) -> str: | ||
if self.repo_name is None or self.organisation is None: | ||
raise ValueError(f'{self.key}: repo_name or organisation not specified') | ||
|
||
org_repo = f'{self.organisation}/{self.repo_name}' | ||
tag = utils.query_latest_release_tag(org_repo) | ||
return tag | ||
|
||
def _get_archive_path(self, tag: str) -> Path: | ||
tools_folder = Path(workstation.get_tools_folder()) | ||
archive = tools_folder / f"{self.key}.{tag}.{self.archive_type}" | ||
|
@@ -252,9 +238,6 @@ def get_env(self) -> Dict[str, str]: | |
def get_gopath(self) -> Path: | ||
return self.get_parent_directory() / "GOPATH" | ||
|
||
def get_latest_release(self) -> str: | ||
raise errors.UnsupportedConfigurationValue("Golang tag must always be explicit, not latest") | ||
|
||
|
||
class Rust(DependencyModule): | ||
def is_installed(self, tag: str) -> bool: | ||
|
@@ -338,7 +321,7 @@ def _install_sc_meta(self): | |
tag = config.get_dependency_tag("sc-meta") | ||
args = ["cargo", "install", "multiversx-sc-meta", "--locked"] | ||
|
||
if tag != "latest": | ||
if tag != "": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced with |
||
args.extend(["--version", tag]) | ||
|
||
myprocess.run_process(args) | ||
|
@@ -354,7 +337,7 @@ def _install_twiggy(self): | |
tag = config.get_dependency_tag("twiggy") | ||
args = ["cargo", "install", "twiggy"] | ||
|
||
if tag != "latest": | ||
if tag != "": | ||
args.extend(["--version", tag]) | ||
|
||
myprocess.run_process(args) | ||
|
@@ -383,9 +366,6 @@ def get_directory(self, tag: str) -> Path: | |
def get_env(self) -> Dict[str, str]: | ||
return dict(os.environ) | ||
|
||
def get_latest_release(self) -> str: | ||
raise errors.UnsupportedConfigurationValue("Rust tag must either be explicit, empty or 'nightly'") | ||
|
||
|
||
class TestWalletsModule(StandaloneModule): | ||
def __init__(self, key: str): | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After re-thinking about this, maybe we can drop
check all
andinstall all
? On the long run, maybe it's better (less issues, less corner cases) if we do.Let's double check with @ovidiuolteanu and @ccorcoveanu. I feel
deps install all
is quite exotic.