-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Better version handling for Arduino #11043
Better version handling for Arduino #11043
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guberti Hi. Thanks for continuing to support Arduino on microTVM :)
I've got some comments inline but overall the change LGTM!
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py
Outdated
Show resolved
Hide resolved
Thanks for the comments @gromero! I've addressed them all, and made a small change to the way we compare versions (we now use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guberti Thanks for the fixes and for adding the test for warning_as_error
! I agree on using packaging
for the version checks. I think I'll use it also for Zephyr :)
I just have one comment - a nit - but won't hold the change on it. Feel free to change it if it makes sense of leave it unchanged.
Finally, out of curiosity, do you know why these Arduino tests are kept in template_project/tests
instead of in tests/micro/arduino
with the other tests? Should we move everything into tests/micro/arduino
?
apps/microtvm/arduino/template_project/tests/test_arduino_microtvm_api_server.py
Outdated
Show resolved
Hide resolved
The original thinking was that unit tests would live in |
Got it! Yeah, that's OOS for this PR, I was just wondering about that organization. 👍 |
* Fix bug allowing microTVM to be used with Arduino version v0.20 and above (see changes to _parse_connected_boards) and adds relevant unit tests. * Only perform version check when calling build or flash (things that actually require arduino-cli), and adds relevant unit tests. * Only raise a warning if the arduino-cli version present is below the min version (previously any version other than v0.18 would cause an error). * Change version comparison to use version.check, like the rest of TVM
* Fix bug allowing microTVM to be used with Arduino version v0.20 and above (see changes to _parse_connected_boards) and adds relevant unit tests. * Only perform version check when calling build or flash (things that actually require arduino-cli), and adds relevant unit tests. * Only raise a warning if the arduino-cli version present is below the min version (previously any version other than v0.18 would cause an error). * Change version comparison to use version.check, like the rest of TVM
This pull request contains three small changes:
microTVM
to be used with Arduino versionv0.20
and above (see changes to_parse_connected_boards
) and adds relevant unit tests.build
orflash
(things that actually requirearduino-cli
), and adds relevant unit tests.arduino-cli
version present is below the min version (previously any version other thanv0.18
would cause an error).version.check
, like the rest of TVM