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

Implement external custom components installing from YAML #1630

Merged
merged 21 commits into from May 7, 2021

Conversation

OttoWinter
Copy link
Member

@OttoWinter OttoWinter commented Mar 21, 2021

What does this implement/fix?

A prototype for #1554

This implements a new system for downloading and including custom components straight from YAML. Note: early PoC - syntax will change.

Important: This also changes the python requirement to 3.7 (previously was 3.6). Now that the docker base image is running 3.7 and most distributions have 3.7+ already, that should not be a major problem.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (this will require users to update their yaml configuration files to keep working)

Related issue or feature (if applicable): fixes TODO

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#1107

Test Environment

  • ESP32
  • ESP8266
  • Windows
  • Mac OS
  • Linux

Example entry for config.yaml:

# Set the component bh1750 to be imported from GitHub esphome/esphome repo on branch importlib-...
external_components:
  - source: esphome/esphome@importlib-custom-components
    components: [bh1750]

Explain your changes

This PR is split in two (large) commits:

  • First, I refactored and rewrote the component loader to use importlib's MetaPathFinder system
  • The second commit implements the external_components system.

The expected format for the repository is:

<REPO_ROOT>/components/my_custom_component/* (this directory must include at least a __init__.py file); this is how I expect custom component creators to present the code.

<REPO_ROOT>/esphome/components/my_custom_component/* (this directory must include at least a __init__.py file); this format is used mainly for being able to import certain components from a specific esphome branch/version

For now, only git and local sources are supported, others can be added later.

Future work includes refreshing the repository every so often.

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@OttoWinter OttoWinter changed the title Move components import loading to importlib MetaPathFinder and importlib.resources Implement external custom components installing from YAML Mar 21, 2021
Copy link
Member

@glmnet glmnet left a comment

Choose a reason for hiding this comment

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

This is definitely simplify component sharing on the community. 🎉🎉

esphome/components/external_components/__init__.py Outdated Show resolved Hide resolved

if config[CONF_COMPONENTS] == "all":
num_components = len(list(components_dir.glob("*/__init__.py")))
if num_components > 100:
Copy link
Member

Choose a reason for hiding this comment

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

Beside this which mostly indicates an user entering wrong source url, I would add a check where it explicitly needs to indicate overriden ESPHome bundled components. So if you want a new fancy captive_portal then you must indicate it here, otherwise ESPHome's is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. On the other hand, doing so would be a problem for some external integrations.

For example: I create an external repo for my fancy new display driver xyz9000 and publish that. Then I want to add another display like xyz9001, but at some point I want to refactor some of the shared code into a new module xyz_base.

In that case, if we force users to manually set the components, we would really limit external component repos - getting all users to change their config would be painful.

I don't think stuff like display drivers will overwrite some internal stuff like captive_portal - or if it does end up being a problem we can create a policy where certain "core" components have to explicitly opted in. We can do that later too because requiring that would not be a big breaking change (I don't suspect many external integrations will overwrite core stuff)

return SOURCE_SCHEMA({CONF_TYPE: TYPE_LOCAL, CONF_PATH: value})
except cv.Invalid:
pass
# Regex for GitHub repo name with optional branch/tag
Copy link
Member

Choose a reason for hiding this comment

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

is this a standard like the way platformio or GH actions names refers to github repos? its a bit magical as no mention to github is made on the string itself, like github:// etc. just an opinion

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, that syntax is based on GH actions'.

The reason I don't want a full URL like https://github.com/esphome/esphome@dev is because then with the @dev part it wouldn't be an actual URL (as in if you type it in your browser you'll get an error page)

github:// could be a good alternative though

@glmnet
Copy link
Member

glmnet commented Apr 5, 2021

I was thinking about this, one minor issue custom_components mechanism has now is that if you put a component there, it will be applied to all your components, that makes sense on HA as you only run one Home Assistant, hwoever all yaml files will share the included elements in custom_components

While this solves the issue, as you can use the file:// in source in a single yaml then you can have a custom component just for one yaml, I believe a mix of this component serving custom_components folder by default will fit better.

I propose a default (always on) configuration for external_components which will load custom_components folder:

external_components:
  - type: local
    path: './custom_components'
    components: 'all'

So that configuration is default, and it will act as current default behavior of ESPHome, but can be overriden so you can:

external_components:

Will disable loading of custom_components
and you can also override to have it load just one component on the custom_components folder:

external_components:
  - type: local
    path: './custom_components'
    components: [ 'rtttl' ]

This might also plain the path to make mandatory setting of the component to be overriden, allowing all as is might yield unexpected results, e.g. if I load a component from a repo, and then the a new component shows up which overrides one of my components I might not notice.

@OttoWinter
Copy link
Member Author

@glmnet I see what you mean with the custom_components dir. One major thing to consider is that default configs for lists can be very unintuitive. For example: very early on ESPHome would automatically apply a smoothing sliding_window_moving_average filter for every sensor, but we'd get tons of reports of "why is the output suddenly so different when I add this other filter".

Maybe we can do something like HA instead: Print a warning message for every component loaded through the /custom_components directory. external_components clearly is the way even local experiments should be implemented with in the future, as it's a safer alternative where the user explicitly opts in. By having a good alternative, the warning message would also not be a big problem.

@SenexCrenshaw SenexCrenshaw mentioned this pull request Apr 6, 2021
3 tasks
@glmnet
Copy link
Member

glmnet commented Apr 6, 2021

Dashboard screenshot:
image

@OttoWinter OttoWinter mentioned this pull request Apr 7, 2021
12 tasks
@glmnet glmnet merged commit 229bf71 into dev May 7, 2021
@glmnet glmnet deleted the importlib-custom-components branch May 7, 2021 18:02
This was referenced May 9, 2021
martgras pushed a commit to martgras/esphome that referenced this pull request May 13, 2021
* Move components import loading to importlib MetaPathFinder and importlib.resources

* Add external_components component

* Fix

* Fix

* fix cv.url return

* fix validate shorthand git

* implement git refresh

* Use finders from sys.path_hooks instead of looking for __init__.py

* use github:// schema

* error handling

* add test

* fix handling git output

* revert file check handling

* fix test

* allow full component path be specified for local

* fix test

* fix path handling

* lint

Co-authored-by: Guillermo Ruffino <glm.net@gmail.com>
This was referenced May 18, 2021
@OttoWinter
Copy link
Member Author

@glmnet Thank you so much for finishing this PR and the docs! Really appreciated!

@jesserockz jesserockz mentioned this pull request Jun 18, 2021
9 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants