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

Cleaning packaging / CI config #451

Merged
merged 8 commits into from
Feb 8, 2024
Merged

Cleaning packaging / CI config #451

merged 8 commits into from
Feb 8, 2024

Conversation

lpascal-ledger
Copy link
Contributor

@lpascal-ledger lpascal-ledger commented Feb 7, 2024

  • Moved most of the package configuration from deprecated setup.py to pyproject.toml
    • except the custom (setuptools-specific) C emulator build part, which remains in the setup.py file.
    • added package qualifiers (Python versions, GPL3 license)
    • tool configurations (mypy, flake8) also moved into pyproject.toml.
  • Synchronized the dependency package versions into Pipefile / Pipfile.lockfrompyproject.toml(andsetup.pybefore it). Useful as some CI tests run usingpipenv(and soPipefile` and the rest) and dependabot was cringed by some old versions.
  • Updated all actions/checkout@ to v4
  • Pushed the sys.exit from speculos.py to speculos/main.py:main. The package entrypoint goes directly to speculos/main.py:main and I guess it is important that the speculos CLI returns useful status codes, instead of 0 always.
    • Is speculos.py ever used ? Given how the entrypoint is configured, speculos.py is totally bypassed when using the CLI speculos. Is it worth keeping this manual launcher?
  • Coverage CI
    • Moved from fast-checks.yml to continuous-intergation-workflow.yml. This job takes 5min+, it is not fast at all.
    • Fixed the codecov-action. Codecov is a fussy action which regularly breaks with unclear error messages. I updated the action version, added a token and explicitly declared the GITHUB_WORKSPACE a safe directory for git, and it seems to work now. Thank you codecov for taking me a couple of hours again.

…flows, + tests on all suppoerted Python versions
@lpascal-ledger lpascal-ledger force-pushed the clean/packaging branch 7 times, most recently from 45244ba to 32c302b Compare February 7, 2024 16:01
@xchapron-ledger
Copy link
Contributor

Is speculos.py ever used ? Given how the entrypoint is configured, speculos.py is totally bypassed when using the CLI speculos. Is it worth keeping this manual launcher?

I personally usually launch Speculos with a command like this:
~/Workspace/LedgerHQ/speculos/speculos.py bin/app.elf

xchapron-ledger
xchapron-ledger previously approved these changes Feb 7, 2024
Copy link
Contributor

@xchapron-ledger xchapron-ledger left a comment

Choose a reason for hiding this comment

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

A bit sad that we go from one setup.py file to still having a setup.py + having a pyproject.toml :/

Also, shouldn't we bump update the changelog and publish a version? Would allow to make sure that if it breaks something, we know it soon?

@lpascal-ledger
Copy link
Contributor Author

lpascal-ledger commented Feb 7, 2024

Yeah the custom build BuildSpeculos is a bit of a pain to migrate, with very few documentation on this sort of tricks. I tried a few strategies but couldn't make it work.
At least now all tool confs are centralized in the same place and the package is almost entirely declaratively defined.

As for the version, a new one will be deployed on pypi.org, only not with a stable release version, and we will be able to test it right now (with pip install --pre speculos). I intended to do it on one or two app test suite. You prefer with a stable release?

@lpascal-ledger
Copy link
Contributor Author

I just realized that the speculos/ Python package source directory coverage is not included into the final codecov coverage. I'll take a look at why.

@xchapron-ledger
Copy link
Contributor

As for the version, a new one will be deployed on pypi.org, only not with a stable release version, and we will be able to test it right now (with pip install --pre speculos). I intended to do it on one or two app test suite. You prefer with a stable release?

Probably good enough!

…tory while tests run against the 'speculos' package
@codecov-commenter
Copy link

Codecov Report

Attention: 1373 lines in your changes are missing coverage. Please review.

Comparison is base (745fe46) 48.95% compared to head (dff2e59) 45.37%.
Report is 116 commits behind head on master.

Files Patch % Lines
speculos/mcu/rle_custom.py 12.74% 486 Missing ⚠️
src/bolos/fonts_info.c 0.00% 124 Missing ⚠️
speculos/mcu/screen.py 0.00% 97 Missing ⚠️
speculos/mcu/bagl.py 0.00% 68 Missing ⚠️
speculos/mcu/ocr.py 24.09% 63 Missing ⚠️
speculos/mcu/display.py 57.00% 46 Missing ⚠️
speculos/mcu/seproxyhal.py 25.42% 44 Missing ⚠️
speculos/main.py 0.00% 39 Missing ⚠️
speculos/mcu/screen_text.py 0.00% 39 Missing ⚠️
src/bolos/nbgl.c 0.00% 39 Missing ⚠️
... and 25 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage   48.95%   45.37%   -3.59%     
==========================================
  Files         113      120       +7     
  Lines       10849    11891    +1042     
  Branches        0      950     +950     
==========================================
+ Hits         5311     5395      +84     
- Misses       5538     6161     +623     
- Partials        0      335     +335     

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

@lpascal-ledger
Copy link
Contributor Author

Alright the coverage is slightly better (not spectacular though), the Python package coverage seems to be included now.

@lpascal-ledger lpascal-ledger merged commit b09b240 into master Feb 8, 2024
25 checks passed
@lpascal-ledger lpascal-ledger deleted the clean/packaging branch February 8, 2024 13:22
@lpascal-ledger
Copy link
Contributor Author

lpascal-ledger commented Feb 8, 2024

Tested the new 0.5.1.dev15 Speculos version on functional tests on all devices for boilerplate, MISC and passwords apps, everything is correctly working (modulo a few passwords tests which are known to be unstable locally).

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