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

Do not force absolute Paths #49

Open
philjb opened this issue Jul 16, 2021 · 5 comments · May be fixed by #58
Open

Do not force absolute Paths #49

philjb opened this issue Jul 16, 2021 · 5 comments · May be fixed by #58

Comments

@philjb
Copy link

philjb commented Jul 16, 2021

fsevents @ 1055680

Which version of macOS are you using?

ProductName:    Mac OS X
ProductVersion: 10.15.7
BuildVersion:   19H1217

Please describe the issue that occurred.

FSEvents api supports two different ways of starting an event stream:

  1. Uses a device id and function call FSEventStreamCreateRelativeToDevice which is expecting paths to be relative to the mount point of the device. This is called the per-disk event stream in the docs. The example/main.go uses this approach since it uses a non-zero device id. See the documentation here specifically about param pathsToWatchRelativeToDevice
pathsToWatchRelativeToDevice
A CFArray of CFStringRefs, each specifying a relative path to a directory on the device identified by the dev parameter. The paths should be relative to the root of the device. For example, if a volume "MyData" is mounted at "/Volumes/MyData" and you want to watch "/Volumes/MyData/Pictures/July", specify a path string of "Pictures/July". To watch the root of a volume pass a path of "" (the empty string).
  1. The alternative is with function FSEventStreamCreate which is expecting absolute paths. This is the "per-host" event stream constructor. fsevents uses this approach if the Device in the EventStream struct is zero. While the docs are ambiguous, on my system, the "per-host" approach only works with absolute paths.
pathsToWatch
A CFArray of CFStringRefs, each specifying a path to a directory, signifying the root of a filesystem hierarchy to be watched for modifications.

However, wrap.go/createPaths() transforms all paths in an EventStream struct to absolute paths at

fsevents/wrap.go

Lines 154 to 158 in f721bd2

func createPaths(paths []string) (C.CFArrayRef, error) {
cPaths := C.ArrayCreateMutable(C.int(len(paths)))
var errs []error
for _, path := range paths {
p, err := filepath.Abs(path)

This prevents using the "per-disk" mode for all volumes except the root volume at /.

Are you able to reproduce the issue? Please provide steps to reproduce and a code sample if possible.

Yes.

  1. Create a new Volume on OSX (I use APFS) mounted at /Volumes/fsevents-repo
  2. Use the example/main.go program with path /Volumes/fsevents-repo (the only change)
  3. go run ./example/main.go
  4. touch /Volumes/fsevents-repo/testfile and observe there is no event reports.
  5. Now, change wrap.go/createPaths to using
	for _, path := range paths {
		p := path
		cpath := C.CString(p)

and repeat the steps above.

PR coming shortly.

@nathany
Copy link
Contributor

nathany commented Jul 31, 2021

Thanks for reporting the issue. A PR would be appreciated.

@marklr
Copy link

marklr commented Jan 22, 2022

I have a tentative PR that works with the provided test case, by stripping the mount points from the paths before adding them to the monitoring list: #58

@pbnjay
Copy link
Contributor

pbnjay commented Jan 22, 2022

So the problem identified here is that EventStream is mixing FSEventStreamCreate and FSEventStreamCreateRelativeToDevice which each have different input requirements, and each provide different sources for event IDs under the hood. The selection of which stream type is done magically depending on the value of EventStream.Device

For the typical users of this package, is there any reason to prefer FSEventStreamCreateRelativeToDevice and add all the complexity of PR #58 ?
Would it make more sense to split the advanced functionality into a DeviceEventStream interface that makes the behavior explicit? This would still allow for persistent notifications but wouldn't introduce more surface area for bugs in the most common use-case.

If the goal of this package is future adoption in fsnotify/fsnotify then simplifying the primary codepath would be beneficial, right?

@marklr
Copy link

marklr commented Jan 22, 2022 via email

@nathany
Copy link
Contributor

nathany commented Jan 24, 2022

Simple is good.

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 a pull request may close this issue.

4 participants