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 python scripts/dependencies from iOS directory #6009

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

faern
Copy link
Member

@faern faern commented Mar 22, 2024

So, if I run the vulnerability scanner osv-scanner on our repository. It points out that ios/requirements.txt has a vulnerable version of gdal 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 is ios/update_translations.py, and this script only depends on nslocalized. nslocalized in turn only depend on six. So the remaining entries in requirements.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 for update_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?

python3 -m venv ./python_env
source python_env/bin/activate
pip3 install -r requirements.txt

python3 update_translations.py

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 Reviewable

@faern faern added the iOS Issues related to iOS label Mar 22, 2024
@faern faern requested a review from buggmagnet March 22, 2024 10:47
Copy link
Member Author

@faern faern left a 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.

@faern faern force-pushed the clean-up-ios-python-deps branch from df05999 to 037aa8e Compare March 22, 2024 11:50
@faern
Copy link
Member Author

faern commented Mar 22, 2024

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.

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@faern faern changed the title Clean up iOS python deps in ios/requirements.txt Remove python scripts/dependencies from iOS directory Mar 22, 2024
@faern faern force-pushed the clean-up-ios-python-deps branch from 037aa8e to dfd4dbe Compare March 22, 2024 12:21
Copy link
Contributor

@buggmagnet buggmagnet left a 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: :shipit: complete! all files reviewed, all discussions resolved

faern added 3 commits March 26, 2024 10:05
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?
@faern faern force-pushed the clean-up-ios-python-deps branch from dfd4dbe to bf00282 Compare March 26, 2024 09:05
@faern faern merged commit cab1180 into main Mar 26, 2024
4 checks passed
@faern faern deleted the clean-up-ios-python-deps branch March 26, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants