search

adafruit / Adafruit_CircuitPython_PortalBase

Base Library for the Portal-style libraries.
MIT License
18 stars 17 forks source link

Add adafruit_esp32spi as dependency #75

Closed tekktrik closed 2 years ago

tekktrik commented 2 years ago

Adds the missing adafruit_esp32spi dependency, which is what is preventing #74 from passing CI as well.

Neradoc commented 2 years ago

PortalBase is used in adafruit_magtag, adafruit_funhouse, etc. ESP32SPI is only a dependency for a submodule that is not used on these boards. Why is this failing all of a sudden, when it used not to ?

tekktrik commented 2 years ago

Good question, both are using the same sphinx version so that shouldn't be it. For what it's worth I cloned the main branch to test it against the CI and it now fails as well: https://github.com/adafruit/Adafruit_CircuitPython_PortalBase/actions/runs/2690960012

tekktrik commented 2 years ago

I think it was always needed for the docs (or to be mock imported), but it looks like previous CI runs were installing it in what looks like a dependency of a dependency. Specifically it looks like the AdafruitIO library which you recently patched (5.6.8 --> 5.6.9) was masking this as a missing dependency. So because it was being installed there, it was available here for use as well. Now that it's been patched, it's a missing dependency for this one.

makermelissa commented 2 years ago

Yeah, this should be added. I'm not sure why/how it went under the radar this long.

tekktrik commented 2 years ago

@makermelissa Should I merge this then?

Neradoc commented 2 years ago

But adding ESP32SPI as a dependency will make it installed on every Magtag and FunHouse project using the project bundler or using circup. It should rather be a dependency in the portal libraries that actually require it, like PyPortal.

I believe it didn't happen before, because the dependency in adafruit_io was spelled with underscores: adafruit_circuitpython_esp32spi, which the bundler apparently does not recognize. Also there is the case of having it in setup.py (so installed on PC) but not in requirements.txt (used for boards).

makermelissa commented 2 years ago

But adding ESP32SPI as a dependency will make it installed on every Magtag and FunHouse project using the project bundler or using circup. It should rather be a dependency in the portal libraries that actually require it, like PyPortal.

I believe it didn't happen before, because the dependency in adafruit_io was spelled with underscores: adafruit_circuitpython_esp32spi, which the bundler apparently does not recognize. Also there is the case of having it in setup.py (so installed on PC) but not in requirements.txt (used for boards).

Hmm, I didn't see this before merging. Those are some good points. Perhaps we should just have it in autodoc_mock_imports. I can either revert this or we can just do it all in a new PR.

tekktrik commented 2 years ago

Looking at the CI of the failed builds and the previous one in main that succeeded, the underscores weren't a probably for pip installing from requirements.txt, so it's the removal of adafruit_esp32spi from adafruit_adafruitio's requirements.txt that's changed, which makes sense since pip shouldn't care in that way if it's underscores or hyphens. The CI log shows it was installed on the previously good run of the CI prior to adafruit_adafruitio's upgrade to 5.6.9 (removal of adafruit_esp32spi from it's own requirements.txt), but not anymore since.

Unfortunately due to the way the CI handles installs (and the way pyproject.toml will work in the end anyway), removing it from requirements.txt will mean it won't get installed (even in the meantime if it's in setup.py). I think it makes sense to mock import based on what @Neradoc is saying since it sounds like requirements.txt is going to end up being used for the bundler and pip in the end, so better to just mock it here in the base library.

makermelissa commented 2 years ago

Unfortunately due to the way the CI handles installs (and the way pyproject.toml will work in the end anyway), removing it from requirements.txt will mean it won't get installed (even in the meantime if it's in setup.py). I think it makes sense to mock import based on what @Neradoc is saying since it sounds like requirements.txt is going to end up being used for the bundler and pip in the end, so better to just mock it here in the base library.

Ok, want to submit another PR?

tekktrik commented 2 years ago

Sure thing, I'll do that now.