Skip to content
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

Install rust globally #331

Merged
merged 36 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
a5b0c84
install rust globally
popenta Sep 13, 2023
7250ffa
try fix env
popenta Sep 13, 2023
69fba5e
code cleanup
popenta Sep 13, 2023
0bb5667
update version
popenta Sep 15, 2023
5295378
remove code related to old rust env
popenta Sep 15, 2023
2845d07
install rust for tests
popenta Sep 15, 2023
6906afa
remove explicit rust installation for workflow
popenta Sep 18, 2023
cad2cbd
Merge branch 'main' into globally-install-rust
popenta Sep 18, 2023
47c897e
add pytest flag for showing logs
popenta Sep 18, 2023
7ef887f
set flag for pytest logs
popenta Sep 18, 2023
6c22bac
remove pytest for testing purposes
popenta Sep 21, 2023
e9542fe
debugging
popenta Sep 21, 2023
67defe7
Merge branch 'main' into globally-install-rust
popenta Sep 21, 2023
45e9698
remove python 3.8 from GH action
popenta Sep 21, 2023
362cb9b
install sc-meta on GH action
popenta Sep 21, 2023
edb1c59
set rust toolchain to nightly
popenta Sep 21, 2023
0c9aaa9
remove default sc-meta installation when installing rust
popenta Sep 21, 2023
d7681dd
fix typo
popenta Sep 21, 2023
87df9d4
add new pytest marker
popenta Sep 22, 2023
84ce195
install sc-meta in GH workflow
popenta Sep 22, 2023
b3c6a0c
remove unnecessary prints
popenta Sep 22, 2023
3cbff95
bump version
popenta Sep 22, 2023
c597e34
started refactoring
popenta Sep 22, 2023
485bfaf
Install sc-meta when installing rust
popenta Sep 25, 2023
d5999e9
skip some tests on windows
popenta Sep 25, 2023
837b794
skip test on windows
popenta Sep 25, 2023
39f1b77
install wasm-opt and twiggy using cargo
popenta Sep 26, 2023
177d263
fix resolution
popenta Sep 27, 2023
648cb4c
Merge pull request #340 from multiversx/cargo-install-wasm-opt
popenta Sep 27, 2023
340e5e1
remove clang installation
popenta Sep 27, 2023
fc8609a
fix localnet and unit tests
popenta Sep 27, 2023
8eea743
fix workflow
popenta Sep 27, 2023
ad585ea
run GH actions using newer python version & wasm-opt fixes
popenta Sep 28, 2023
851bd30
fix test assert
popenta Sep 28, 2023
d2af8f5
fixes after review
popenta Sep 28, 2023
d8bb7a4
add link to mxpy cookbook
popenta Sep 28, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 38 additions & 38 deletions .github/workflows/build-windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ name: build

on:
pull_request:
branches: [ main, feat/* ]
branches: [main, feat/*]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some unrelated whitespace changes in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll blame it on the formatter.

workflow_dispatch:

jobs:
build:
name: Build and Test mxpy for ${{ matrix.os }}, python ${{ matrix.python-version }}

runs-on: ${{ matrix.os }}

strategy:
Expand All @@ -20,39 +20,39 @@ jobs:
python-version: [3.8]

steps:
- uses: actions/checkout@v2
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
shell: bash
run: |
python3 -m pip install --upgrade pip
pip3 install -r requirements.txt
pip3 install pytest
- name: Set github_api_token
shell: bash
run: |
mkdir ~/multiversx-sdk
export PYTHONPATH=.
python3 -m multiversx_sdk_cli.cli config new test
python3 -m multiversx_sdk_cli.cli config set github_api_token ${{ secrets.GITHUB_TOKEN }}
- name: Setup test dependencies
shell: bash
run: |
python3 -m multiversx_sdk_cli.cli deps install testwallets
- name: Run unit tests
shell: bash
run: |
export PYTHONPATH=.
pytest -m "not skip_on_windows" .
- name: Run CLI tests
shell: bash
run: |
export PROXY=https://testnet-gateway.multiversx.com
export CHAIN_ID=T
cd ./multiversx_sdk_cli/tests
source ./test_cli_tx.sh && testAll || return 1
source ./test_cli_dns.sh && testOffline || return 1
source ./test_cli_validators.sh && testAll || return 1
- uses: actions/checkout@v2
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
shell: bash
run: |
python3 -m pip install --upgrade pip
pip3 install -r requirements.txt
pip3 install pytest
- name: Set github_api_token
shell: bash
run: |
mkdir ~/multiversx-sdk
export PYTHONPATH=.
python3 -m multiversx_sdk_cli.cli config new test
python3 -m multiversx_sdk_cli.cli config set github_api_token ${{ secrets.GITHUB_TOKEN }}
- name: Setup test dependencies
shell: bash
run: |
python3 -m multiversx_sdk_cli.cli deps install testwallets
- name: Run unit tests
shell: bash
run: |
export PYTHONPATH=.
pytest -m "not skip_on_windows" .
- name: Run CLI tests
shell: bash
run: |
export PROXY=https://testnet-gateway.multiversx.com
export CHAIN_ID=T
cd ./multiversx_sdk_cli/tests
source ./test_cli_tx.sh && testAll || return 1
source ./test_cli_dns.sh && testOffline || return 1
source ./test_cli_validators.sh && testAll || return 1
1 change: 0 additions & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ jobs:
python3 -m multiversx_sdk_cli.cli deps install testwallets
python3 -m multiversx_sdk_cli.cli deps install wasm-opt
python3 -m multiversx_sdk_cli.cli deps install rust --overwrite
cargo install multiversx-sc-meta
- name: Run unit tests
run: |
export PYTHONPATH=.
Expand Down
53 changes: 42 additions & 11 deletions multiversx_sdk_cli/dependencies/modules.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
myprocess, utils, workstation)
from multiversx_sdk_cli.dependencies.resolution import (
DependencyResolution, get_dependency_resolution)
from multiversx_sdk_cli.ux import show_warning

logger = logging.getLogger("modules")

Expand All @@ -34,7 +35,6 @@
logger.info("Already exists. Skip install.")
return

self._guard_cannot_install_on_host()
self.uninstall(tag)
self._do_install(tag)

Expand Down Expand Up @@ -66,10 +66,6 @@
def get_resolution(self) -> DependencyResolution:
return get_dependency_resolution(self.key)

def _guard_cannot_install_on_host(self):
if self.get_resolution() == DependencyResolution.Host:
raise errors.KnownError(f"Installation of {self.key} on the host machine is not supported. Perhaps set 'dependencies.{self.key}.resolution' to 'SDK' in config?")


class StandaloneModule(DependencyModule):
def __init__(self,
Expand Down Expand Up @@ -256,6 +252,29 @@


class Rust(DependencyModule):
def is_installed(self, tag: str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove tag from is_installed, as well. Can be done in next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about it, but the other modules need the parameter. I think. We can further look into this.

which_rustc = shutil.which("rustc")
which_cargo = shutil.which("cargo")
logger.info(f"which rustc: {which_rustc}")
logger.info(f"which cargo: {which_cargo}")

return which_rustc is not None and which_cargo is not None

def install(self, tag: str, overwrite: bool) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, here we completely override the function DependencyModule.install().

# Fallback to default tag if not provided
tag = tag or config.get_dependency_tag(self.key)

logger.info(f"install: key={self.key}, tag={tag}, overwrite={overwrite}")

if overwrite:
logger.info("Overwriting the current rust version...")
elif self._is_installed_and_set_to_nightly():
return

self._do_install(tag)
self._install_sc_meta()
self._post_install(tag)

def _do_install(self, tag: str) -> None:
installer_url = self._get_installer_url()
installer_path = self._get_installer_path()
Expand All @@ -273,6 +292,11 @@

myprocess.run_process(args)

def _install_sc_meta(self):
logger.info("Installing multiversx-sc-meta")
args = ["cargo", "install", "multiversx-sc-meta"]
myprocess.run_process(args)

def _get_installer_url(self) -> str:
if workstation.is_windows():
return "https://win.rustup.rs"
Expand All @@ -290,13 +314,20 @@
if os.path.isdir(directory):
shutil.rmtree(directory)

def is_installed(self, tag: str) -> bool:
which_rustc = shutil.which("rustc")
which_cargo = shutil.which("cargo")
logger.info(f"which rustc: {which_rustc}")
logger.info(f"which cargo: {which_cargo}")
def _is_installed_and_set_to_nightly(self) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems to be a clone of is_installed(). Do we still require it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Removed it.

# the method parameter is not used in this specific module
is_rust_installed = self.is_installed("")

return which_rustc is not None and which_cargo is not None
if not is_rust_installed:
return is_rust_installed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can say return False (more explicit). Then, continue without else (less indentation).

else:
self._recommend_default_rust_version()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let's move this warning in the if-else that starts on line 269. This way, _is_installed_and_set_to_nightly is a bit more "pure" (does not take any decision, does not produce any effect as a consequence of its own internal checks).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, you can directly inline the implementation of _recommend_default_rust_version inside install - also because the warning message mentions the "overwrite" flag, and the overwrite check is performed there.

Can also stay as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it inside the install() method.

return True

def _recommend_default_rust_version(self):
module = dependencies.get_module_by_key("rust")
default_tag: str = config.get_dependency_tag(module.key)

Check warning on line 329 in multiversx_sdk_cli/dependencies/modules.py

View workflow job for this annotation

GitHub Actions / runner / mypy

[mypy] reported by reviewdog 🐶 By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked] Raw Output: /home/runner/work/mx-sdk-py-cli/mx-sdk-py-cli/multiversx_sdk_cli/dependencies/modules.py:329:9: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
show_warning(f"We recommend using rust {default_tag}. If you'd like to overwrite your current version please run `mxpy deps install rust --overwrite`.")

def get_directory(self, tag: str) -> Path:
tools_folder = workstation.get_tools_folder()
Expand Down
2 changes: 1 addition & 1 deletion multiversx_sdk_cli/tests/shared.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ PROXY="${PROXY:-http://localhost:7950}"
CHAIN_ID="${CHAIN_ID:-localnet}"
TestUser=./testdata/testUser.pem
TestUser2=./testdata/testUser2.pem
RUST_VERSION="nightly-2023-04-24"
RUST_VERSION="nightly-2023-05-26"

cleanSandbox() {
rm -rf ${SANDBOX}
Expand Down
50 changes: 50 additions & 0 deletions multiversx_sdk_cli/tests/test_cli_deps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import shutil

import pytest

from multiversx_sdk_cli.cli import main
from multiversx_sdk_cli.config import get_dependency_tag


def test_deps_install_rust():
default_tag = get_dependency_tag("rust")
return_code = main(["deps", "install", "rust", "--tag", default_tag, "--overwrite"])
if return_code:
assert False
else:
assert True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be assert return_code == 0.

Also, maybe do an assert for existence of some files in ~/.cargo/...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done



@pytest.mark.skip_on_windows
def test_deps_install_vmtools():
return_code = main(["deps", "install", "vmtools"])
if return_code:
assert False
else:
assert True


def test_deps_check_rust():
return_code = main(["deps", "check", "rust"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unit test can be merged into the first one, test_deps_install_rust.

if return_code:
assert False
else:
assert True


@pytest.mark.skip_on_windows
def test_check_sc_meta():
which_sc_meta = shutil.which("sc-meta")
if which_sc_meta:
assert True
elif which_sc_meta is None:
assert False


@pytest.mark.skip_on_windows
def test_deps_check_vmtools():
return_code = main(["deps", "check", "vmtools"])
if return_code:
assert False
else:
assert True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit test can be merged into the other one, which installs vmtools. Also, assert return_code == 0, here and above.

15 changes: 0 additions & 15 deletions multiversx_sdk_cli/tests/test_cli_deps.sh

This file was deleted.

Loading