Install rust globally #331

popenta · 2023-09-13T13:19:46Z

In this PR the mxpy deps install rust command has been changed.

From now on mxpy will install rust system-wide. The Rust module now also installs sc-meta, twiggy and wasm-opt.

The clang dependency has also been removed. You can not install it through mxpy anymore. You'll have to install it manually, if needed.

Solves issue #306, #310, #316 and #332.

andreibancioiu · 2023-09-15T11:39:03Z

multiversx_sdk_cli/dependencies/modules.py

-            "RUSTUP_HOME": str(directory),
-            "CARGO_HOME": str(directory)
-        }
+        env: Dict[str, str] = {}


I think get_env_for_install() isn't used anymore? Also, _get_env_for_is_installed_in_sdk() - can both be removed?

Alternatively, we can do the same as for get_env(), and keep the old calls to get_env_for_install(), but simply replace the implementation with return os.environ?

andreibancioiu · 2023-09-15T11:39:22Z

multiversx_sdk_cli/dependencies/modules.py

@@ -269,12 +269,12 @@ def _do_install(self, tag: str) -> None:
            toolchain = "nightly"

        args = [str(installer_path), "--verbose", "--default-toolchain", toolchain, "--profile",
-                "minimal", "--target", "wasm32-unknown-unknown", "--no-modify-path", "-y"]
-        output = myprocess.run_process(args, env=self.get_env_for_install())
+                "minimal", "--target", "wasm32-unknown-unknown", "-y"]


andreibancioiu · 2023-09-15T11:39:46Z

multiversx_sdk_cli/dependencies/modules.py

-
-        raise errors.BadDependencyResolution(self.key, resolution)
+    def get_env(self) -> Dict[str, str]:
+        return dict(os.environ)


andreibancioiu · 2023-09-22T08:39:23Z

multiversx_sdk_cli/projects/project_rust.py

+        which_sc_meta = shutil.which("sc-meta")
+
+        if which_sc_meta is None:
+            raise errors.KnownError("'sc-meta' is not installed. Run 'cargo install multiversx-sc-meta' then try again.")


andreibancioiu · 2023-09-22T08:40:10Z

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "hatchling.build"

 [project]
 name = "multiversx-sdk-cli"
-version = "8.2.0b1"
+version = "8.1.1"


Should be 8.2.0.

Bumped the version

andreibancioiu · 2023-09-25T14:18:26Z

.github/workflows/build-windows.yml

@@ -5,13 +5,13 @@ name: build

 on:
  pull_request:
-    branches: [ main, feat/* ]
+    branches: [main, feat/*]


There are some unrelated whitespace changes in this file.

I'll blame it on the formatter.

andreibancioiu · 2023-09-25T14:22:03Z

multiversx_sdk_cli/dependencies/modules.py

-            except Exception:
-                return False
+        if not is_rust_installed:
+            return is_rust_installed


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

andreibancioiu · 2023-09-25T14:23:31Z

multiversx_sdk_cli/tests/test_cli_deps.py

+    if return_code:
+        assert False
+    else:
+        assert True


Can be assert return_code == 0.

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

andreibancioiu · 2023-09-25T14:24:41Z

multiversx_sdk_cli/tests/test_cli_deps.py

+
+
+def test_deps_check_rust():
+    return_code = main(["deps", "check", "rust"])


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

andreibancioiu · 2023-09-25T14:25:14Z

multiversx_sdk_cli/tests/test_cli_deps.py

+def test_deps_check_vmtools():
+    return_code = main(["deps", "check", "vmtools"])
+    if return_code:
+        assert False
+    else:
+        assert True


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

andreibancioiu · 2023-09-25T14:25:42Z

multiversx_sdk_cli/tests/test_cli_deps.sh

@@ -1,15 +0,0 @@
-#!/usr/bin/env bash
-


andreibancioiu · 2023-09-25T14:26:44Z

multiversx_sdk_cli/dependencies/modules.py

+
+        return which_rustc is not None and which_cargo is not None
+
+    def install(self, tag: str, overwrite: bool) -> None:


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

andreibancioiu · 2023-09-26T09:45:30Z

multiversx_sdk_cli/dependencies/modules.py

+        if not is_rust_installed:
+            return is_rust_installed
+        else:
+            self._recommend_default_rust_version()


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).

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.

Moved it inside the install() method.

Cargo install wasm-opt and twiggy when installing rust

andreibancioiu · 2023-09-28T09:39:22Z

multiversx_sdk_cli/config.py

-        "dependencies.wasm-opt.urlTemplate.linux": "https://github.com/WebAssembly/binaryen/releases/download/{TAG}/binaryen-{TAG}-x86_64-linux.tar.gz",
-        "dependencies.wasm-opt.urlTemplate.osx": "https://github.com/WebAssembly/binaryen/releases/download/{TAG}/binaryen-{TAG}-x86_64-macos.tar.gz",
-        "dependencies.wasm-opt.urlTemplate.windows": "https://github.com/WebAssembly/binaryen/releases/download/{TAG}/binaryen-{TAG}-x86_64-windows.tar.gz",
+        "dependencies.wasm-opt.tag": "0.112.0",


andreibancioiu · 2023-09-28T09:40:44Z

multiversx_sdk_cli/dependencies/install.py


 logger = logging.getLogger("install")


-def install_module(key: str, tag: str = "", overwrite: bool = False):
+def install_module(key: str, overwrite: bool = False):


Indeed, we are getting rid of --tag from mxpy deps. Tags are stored within the config file.

andreibancioiu · 2023-09-28T09:44:57Z

multiversx_sdk_cli/dependencies/modules.py

+        logger.info(f"which twiggy: {which_twiggy}")
+
+        dependencies = [which_rustc, which_cargo, which_sc_meta, which_wasm_opt, which_twiggy]
+        return all(dependency is not None for dependency in dependencies)


Indeed, shutil.which() returns None when application is missing.

andreibancioiu · 2023-09-28T09:48:12Z

multiversx_sdk_cli/dependencies/modules.py

+        self._do_install(tag)
+        self._install_sc_meta()
+        self._install_wasm_opt()
+        self._install_twiggy()


Let's rename _do_install() to _install_rust (for consistency). Since we anyway override the install from the base class.

Renamed to _install_rust().

andreibancioiu · 2023-09-28T09:49:15Z

multiversx_sdk_cli/dependencies/modules.py

+
+    def _install_twiggy(self):
+        logger.info("Installing twiggy.")
+        default_tag = config.get_dependency_tag("twiggy")


In _install_wasm_opt, the variable name is tag, while here is default_tag. Rename (either way) to be consistent?

Indeed. Renamed to tag.

andreibancioiu · 2023-09-28T09:55:15Z

multiversx_sdk_cli/projects/project_clang.py

@@ -26,6 +29,10 @@ def perform_build(self):
        self.file_output = self.unit.with_suffix('.wasm')

        try:
+            is_installed = are_clang_and_cpp_dependencies_installed()


We should mention this in the PR description, as well.

Let's do a raise KnownError instead.

Perhaps we can have a check_clang_and_cpp_dependencies_installed in shared.py. And this would, actually, throw an error if dependencies are missing.

Renamed to check_clang_and_cpp_dependencies_installed and now raising a KnownError.

andreibancioiu · 2023-09-28T10:18:24Z

multiversx_sdk_cli/projects/project_rust.py

@@ -53,15 +54,16 @@ def prepare_build_wasm_args(self, args: List[str]):
            self.get_output_folder()
        ])

+    def check_if_sc_meta_is_installed(self):
+        which_sc_meta = shutil.which("sc-meta")


How about checking if the rust dependency is installed (thus, all parts of the rust "module")?

check_if_sc_meta_is_installed can actually be deleted. Before the code reaches this part, the ensure_dependencies_installed() is called. That calls the install() method of the Rust module. If not all the dependencies are installed, it should install them.

andreibancioiu · 2023-09-28T10:19:06Z

multiversx_sdk_cli/projects/shared.py

@@ -22,3 +28,29 @@ def _directory_contains_file(directory: Path, name_suffix: str) -> bool:
        if str(file).lower().endswith(name_suffix.lower()):
            return True
    return False
+
+
+def are_clang_and_cpp_dependencies_installed() -> bool:


Wrt. the other comment, can directly throw error here.

Throwing error here.

andreibancioiu · 2023-09-28T10:24:52Z

.github/workflows/build.yml

+        run: |
+          python3 -m multiversx_sdk_cli.cli config set dependencies.vmtools.tag v1.4.60
+          cd ./multiversx_sdk_cli/tests
+          source ./test_cli_contracts.sh && testAll || return 1


How come that we don't need to install clang / llvm, as well? No C smart contracts in our tests?

If so, we should add some tests for C contracts in the next PR.

we only use rust contracts in our tests

andreibancioiu · 2023-09-28T10:26:57Z

multiversx_sdk_cli/tests/test_cli_deps.py

+    assert True if return_code == 0 else False
+
+    which_rustc = shutil.which("rustc")
+    if which_rustc:


I think these ifs make the test a bit more weak. We can drop the ifs to make it more restrictive.

multiversx_sdk_cli/cli_deps.py

install rust globally

a5b0c84

popenta self-assigned this Sep 13, 2023

popenta marked this pull request as draft September 13, 2023 13:19

popenta added 2 commits September 13, 2023 16:34

try fix env

7250ffa

code cleanup

69fba5e

popenta marked this pull request as ready for review September 15, 2023 10:31

update version

0bb5667

andreibancioiu reviewed Sep 15, 2023

View reviewed changes

popenta added 17 commits September 15, 2023 14:54

remove code related to old rust env

5295378

install rust for tests

2845d07

remove explicit rust installation for workflow

6906afa

Merge branch 'main' into globally-install-rust

cad2cbd

add pytest flag for showing logs

47c897e

set flag for pytest logs

7ef887f

remove pytest for testing purposes

6c22bac

debugging

e9542fe

Merge branch 'main' into globally-install-rust

67defe7

remove python 3.8 from GH action

45e9698

install sc-meta on GH action

362cb9b

set rust toolchain to nightly

edb1c59

remove default sc-meta installation when installing rust

0c9aaa9

fix typo

d7681dd

add new pytest marker

87df9d4

install sc-meta in GH workflow

84ce195

remove unnecessary prints

b3c6a0c

andreibancioiu reviewed Sep 22, 2023

View reviewed changes

bump version

3cbff95

andreibancioiu previously approved these changes Sep 22, 2023

View reviewed changes

popenta marked this pull request as draft September 22, 2023 10:20

started refactoring

c597e34

popenta dismissed andreibancioiu’s stale review via c597e34 September 22, 2023 13:46

popenta added 3 commits September 25, 2023 12:34

Install sc-meta when installing rust

485bfaf

skip some tests on windows

d5999e9

skip test on windows

837b794

popenta marked this pull request as ready for review September 25, 2023 12:36

andreibancioiu reviewed Sep 26, 2023

View reviewed changes

popenta marked this pull request as draft September 26, 2023 13:41

popenta and others added 3 commits September 26, 2023 17:22

install wasm-opt and twiggy using cargo

39f1b77

fix resolution

177d263

Merge pull request #340 from multiversx/cargo-install-wasm-opt

648cb4c

Cargo install wasm-opt and twiggy when installing rust

popenta changed the base branch from main to feat/next September 27, 2023 09:08

popenta added 5 commits September 27, 2023 18:44

remove clang installation

340e5e1

fix localnet and unit tests

fc8609a

fix workflow

8eea743

run GH actions using newer python version & wasm-opt fixes

ad585ea

fix test assert

851bd30

popenta marked this pull request as ready for review September 28, 2023 09:03

andreibancioiu reviewed Sep 28, 2023

View reviewed changes

popenta added 2 commits September 28, 2023 15:14

fixes after review

d2af8f5

add link to mxpy cookbook

d8bb7a4

andreibancioiu approved these changes Sep 28, 2023

View reviewed changes

bogdan-rosianu self-requested a review October 2, 2023 08:01

bogdan-rosianu reviewed Oct 2, 2023

View reviewed changes

multiversx_sdk_cli/cli_deps.py Show resolved Hide resolved

bogdan-rosianu approved these changes Oct 2, 2023

View reviewed changes

popenta merged commit 4cfcf50 into feat/next Oct 2, 2023
9 checks passed

popenta deleted the globally-install-rust branch October 2, 2023 08:44

popenta mentioned this pull request Oct 13, 2023

Install wasm-opt using cargo install #332

Closed



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


		return which_rustc is not None and which_cargo is not None

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

Install rust globally #331

Install rust globally #331

Conversation

popenta commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popenta commented Sep 13, 2023 •

edited

Loading