Fix "deps check all" command #388
Conversation
| @@ -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.
Can be if not tag (also below).
There was a problem hiding this comment.
Replaced with if tag:
|
|
||
| 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.
This does not handle rust the complete way, as done below.
multiversx_sdk_cli/cli_deps.py
Outdated
|
|
||
| raise errors.DependencyMissing(name, tag_to_check) | ||
| def _get_actual_installed_rust_version() -> str: |
There was a problem hiding this comment.
Can we have it directly in Rust dependency module - modules.py, by overriding is_installed()?
There was a problem hiding this comment.
Moved it in the Rust module.
multiversx_sdk_cli/cli_deps.py
Outdated
|
|
||
| raise errors.DependencyMissing(name, tag_to_check) | ||
| def _get_actual_installed_rust_version() -> str: | ||
| args = ["rustup", "default"] |
There was a problem hiding this comment.
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.
It didn't trow any error as it was called only if is_installed was True.
multiversx_sdk_cli/cli_deps.py
Outdated
| def _get_actual_installed_rust_version() -> str: | ||
| args = ["rustup", "default"] | ||
| output = run_process(args, dump_to_stdout=False) | ||
| return output.rstrip("\n") |
There was a problem hiding this comment.
Perhaps use strip() directly, in case other whitespace might occur in the future?
multiversx_sdk_cli/cli_deps.py
Outdated
| if len(missing_dependencies): | ||
| raise errors.DependenciesMissing(missing_dependencies) | ||
| return | ||
| if name == "rust": |
There was a problem hiding this comment.
Nice for having the check done in this new, better way.
There was a problem hiding this comment.
This is now refactored.
| module = dependencies.get_module_by_key(name) | ||
| tag_to_check: str = config.get_dependency_tag(module.key) | ||
|
|
||
| if name == "all": |
There was a problem hiding this comment.
After re-thinking about this, maybe we can drop check all and install 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.
|
|
||
| if tag in actual_version_installed: | ||
| logger.info(f"[{self.key} {tag}] is installed.") | ||
| elif "Command 'rustup' not found" in actual_version_installed: |
There was a problem hiding this comment.
The error message might look differently on MacOS. E.g.
multiversx@papple ~ % rustupfoobar
zsh: command not found: rustupfoobar
multiversx@papple ~ %
Thus, we can check for "not found" in actual_version_installed perhaps?
There was a problem hiding this comment.
Indeed. Now the elif statement is as above.
| tag_to_check: str = config.get_dependency_tag(module.key) | ||
|
|
||
| is_installed = check_module_is_installed(module, tag_to_check) | ||
| if is_installed and name != "rust": |
There was a problem hiding this comment.
For Rust, we don't display this log info? Or without this if the message will be duplicated by the one from modules.py? I think better to have it duplicated on the CLI than to have this custom if here (e.g. in the future we will forget its reason).
There was a problem hiding this comment.
Without the if statement the message will be duplicated by the one in modules.py. Why I added it it's because if you have a different Rust version other than the recommended one, you'll see a warning that the versions don't match but the log will show that the correct version is installed since it takes the tag from the config.
A problem that could occur with the other dependencies as well, now that I think about it, but is less common.
|
|
||
| if name == "all": |
There was a problem hiding this comment.
cargo does not seem to be loaded correctly when trying to install all deps
There was a problem hiding this comment.
tested and it seems to work now 👍
No description provided.