SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
699 stars 161 forks source link

Remove unused dependencies from requirements.txt #543

Closed dbast closed 4 months ago

dbast commented 5 months ago

This removes numpy and six from requirements.txt as there are no left references in the code and moves numpy to the requirements-raspi.txt file as it is a transitive picamera dependency.

Description

Describe the change simply. Provide a reason for the change.

Include screenshots of any new or modified screens (or at least explain why they were omitted)

This pull request is categorized as a:

Checklist

If you modified or added functionality/workflow, did you add new unit tests?

I have tested this PR on the following platforms/os:

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

jdlcdl commented 5 months ago

I recall that while numpy is not used directly in seedsigner, it was still needed as a dependency for picamera. That leaving numpy in requirements.txt ensured this dependency gets built.

dbast commented 5 months ago

should numpy then move to requirements-raspi.txt (where also picamera is referenced)?

jdlcdl commented 5 months ago

Yes @dbast,

I believe that you are correct.

My recollection of trying to remove numpy from requirements was from https://github.com/SeedSigner/seedsigner/pull/368 ...and at that point in time, we had not yet separated requirements.txt into two versions.

Ideally, I think that installing picamera should require numpy as a dependency, but as long as we're requiring it in our own (maybe with a comment), then requirements-raspi.txt seems best.

@kdmukai and @newtonick, do you foresee any problems with that?

UPDATED: I've successfully run the test-suite w/o errors:

dbast commented 5 months ago

Moved numpy to requirements-raspi.txt :)

newtonick commented 5 months ago

LGTM but I want to do a full build test before merging.

newtonick commented 4 months ago

Tested