[GitHub] [tvm] gromero commented on a diff in pull request #11043: Better version handling for Arduino

[GitBox <gi...@apache.org>]

gromero commented on code in PR #11043:
URL: https://github.com/apache/tvm/pull/11043#discussion_r853238754


##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,11 +347,44 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
         # Recursively change includes
         self._convert_includes(project_dir, source_dir)
 
+    def _get_arduino_cli_cmd(self, options: dict):
+        arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+        assert arduino_cli_cmd, "'arduino_cli_cmd' command not passed and not found by default!"
+        return arduino_cli_cmd
+
+    def _get_platform_version(self, arduino_cli_path: str) -> float:
+        # sample output of this command:
+        # 'arduino-cli alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z\n'
+        version_output = subprocess.run(
+            [arduino_cli_path, "version"], check=True, stdout=subprocess.PIPE
+        ).stdout.decode("utf-8")
+
+        full_version = re.findall(r"version: ([\.0-9]*)", version_output.lower())
+        full_version = full_version[0].split(".")
+        version = float(f"{full_version[0]}.{full_version[1]}")
+        return version
+
+    # This will only be run for build and upload
+    def _check_platform_version(self, options):
+        if not self._version:
+            cli_command = self._get_arduino_cli_cmd(options)
+            self._version = self._get_platform_version(cli_command)
+
+        if self._version < MIN_ARDUINO_CLI_VERSION:
+            message = (
+                f"Arduino CLI version too old: found {self._version}, "
+                f"need at least {ARDUINO_CLI_VERSION}."
+            )
+            if options.get("warning_as_error") is not None and options["warning_as_error"]:
+                raise server.ServerError(message=message)
+            _LOG.warning(message)
+
     def _get_fqbn(self, options):
         o = BOARD_PROPERTIES[options["arduino_board"]]
         return f"{o['package']}:{o['architecture']}:{o['board']}"
 
     def build(self, options):
+        self._check_platform_version(options)

Review Comment:
   Now that `warning_as_error` option has no effect on `generate_project` but on `build()` and `flash()`, could you please adapt  "ProjectOption" in the server to list `build` and `flash` in the `optional` field instead of `generate_project`?  This mostly useful for the CLI that use the Project API, like `tvmc`.



##########
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py:
##########
@@ -105,9 +106,8 @@ def test_auto_detect_port(self, sub_mock):
         assert handler._auto_detect_port(self.DEFAULT_OPTIONS) == "/dev/ttyACM0"
 
         # Test it raises an exception when no board is connected
-        sub_mock.return_value.stdout = bytes(self.BOARD_DISCONNECTED_V18, "utf-8")
-        with pytest.raises(microtvm_api_server.BoardAutodetectFailed):
-            handler._auto_detect_port(self.DEFAULT_OPTIONS)
+        sub_mock.return_value.stdout = bytes(self.BOARD_CONNECTED_V21, "utf-8")

Review Comment:
   I think now comment "# Test it raises an exception when no board is connected" in line 108 must go to above line 112? 



##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,11 +347,44 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
         # Recursively change includes
         self._convert_includes(project_dir, source_dir)
 
+    def _get_arduino_cli_cmd(self, options: dict):
+        arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+        assert arduino_cli_cmd, "'arduino_cli_cmd' command not passed and not found by default!"
+        return arduino_cli_cmd
+
+    def _get_platform_version(self, arduino_cli_path: str) -> float:
+        # sample output of this command:
+        # 'arduino-cli alpha Version: 0.18.3 Commit: d710b642 Date: 2021-05-14T12:36:58Z\n'
+        version_output = subprocess.run(
+            [arduino_cli_path, "version"], check=True, stdout=subprocess.PIPE
+        ).stdout.decode("utf-8")
+
+        full_version = re.findall(r"version: ([\.0-9]*)", version_output.lower())
+        full_version = full_version[0].split(".")
+        version = float(f"{full_version[0]}.{full_version[1]}")
+        return version
+
+    # This will only be run for build and upload
+    def _check_platform_version(self, options):
+        if not self._version:
+            cli_command = self._get_arduino_cli_cmd(options)
+            self._version = self._get_platform_version(cli_command)
+
+        if self._version < MIN_ARDUINO_CLI_VERSION:
+            message = (
+                f"Arduino CLI version too old: found {self._version}, "
+                f"need at least {ARDUINO_CLI_VERSION}."

Review Comment:
   This now needs to be `MIN_ARDUINO_CLI_VERSION`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org