[microtvm][RVM] Refactor Arduino/Zephyr into one RVM #12023

mehrdadh · 2022-07-06T17:52:04Z

This PR removes duplication of two RVMs for testing Arduino and Zephyr.
cc @alanmacd @areusch @gromero @guberti

guberti

Glad to see we finally merged these RVMs! Thanks @mehrdadh for doing this work.

guberti · 2022-07-06T22:49:52Z

apps/microtvm/reference-vm/README.md

+## Reference VM Organization

-Reference VMs are organized in this directory as follows:
+Reference VM is organized in this directory as follows:


Suggested change

Reference VM is organized in this directory as follows:

The Reference VM is organized in this directory as follows:

guberti · 2022-07-06T22:51:04Z

apps/microtvm/reference-vm/README.md

   tool.
+
+## Versioning
+We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.


Suggested change

We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.

We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updates to the Zephyr SDK or Arduino board SDKs are considered major changes and require incrementing major version `X`.

guberti · 2022-07-06T22:51:39Z

apps/microtvm/reference-vm/README.md

   tool.
+
+## Versioning
+We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.


Do we use Z in X.Y.Z? If we don't (as I don't think we do) we should mention that it is rarely used.

guberti · 2022-07-06T22:52:08Z

apps/microtvm/reference-vm/README.md

+## Versioning
+We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.
+
+**Note**: We will release all microTVM RVM boxes under [microtvm](https://app.vagrantup.com/tlcpack/boxes/microtvm) and use box versioning in Vagrant file. Previous versions like `microtvm-zephyr`, `microtvm-arduino`, `microtvm-zephyr-2.5` and etc are not continued and will be removed in future.


Suggested change

**Note**: We will release all microTVM RVM boxes under [microtvm](https://app.vagrantup.com/tlcpack/boxes/microtvm) and use box versioning in Vagrant file. Previous versions like `microtvm-zephyr`, `microtvm-arduino`, `microtvm-zephyr-2.5` and etc are not continued and will be removed in future.

**Note**: We will release all microTVM RVM boxes under [microtvm](https://app.vagrantup.com/tlcpack/boxes/microtvm) and use box versioning in Vagrant file. Previous versions like `microtvm-zephyr`, `microtvm-arduino`, `microtvm-zephyr-2.5`, etc. are deprecated and will be removed in the future.

guberti · 2022-07-06T22:56:09Z

apps/microtvm/reference-vm/base-box-tool.py



-THIS_DIR = os.path.realpath(os.path.dirname(__file__) or ".")
+THIS_DIR = pathlib.Path(os.path.realpath(os.path.dirname(__file__) or "."))


Can we just do:

Suggested change

THIS_DIR = pathlib.Path(os.path.realpath(os.path.dirname(__file__) or "."))

THIS_DIR = pathlib.Path(__file__)

If __file__ isn't defined for some reason, I think the previous code would throw an error anyway.

guberti · 2022-07-06T22:57:12Z

apps/microtvm/reference-vm/base-box-tool.py

+            tvm_home_m = VM_TVM_HOME_RE.match(line)
+
+            if tvm_home_m:
+                f.write(f'{tvm_home_m.group(1)} = "../../../.."\n')


What is this line doing? Can you add a comment?

it is for adjusting tvm home in testing step

guberti · 2022-07-06T22:57:57Z

apps/microtvm/reference-vm/base-box/base_box_provision.sh

@@ -20,12 +20,23 @@
 #   virtual machine similar to CI QEMU setup.
 #

-set -e


Why are we removing this?

because it is already added in first line

it's not terrible to do this explicitly in the script actually--the first line is only ran when you directly invoke it as a command rather than just via bash path/to/script.sh

guberti · 2022-07-06T22:58:27Z

apps/microtvm/reference-vm/base-box/base_box_test.sh

+    else
+        pytest tests/micro/arduino/test_arduino_rpc_server.py --arduino-board=${board}
+    fi
+fi


guberti · 2022-07-06T22:59:30Z

apps/microtvm/reference-vm/base-box/base_box_test.sh

 fi

+platform=$1
 board=$1


Suggested change

board=$1

board=$2

?

mehrdadh

@guberti I addressed the comments. thanks for the review!
PTAL

mehrdadh · 2022-07-06T23:10:55Z

apps/microtvm/reference-vm/README.md

   tool.
+
+## Versioning
+We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.


mehrdadh · 2022-07-06T23:13:00Z

apps/microtvm/reference-vm/README.md

   tool.
+
+## Versioning
+We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.


mehrdadh · 2022-07-06T23:13:29Z

apps/microtvm/reference-vm/README.md

+## Versioning
+We use semantic versioning as it is recommended by [Vagrant](https://www.vagrantup.com/docs/boxes/versioning). We use `X.Y.Z` version where we maintain the same major version `X` it has minor changes and newer version is still compatible with older versions and we increase minor version `Y`. However, We increase the major version `X` when new RVM is not compatible with older onces. Updating Zephyr SDK is considered a major change and it requires incrementing major version `X`.
+
+**Note**: We will release all microTVM RVM boxes under [microtvm](https://app.vagrantup.com/tlcpack/boxes/microtvm) and use box versioning in Vagrant file. Previous versions like `microtvm-zephyr`, `microtvm-arduino`, `microtvm-zephyr-2.5` and etc are not continued and will be removed in future.


mehrdadh · 2022-07-06T23:15:27Z

apps/microtvm/reference-vm/base-box-tool.py

+            tvm_home_m = VM_TVM_HOME_RE.match(line)
+
+            if tvm_home_m:
+                f.write(f'{tvm_home_m.group(1)} = "../../../.."\n')


mehrdadh · 2022-07-06T23:15:41Z

apps/microtvm/reference-vm/base-box-tool.py

+            tvm_home_m = VM_TVM_HOME_RE.match(line)
+
+            if tvm_home_m:
+                f.write(f'{tvm_home_m.group(1)} = "../../../.."\n')


it is for adjusting tvm home in testing step

mehrdadh · 2022-07-06T23:16:23Z

apps/microtvm/reference-vm/base-box/base_box_provision.sh

@@ -20,12 +20,23 @@
 #   virtual machine similar to CI QEMU setup.
 #

-set -e


because it is already added in first line

mehrdadh · 2022-07-06T23:17:48Z

apps/microtvm/reference-vm/base-box/base_box_test.sh

 fi

+platform=$1
 board=$1


mehrdadh · 2022-07-06T23:17:58Z

apps/microtvm/reference-vm/base-box/base_box_test.sh

+    else
+        pytest tests/micro/arduino/test_arduino_rpc_server.py --arduino-board=${board}
+    fi
+fi


mehrdadh · 2022-07-06T23:25:52Z

apps/microtvm/reference-vm/base-box-tool.py



-THIS_DIR = os.path.realpath(os.path.dirname(__file__) or ".")
+THIS_DIR = pathlib.Path(os.path.realpath(os.path.dirname(__file__) or "."))


guberti

Thanks for the changes! LGTM.

guberti · 2022-07-07T18:04:40Z

While testing this PR on physical hardware, me and @mehrdadh ran into #12033. However, I'm confident that the test failure is unrelated to the changes in this PR, so I'm cool with this PR being merged.

areusch · 2022-07-11T16:27:07Z

apps/microtvm/reference-vm/base-box/base_box_provision.sh

@@ -20,12 +20,23 @@
 #   virtual machine similar to CI QEMU setup.
 #

-set -e


it's not terrible to do this explicitly in the script actually--the first line is only ran when you directly invoke it as a command rather than just via bash path/to/script.sh

github-actions bot requested a review from areusch July 6, 2022 17:52

move scripts

6a3341f

mehrdadh force-pushed the micro/refactor_two_rvms branch from 5f28742 to 6a3341f Compare July 6, 2022 17:54

github-actions bot requested a review from gromero July 6, 2022 17:54

mehrdadh added 3 commits July 6, 2022 11:47

lint

0c98231

fix path

ec8dbe1

lint

6942c09

guberti requested changes Jul 6, 2022

View reviewed changes

mehrdadh commented Jul 6, 2022

View reviewed changes

address comments

141f7c2

guberti approved these changes Jul 7, 2022

View reviewed changes

areusch approved these changes Jul 11, 2022

View reviewed changes

mehrdadh merged commit 04db26e into apache:main Jul 11, 2022

mehrdadh deleted the micro/refactor_two_rvms branch July 11, 2022 18:37

masahi pushed a commit to masahi/tvm that referenced this pull request Jul 15, 2022

[microtvm][RVM] Refactor Arduino/Zephyr into one RVM (apache#12023)

ee2016c

junrushao pushed a commit to yelite/tvm that referenced this pull request Jul 27, 2022

[microtvm][RVM] Refactor Arduino/Zephyr into one RVM (apache#12023)

4808c1c

AndrewZhaoLuo mentioned this pull request Oct 4, 2022

TVM v0.10.0.rc0 Release Candidate Notes #12979

Closed

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022

[microtvm][RVM] Refactor Arduino/Zephyr into one RVM (apache#12023)

cbf4396

mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023

[microtvm][RVM] Refactor Arduino/Zephyr into one RVM (apache#12023)

cad119b

	Reference VM is organized in this directory as follows:
	The Reference VM is organized in this directory as follows:

	Note: We will release all microTVM RVM boxes under [microtvm](https://app.vagrantup.com/tlcpack/boxes/microtvm) and use box versioning in Vagrant file. Previous versions like `microtvm-zephyr`, `microtvm-arduino`, `microtvm-zephyr-2.5` and etc are not continued and will be removed in future.
	Note: We will release all microTVM RVM boxes under [microtvm](https://app.vagrantup.com/tlcpack/boxes/microtvm) and use box versioning in Vagrant file. Previous versions like `microtvm-zephyr`, `microtvm-arduino`, `microtvm-zephyr-2.5`, etc. are deprecated and will be removed in the future.



		THIS_DIR = os.path.realpath(os.path.dirname(__file__) or ".")
		THIS_DIR = pathlib.Path(os.path.realpath(os.path.dirname(__file__) or "."))

	THIS_DIR = pathlib.Path(os.path.realpath(os.path.dirname(__file__) or "."))
	THIS_DIR = pathlib.Path(__file__)

[microtvm][RVM] Refactor Arduino/Zephyr into one RVM #12023

[microtvm][RVM] Refactor Arduino/Zephyr into one RVM #12023

Uh oh!

Conversation

mehrdadh commented Jul 6, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guberti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehrdadh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guberti left a comment

Choose a reason for hiding this comment

Uh oh!

guberti commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

mehrdadh commented Jul 6, 2022 •

edited by github-actions bot

Loading

guberti commented Jul 7, 2022 •

edited

Loading