Skip to content

Add adafruit_esp32spi as dependency #75

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

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Add adafruit_esp32spi as dependency #75

merged 1 commit into from
Jul 18, 2022

Conversation

tekktrik
Copy link
Member

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

@Neradoc
Copy link
Contributor

Neradoc commented Jul 18, 2022

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
Copy link
Member Author

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
Copy link
Member Author

tekktrik commented Jul 18, 2022

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
Copy link
Collaborator

makermelissa commented Jul 18, 2022

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

@tekktrik
Copy link
Member Author

@makermelissa Should I merge this then?

@Neradoc
Copy link
Contributor

Neradoc commented Jul 18, 2022

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 makermelissa merged commit 3b0e5ab into adafruit:main Jul 18, 2022
@makermelissa
Copy link
Collaborator

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
Copy link
Member Author

tekktrik commented Jul 18, 2022

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
Copy link
Collaborator

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
Copy link
Member Author

Sure thing, I'll do that now.

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 19, 2022
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.

3 participants