-
Notifications
You must be signed in to change notification settings - Fork 38
Add support for pinned bundle versions #259
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
Conversation
This adds support for pinning the version of any bundle used. If a pinned version is found in the 'pyproject.toml' that will be used as the bundle version when running circup commands. Otherwise, the behavior remains unchanged and circup will use the latest available bundle version.
|
Fixed up the failing tests and added tests for this feature. |
|
I tested this today and can confirm it seems to work as intended. I think it's a little awkward to have to configure this with a pyproject.toml file, other options for circup like the path to the target device, and host/port for web workflow are currently accepted only as command line arguments. I am in favor of all configurations for the utility supporting the same input mechanism rather than the user needing to use different things to input different configuration options. So I would lean towards changing this from using That being said I can see convenience value in taking values from pyproject.toml as well. I would not be opposed if we wanted to implement that as one possible way to input the circup configurations, if we make it so the rest of the configurations beyond just the pinned bundle version can be done that way too so that the user doesn't have to keep track of which configs must be done in which different way. |
circup/command_utils.py
Outdated
| logger.info("Looking for pyproject.toml file.") | ||
| cwd = Path.cwd() | ||
| candidates = [cwd] | ||
| candidates.extend(cwd.parents) |
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.
This extends the search all the way back to the root of the system (on linux), I think that is a bit of an extreme length to go looking for a pyproject.toml file.
I would be in favor of limiting this to either the direct parent only, or maybe 1 level above that if you feel its warranted.
If very deeply nested project structures beyond that are a concern perhaps we could also just allow the pyproject.toml file path to be specified by the user with a command line argument so that we aren't searching around too many places for it.
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.
If you feel strongly about it, sure, I can make it so that it only checks the current directory and the parent. I went with this as it is how poetry and uv look for a pyproject.toml file. In fact, I just used poetry's implementation.
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 current directory and parent would be good. I think the further away it gets upward from the current directory it increases the chances of finding an unrelated pyproject.toml file and then having unexpected behavior that is a little tricky to trace down.
This seems fair, and I agree the goal here should be to prioritize simplicity. I think we might be thinking of what is the purpose of this feature slightly differently. I tried to describe the reason I felt "pinning" would be useful in #258. I realize I forgot to link that issue to this PR so that's my fault. But to summarize, I'm thinking of pinning as the same thing as the Currently, if you run Using I don't know how most people use As another point, I don't actually feel But I'd love to know what you think @FoamyGuy and how you see "pinning" getting used. |
|
Okay, I've read up on the issue and your summary above, that does all make good sense to me as well. I do think I fall more into the "manually circup install 'library'..." category with my usage, so I was indeed thinking about it a bit differently from how you described it here and the issue. I don't use circup with requirements.txt much.
Yeah that is a good point, pin-bundle won't work out well.
That is fair, and pyproject.toml does neatly solve all of those problems so I can see it's appeal. How about keeping the existing behavior but adding As an aside to the bundle tags but still to the core of this issue: It should be possible to make circup able to install individual libraries at specific versions also by downloading .py or .mpy files from the release assets of the library repo on github. Then it could behave more like |
I like that idea 👍
That's interesting. I think that would be really cool, it would completely remove the need for bundle pinning, and maybe for using bundles altogether in |
This provides a way to specify a specific bundle version for a given bundle through the command line. If a value is given from the command line and a pinned version is also found in the pyproject.toml, the value from the command line will be used.
This will output the necessary text to add to the pyproject.toml to pin the bundles that are used for any modules that are currently on the device. Similar to regular freeze, just for bundles.
|
Okay, added a I also added a |
# Conflicts: # circup/command_utils.py
FoamyGuy
left a comment
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.
This looks goo to me. Thanks for working on this @dunkmann00! I think this is great functionality to have.
I tested the --bundle-version option successfully. Good call on moving that to the main options instead of inside install.
I did limit the pyproject.toml search to cwd and parent and tested to ensure both places still correctly find the pinned version.
|
Thanks! Glad to get this in so quickly!
Okay, that sounds fine! I would like to note just in case this happens to come up in the future that I did check other python tools and they all checked back up to the root (poetry, uv, ruff, pytest, pylint, black). |
This adds support for pinning the version of any bundle used. If a pinned version is found in the
pyproject.tomlthat will be used as the bundle version when runningcircupcommands. Otherwise, the behavior remains unchanged andcircupwill use the latest available bundle version.I've not added any tests yet, happy to do so if this addition is welcome.
Closes #258