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

With 2.7 dropped, use shutil disk_usage function universally. #12041

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

rtibbles
Copy link
Member

Summary

Removes platform specific behaviour.
Uses shutil disk_usage which has support on Unix systems and Windows.
This may well break Android, as it removes the Android specific code, but this will be rectified in the Android installer with this issue: learningequality/kolibri-installer-android#211

References

Fixes the Kolibri side of learningequality/kolibri-installer-android#211

Reviewer guidance

Windows and Unix systems should show free space appropriately for content imports.

Opening as a draft to first confirm the impact on the Android installer.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Mar 30, 2024
@pcenov
Copy link
Member

pcenov commented Apr 11, 2024

Hi @rtibbles - the app force closes immediately after launch on all of my devices. Here are the logs, db and logcat from Android studio:

logs-logcat.zip

@rtibbles
Copy link
Member Author

I guess that takes the uncertainty out of this!

This may well break Android

@rtibbles
Copy link
Member Author

Looking at the logs, this is the same error you saw on my PR: #11974 - and doesn't seem to be directly related to this PR at all.

@rtibbles
Copy link
Member Author

I will come back to this PR when the other one has been merged.

Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

loving when a pr mostly removes code. It's pretty clear (as far as we're using at least python 3.3, which will be the case in develop)

@radinamatic
Copy link
Member

This might need a rebase, since #11974 has been merged... 🤔
(tested the APK today, the initial app force close after launch is still extant)

@rtibbles
Copy link
Member Author

Have rebased on latest develop, so hopefully any errors should be purely its own!

@radinamatic
Copy link
Member

Comment Screenshhot
This latest APK installs and goes through the setup wizard without issues 🙂 Any specific reviewer guidance for this PR? 2024_05_24_23 08 39

@rtibbles
Copy link
Member Author

The only possible source of breakage would be the free space being misreported during resource import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants