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

feat(plugins): add Matter plugin support #4491

Merged
merged 27 commits into from
Apr 9, 2024
Merged

Conversation

MonicaisHer
Copy link
Contributor

@MonicaisHer MonicaisHer commented Dec 11, 2023

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint?
  • Have you successfully run pytest tests/unit?

This PR adds a new Matter plugin which is useful for building Matter SDK based parts.

This PR could be merged once the following tasks been completed:

  • add unit test
  • add spread test to build a matter snap

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (d75fdaf) 89.19% compared to head (18b3a68) 89.15%.
Report is 5 commits behind head on main.

❗ Current head 18b3a68 differs from pull request most recent head 7c484e2. Consider uploading reports for the commit 7c484e2 to get more accurate results

Files Patch % Lines
snapcraft/parts/plugins/matter_plugin.py 53.48% 20 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4491      +/-   ##
==========================================
- Coverage   89.19%   89.15%   -0.05%     
==========================================
  Files         322      323       +1     
  Lines       21826    21850      +24     
==========================================
+ Hits        19468    19480      +12     
- Misses       2358     2370      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

It is a very good start, which can be followed by adding tests.

My biggest concern on the consumer side based on your example is that every time the application code changes, the Matter SDK data will be removed, and the bootstrapping process (the most resource consuming step) will be repeated from scratch. This is because the plugin and application data are maintained under the same part's build directory, which gets cleared when app sources change.

snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
@syu-w
Copy link
Contributor

syu-w commented Dec 15, 2023

Also you can run tox -m lint locally to check any linters error so you fix them quicker.

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

In addition to unit tests, we'll also need a spread test to build a hello world matter snap.

It would probably live in tests/spread/plugins/craft-parts, but possibly in tests/spread/plugins/craft-parts/build-and-run-hello if you can build an app that outputs 'Hello world" when executed.

snapcraft/parts/plugins/matter_plugin.py Outdated Show resolved Hide resolved
- address linter errors
- improve readability
* Add matter plugin unit test
* Add craft parts matter plugin test
* Export zap and pigweed environment variables
* Fix tox linter
* Improve formatting
@MonicaisHer
Copy link
Contributor Author

It is a very good start, which can be followed by adding tests.

My biggest concern on the consumer side based on your example is that every time the application code changes, the Matter SDK data will be removed, and the bootstrapping process (the most resource consuming step) will be repeated from scratch. This is because the plugin and application data are maintained under the same part's build directory, which gets cleared when app sources change.

Thank you for the review. The usage of consumer snap has been improved and added to tests. The Matter SDK plugin has now become a standalone part and the application building part depending on it. This means that each time the application building part is modified, the building will be much faster, as the Matter SDK plugin will not need to run again.

@MonicaisHer
Copy link
Contributor Author

Also you can run tox -m lint locally to check any linters error so you fix them quicker.

Thanks for the hint! It is very helpful.

@MonicaisHer
Copy link
Contributor Author

MonicaisHer commented Jan 26, 2024

In addition to unit tests, we'll also need a spread test to build a hello world matter snap.

It would probably live in tests/spread/plugins/craft-parts, but possibly in tests/spread/plugins/craft-parts/build-and-run-hello if you can build an app that outputs 'Hello world" when executed.

Thanks for providing testing guidance.The unit test and spread test have been added. There is a snap using the Matter SDK plugin for spread test.

In the spread test, right now it checks the Matter SDK related functionality whether the application was successfully initialized and whether the storage path was successfully replaced. I want to ensure that I understand the requirements correctly. Is it required for the app built in the spread test also to output "Hello world" when executed, or is it simply a suggested outcome?

@MonicaisHer MonicaisHer marked this pull request as ready for review January 26, 2024 09:21
@MonicaisHer
Copy link
Contributor Author

This PR is ready for re-review. Could you please take a look? Thanks! @syu-w @sergiusens @mr-cal @cmatsuoka

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

I think everything you have implemented looks really good. The plugin, unit, and spread tests are all well done. And nice work satisfying all the linters!

I echo Sergio's concern - a lot of code is required in an override-build script to build the example snap. I don't know much about Matter, but I wonder if more of that can be hidden from the user via the plugin, an extension, or both. I agree that we should have a meeting!

tests/spread/plugins/craft-parts/matter-sdk/task.yaml Outdated Show resolved Hide resolved
tests/unit/parts/plugins/test_matter_sdk_plugin.py Outdated Show resolved Hide resolved
tests/unit/parts/plugins/test_matter_sdk_plugin.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks!

Since this plugin is experimental and there is not an established user base, I am OK with the design.

For posterity, the scope of work and design choices are covered in IENG-830.

Copy link
Contributor

@syu-w syu-w left a comment

Choose a reason for hiding this comment

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

Tried it locally and it seems to work fine

@syu-w syu-w changed the title Add Matter plugin feat(plugins): add Matter plugin support Mar 20, 2024
Co-authored-by: Sergio Schvezov <[email protected]>
@mr-cal
Copy link
Collaborator

mr-cal commented Mar 28, 2024

@MonicaisHer - Do you mind resolving the last file conflict? After that, we can land this PR if you are ready

@mr-cal mr-cal merged commit 6eb812d into canonical:main Apr 9, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants