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

Port to non-MacOS platforms #1026

Merged
merged 6 commits into from
May 7, 2023
Merged

Port to non-MacOS platforms #1026

merged 6 commits into from
May 7, 2023

Conversation

dvdkon
Copy link
Contributor

@dvdkon dvdkon commented Apr 1, 2023

This PR allows osxphotos to run on non-MacOS platforms (tested on NixOS Linux), both as a library and a CLI tool. This is accomplished mainly by gating specific functionality behind OS checks and providing alternative codepaths where necessary.

This mostly doesn't bring any complexity to the codebase, except for the need to gate new MacOS specific code and the need to deal with Unicode normalisation. MacOS does it in the background, other OSs can theoretically deal with any form of Unicode, but it seems Photos databases aren't internally consistent, so the best solution is to normalise on output.

Two tests are failing, tests/test_cli.py::test_export_touch_files and test_export_touch_files_update. I don't understand why only 31 files should be touched, so I can't fix the test. Other tests are either skipped or run fine.

A port to Linux will allow running tests on GitHub's CI platform, but that's not part of this PR.

@RhetTbull
Copy link
Owner

Wow -- thank you. This looks like a lot of work. As this is a big PR it will take me some time to review.

osxphotos/cli/__init__.py Outdated Show resolved Hide resolved
@RhetTbull
Copy link
Owner

@all-contributors please add @dvdkon for code

@allcontributors
Copy link
Contributor

@RhetTbull

I've put up a pull request to add @dvdkon! 🎉

"Frítest (3).jpg",
"Frítest_edited (1).jpeg",
"Frítest_edited.jpeg",
"Frítest.jpg",
Copy link
Owner

Choose a reason for hiding this comment

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

This causes most export tests to fail on macOS. The unicode for macOS must be NFD or anyone who's previously exported unicode filenames with osxphotos will have their exports corrupted (duplicate exports, invalid updates, etc). These file names are chosen because they came from users who had issues with exporting in unicode. For tests to pass on both macOS and linux you'll need separate versions of the test filenames. Can linux worth with NFD filenames? If so, perhaps it would be better to leave everything as NFD on export.

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @dvdkon I would love to get this merged but need to get the unicode names in tests fixed. If you're able to add logic to the tests to use the right unicode names depending on platform I can get this merged. Perhaps it might be possible to transform the names from NFC/NFD when importing the test module so you don't need two separate sets of names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sorry for not getting to this sooner. It would be best to leave Unicode issues to the user and continue as on macOS, but when running on Linux without the changes I've made, many tests fail, since somehow osxphotos is working with both NFC and NFD versions of the same strings. My guess is the Photos DB is not properly normalised, since it doesn't matter on HFS+.

I think your idea of re-normalising the filenames on import based on the platform is the best approach forward. It's going to be strict in requiring that it uses NFD on macOS and NFC elsewhere, but that might be for the better. I'll rebase on master and make the changes soon.

@@ -0,0 +1,15 @@
""" Helpers for running locale-dependent tests """

import locale
Copy link
Contributor

Choose a reason for hiding this comment

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

placate the linter with https://docs.python.org/3/library/contextlib.html

Suggested change
import locale
import contextlib
import locale

@dvdkon
Copy link
Contributor Author

dvdkon commented Apr 28, 2023

I got hold of a Mac and can confirm all tests now pass on macOS.

There's still a problem on Linux with two tests checking touched date counts. Apart from that, everything seems OK.

@RhetTbull
Copy link
Owner

Thanks for the update! I'll run the subset of tests that only run on my machine or require user interaction (timewarp, etc.) this weekend and try to get this merged.

@RhetTbull RhetTbull merged commit ca3da64 into RhetTbull:main May 7, 2023
4 checks passed
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.

None yet

3 participants