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

Change build to use poetry #606

Merged
merged 8 commits into from
Jun 26, 2024
Merged

Conversation

geeksville
Copy link
Member

@geeksville geeksville commented Jun 21, 2024

I'm just opening this PR (as draft hopefully), so I can watch all the horrible ways this breaks CI. I'll rebase and edit (and add docs) before this is ready to submit.

remaining TODOS:

  • DONE update with recent requirements.txt changes
  • DONE update with latest setup.py and delete setup.py
  • DONE split tunnel out as an optional variant
  • DONE document how to install/use/poetry
  • DONE document how to use "poetry shell" to facilitate debugging
  • NOT DONE - SEE BELOW test pypi upload from poetry

@geeksville geeksville marked this pull request as draft June 21, 2024 22:38
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.78%. Comparing base (b15e27c) to head (e6a88e0).

Current head e6a88e0 differs from pull request most recent head 195f0c9

Please upload reports for the commit 195f0c9 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
- Coverage   61.99%   61.78%   -0.21%     
==========================================
  Files          14       14              
  Lines        3005     2986      -19     
==========================================
- Hits         1863     1845      -18     
+ Misses       1142     1141       -1     
Flag Coverage Δ
unittests 61.78% <ø> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

still need to update readme and pypi upload
* add script to run 'act' local github actions tool (lets devs check github
actions on their local machine)
* Update various github actions to latest (so they can work with the 'act'
tool)
* change a few places where python version was not properly quoted as a
string (act yaml parser is more strict than the github version)
* update pylint min-version to work with recent github actions
* remove pandas/riden requirement (that's in my other branch for now)
@CLAassistant
Copy link

CLAassistant commented Jun 22, 2024

CLA assistant check
All committers have signed the CLA.

add pyinstaller as a dev dep.  Use it to make "bin/build-bin.sh"
remove old version scripts (no longer needed with poetry)
@geeksville
Copy link
Member Author

Hi ya'll!

So I think this PR is probably good to go, except for three caveats:

  • I'm waiting to push the docs to the web group until after this change is in. You can see the new docs here: Update python developer instructions to use poetry meshtastic#1309

  • I've only half tested the github release workflow. I think it is probably good (tested most of it locally on my desktop using the act tool). When the new python leads (@ianmcorvidae ?) are next ready to do a release they might find (small?) problems. I'm happy to help then as needed.

  • I chose to not use the (built-in) pypi upload support of poetry so as to minimize the changes to the release workflow CI script. Instead it just builds to dist/ like the old setup.py version.

Thoughts? Please review kinda carefully. Poetry means we no longer have a setup.py/requirements.txt or the various crufty scripts I wrote to hand increment version.

I think if you haven't used poetry before you might really dig it. It makes everything much more standardized and seems to be the 'modern' way of robustly developing/releasing python packages.

btw: I added a run-ci-local.sh script to bin. It allows the user to run the github CI actions on your desktop which is super useful for testing this stuff.

@geeksville geeksville marked this pull request as ready for review June 22, 2024 16:59
@geeksville geeksville requested a review from ianmcorvidae June 22, 2024 17:00
@geeksville geeksville changed the title Change build to use poetry (WIP - do not merge) Change build to use poetry Jun 22, 2024
@geeksville
Copy link
Member Author

fixes #604

pyproject.toml Outdated
pyyaml = "^6.0.1"
pypubsub = "^4.0.3"
bleak = "^0.21.1"
mypy = "^1.10.0"
Copy link

Choose a reason for hiding this comment

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

Sorry, I know nothing about Poetry but Is it possible to separate out the things that are needed for testing vs. runtime?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - there is a separate dev depends. bleak and pypubsub need to stay in the runtime depends. But good point mypy can move to the dev depends. I'm not sure about pyyaml - haven't looked to see what that's used for.

Copy link
Member Author

Choose a reason for hiding this comment

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

pyyaml is used for some sort of save as yaml feature added sometime ago - so it needs to stay in runtime deps.

Copy link
Contributor

@ianmcorvidae ianmcorvidae left a comment

Choose a reason for hiding this comment

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

Sorry it took me a bit to get to this! Looks good to me, so I'll merge it now. I also just merged the PR removing the need for the timeago library, so I'll handle any conflicts there/remove that library from our dependencies after merging.

@ianmcorvidae ianmcorvidae merged commit 68836b1 into meshtastic:master Jun 26, 2024
9 checks passed
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.

4 participants