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

Fix fsevents segfault #763

Merged
merged 9 commits into from
Feb 17, 2021
Merged

Conversation

samschott
Copy link
Contributor

@samschott samschott commented Feb 16, 2021

Fixes #762 and adds a test case.

Edit: Apologies for the oversight. I did warn you that this was my first time programming in C...

Copy link
Contributor

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

Comment on lines 301 to 304
const char *c_string_ptr;
PyObject *py_string;

c_string_ptr = CFStringGetCStringPtr(cf_string_ref, 0);
c_string_ptr = CFStringGetCStringPtr(cf_string_ref, kCFStringEncodingUTF8);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can declare and initialize c_string_ptr in the same line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point :)

@samschott samschott force-pushed the fix-fsevents-segfault branch from 930bf3a to c993fa4 Compare February 16, 2021 20:30
@samschott
Copy link
Contributor Author

samschott commented Feb 16, 2021

The tests on macOS were all failing but only on Travis. The new test was creating a directory named "TéstClass" with the e-acute represented by U+00E9. On Travis, the resulting native event had a path with name "TéstClass" where the e-acute is U+0065 and U+0301.

It appears that the path as reported by FSEvents is correct, the actual file which is created is using the second, decomposed representation of e-acute, contrary to what is passed to os.mkdir. It looks like the CI is running macOS 10.13 with HFS+ which, according to Apple's docs here, "converts all file names to decomposed Unicode". This is unlike APFS which doesn't perform such normalisation and where the test passes.

A good writeup of the changes: https://eclecticlight.co/2017/04/06/apfs-is-currently-unusable-with-most-non-english-languages/

I have therefore changed the file name to use the decomposed characters in the first place. This explains the last commit with changes that might otherwise appear to be just "cosmetic". I can confirm that the new file name still tests the same code path and that the test fails on the master branch.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 17, 2021

Thank you for the fix and for the explanation about unicode stuff :)

No worries about your "C level", you are making the module so much better that it does not count :)
And I will release 2.0.1 with the fix, so the issue is quickly fixed and available for everyone.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 17, 2021

Release v2.0.1 online.

@samschott
Copy link
Contributor Author

Nice!

@samschott samschott deleted the fix-fsevents-segfault branch February 17, 2021 15:58
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.

SEGFAULT on macOS with the latest version of watchdog
3 participants