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

Remove unused dependencies from requirements.txt #543

Merged
merged 2 commits into from
May 1, 2024

Conversation

dbast
Copy link
Contributor

@dbast dbast commented Apr 15, 2024

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:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

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

  • No, I’m a fool
  • Yes
  • N/A

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.

This removes numpy and six from requirements.txt as there are no left
references in the code.
@jdlcdl
Copy link

jdlcdl commented Apr 15, 2024

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
Copy link
Contributor Author

dbast commented Apr 15, 2024

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

@jdlcdl
Copy link

jdlcdl commented Apr 15, 2024

Yes @dbast,

I believe that you are correct.

My recollection of trying to remove numpy from requirements was from
#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:

  • on debian w/o numpy in requirements.txt
  • on rpi0 manual-installation at release0.7.0 w/ numpy in requirements-raspi.txt (both for old 1.21 and new 1.25 numpy)
    ...and the app starts up, video working, this works for me.

@dbast
Copy link
Contributor Author

dbast commented Apr 15, 2024

Moved numpy to requirements-raspi.txt :)

@newtonick
Copy link
Collaborator

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

@newtonick
Copy link
Collaborator

Tested

@newtonick newtonick merged commit 9aa95e8 into SeedSigner:dev May 1, 2024
1 check passed
@dbast dbast deleted the six branch May 4, 2024 08:23
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