-
Notifications
You must be signed in to change notification settings - Fork 381
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 python scripts/dependencies from iOS directory #6009
Conversation
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.
Reviewable status: 0 of 2 files reviewed, all discussions resolved
.github/workflows/verify-locked-down-signatures.yml
line 13 at r1 (raw file):
- ci/keys/** - ci/verify-locked-down-signatures.sh - ios/requirements.txt
All lockfiles, or all files that affect supply chain security, must be listed here. Changes to them are not allowed by third parties, and commits changing them must be signed.
df05999
to
037aa8e
Compare
I now changed this PR to remove the iOS python script altogether. I learned that we never use it, and have no concrete plans to use it in the short term. Whenever we implement translations, we should evaluate if we can use a different language than python to do this type of task, if it's still needed by then. |
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
ios/requirements.txt
037aa8e
to
dfd4dbe
Compare
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.
nice job !
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
We don't use this script, since we don't have translations yet. And when we do, it would be cool if we could use another language maybe. To keep the number of different languages a bit lower?
dfd4dbe
to
bf00282
Compare
So, if I run the vulnerability scanner
osv-scanner
on our repository. It points out thatios/requirements.txt
has a vulnerable version ofgdal
in it: https://osv.dev/vulnerability/PYSEC-2022-43065.Upon further investigation my conclusion is that
ios/requirements.txt
has a lot of stuff in it that we don't actually need. As far as I understand, our only iOS python code isios/update_translations.py
, and this script only depends onnslocalized
.nslocalized
in turn only depend onsix
. So the remaining entries inrequirements.txt
seem to be just noise.My theory on why they ended up there in the first place is that by default
pip freeze
that generate these files generate a list of all globally installed python packages. Or all packages installed in a venv if one is active. And the person generating this lockfile probably had a lot more packages installed than the ones purely needed forupdate_translations.py
.Can you please verify my assumptions by trying to use this script in whatever ways you usually do, but isolated to a venv with only the things from my updated requirements file are installed?
EDIT: Turns out we don't use this script at all. So the PR changed into just removing it altogether. Later, when we want to introduce translations, we can resurrect it if we feel like this solution is the best way to go. But we should also consider writing such tooling in other languages that we already use, to keep the tech stack a bit narrower.
This change is