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

Fixes for native fs-events #716

Merged
merged 8 commits into from Dec 9, 2020

Conversation

CCP-Aporia
Copy link
Contributor

@CCP-Aporia CCP-Aporia commented Dec 7, 2020

Some fixes and code simplifications for the native fsevents module:

  1. PyCapsule is supported in 2.7, and so is the PyBytes_ C API, so we can remove a bunch of the #if PY_MAJOR_VERSION conditionals.
  2. Ensure on the C side that a valid UTF-8 encoded path is passed. This was one source of error memory corruption on my machine.
  3. Exposes missing attributes on NativeEvent (and provides additional ones that weren't exposed so far). This fixes [macOS] Events without "renamed" or "modified" flags are dropped #702 .

Alas, the Python 2.7 tests currently fail with an interesting error in tests/test_fsevents.py::test_unschedule_removed_folder that I still need to investigate. So this draft PR is primarily to see what the CI says for everything that I cannot test locally.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Dec 7, 2020

Thank you for the fix 🍾

FTR I disabled tests for Python 3.6-3.10 as they will be done on master.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Dec 7, 2020

Oups, I forgot to tell that the clean-up is already active on the branch. Could you rebase?

@CCP-Aporia
Copy link
Contributor Author

Sure, no problem.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Dec 8, 2020

I am OK to merge!

@CCP-Aporia
Copy link
Contributor Author

I'd first like to figure out the reason for the broken test. It appears to be a regression, and is pretty weird overall.

Python 2.7 didn't like the double string conversion.

Also improve some of the error handling code to behave better, as
well as add error handling to th PyCapsule creation.
@CCP-Aporia CCP-Aporia marked this pull request as ready for review December 9, 2020 13:52
CCP-Aporia added a commit to CCP-Aporia/watchdog that referenced this pull request Dec 9, 2020
@CCP-Aporia
Copy link
Contributor Author

Should be OK to merge now, travis is just slow to get around to all environments.

@BoboTiG BoboTiG merged commit 605d607 into gorakhargosh:python-2.7 Dec 9, 2020
@BoboTiG
Copy link
Collaborator

BoboTiG commented Dec 9, 2020

You are a killer :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants