-
Notifications
You must be signed in to change notification settings - Fork 37
Only install platforms needed for current device #262
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
base: main
Are you sure you want to change the base?
Conversation
Rather than installing all the platforms from the global PLATFORMS variable, only install source (py) and the compiled (mpy) version that the currently connected device needs. This allows some nice improvements: 1) Only need to download at most, 2 versions of a bundle, not however many exist in PLATFORMS. 2) A device doesn't NEED the compiled versions. So with this in mind, this change adds the behavior where if a compiled bundle can't be found, it falls back to installing the source version. 3) Because of 2, a bundle maintainer doesn't have to provide compiled bundles to use their bundles with circup. If only providing source bundles is sufficient, circup is now happy with that. It will check for and try to download a compiled bundle, but if it doesn't find it, it just falls back to the source bundle. 4) Also because of 2, if someone is using an older version of circuitpython that is no longer getting compiled bundle builds, they can also still use circup. In this case it again falls back to the source bundle (or pin to an older bundle version that does have the compiled bundle for the version of circuitpython being used...circup no longer only supports specific platforms so if a platform bundle exists, it can use it).
| # TODO: Need to pass int to sorted key...otherwise this might not sort them how it should | ||
| self._available = sorted(tags) |
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.
Do we still the changed mention here?
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.
I address this in #263, so depending which (if either) gets merged first I will have to do some updating and can remove this after that
|
|
||
|
|
||
| def get_bundle(bundle, tag): | ||
| def get_bundle(bundle, tag, platform): |
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.
for this and the other functions where platform or platform_version argument has been added can you please add them to the docstrings as well.
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.
Okay, think I fixed this, let me know if I missed any others!
|
|
||
| #: Module formats list (and the other form used in github files) | ||
| PLATFORMS = {"py": "py", "9mpy": "9.x-mpy", "10mpy": "10.x-mpy"} | ||
| PLATFORMS = {"py": "py", "8mpy": "8.x-mpy", "9mpy": "9.x-mpy", "10mpy": "10.x-mpy"} |
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.
I think 8.x was removed from here intentionally when we stopped supporting it.
If we're going to add it back here and allow circup to install libraries from it, I think we should perhaps also have it print a message that the older bundles (and versions of CircuitPython) are deprecated and it's recommended to update to the supported version of CP/Bundle mpy.
It's maybe worth discussing in the weeds during a meeting to get input from the rest of the team as well.
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.
The idea now is circup isn't deciding what is and isn't supported. If I plug in a device with CP 8 on it, I don't get to choose what compiled bundle I can use, I need 8...if 8 doesn't exist because it isn't supported, circup can handle that by falling back to the source version instead.
So Adafruit might no longer build bundle's with 8 (and at some point drop support for 9), but it's quite possible a bundle maintainer of some other third party bundle does still want to support 8 (and at some point 9). In this PR, PLATFORMS changes from indicating that "these are the supported versions," and becomes only a mapping to find the download URL.
I think what you are saying is valid, but I'm not sure if the responsibility should fall on circup to let the user know something is deprecated/unsupported. Mostly because that would be up to the bundle maintainer, not circup.
Happy to discuss this more if it would be helpful to figure out the best behavior!
Rather than installing all the platforms from the global
PLATFORMSvariable, only install source (py) and the compiled (mpy) version that the currently connected device needs.This allows some nice improvements:
PLATFORMS.circup. If only providing source bundles is sufficient,circupis now happy with that. It will check for and try to download a compiled bundle, but if it doesn't find it, it just falls back to the source bundle.circuitpythonthat is no longer getting compiled bundle builds, they can also still usecircup. In this case it again falls back to the source bundle (or pin to an older bundle version that does have the compiled bundle for the version ofcircuitpythonbeing used...circupno longer only supports specific platforms so if a platform bundle exists, it can use it).What first prompted this was trying to find someone else's
circuitpythonbundle to test for one of the other PRs I did. I found sparkfun had made one but they hadn't updated it in a while and didn't have the10.x-mpyplatform bundle. Because of that,circupwouldn't even let me add their bundle because it failed to validate successfully. So from that I kinda started to think about it and wonder if there was a different way to go about handling platforms incircup. I figured it was kind of something I could try to play around with and see if I could put something together that made sense/was not too difficult to do. Ultimately, I'm happy with the end result.But let me know what you think or if you have any questions about it!