Thanks for working on this.

Should #263 be closed?

Should #263 be closed?

That is what I would do.

While I have your attention, any thoughts on my question:

Is there a preference for keeping the changes all as a single commit, or should some of these changes be split into separate commits?

Thanks!

Is there a preference for keeping the changes all as a single commit, or should some of these changes be split into separate commits?

Fewer commits (or squash merge) is nice, as you've done here.

@nshalman Is there someone with FEN/Solaris experience that can provide your code review and approval?

@nshalman It may be better to build this out as a separate FEN project -- e.g. fsnotify/fen

I can't see myself maintaining or even doing code reviews on this -- I have enough trouble keeping up with the existing platforms.

What do you think?

@nshalman It may be better to build this out as a separate FEN project -- e.g. fsnotify/fen

I can't see myself maintaining or even doing code reviews on this -- I have enough trouble keeping up with the existing platforms.

What do you think?

Hey, I haven't had free time to work on this code myself. I've still seen odd test failures when I stress test it and I'm not sure if it's bug(s) in my code, bugs in the tests, or what.

I have no objection moving the code somewhere else rather than landing it into the tree as is, but my question is how that would enable other software to consume it. Ideally existing software that uses fsnotify that currently doesn't work on Solaris and illumos would be able to start working when a new release of fsnotify comes out that uses it.

Do you mean that I could keep the code in a separate package that fsnotify could pull in?

Thank you for keeping this conversation alive. I'm happy to work toward whatever makes the most sense when I finally have time to pick it up again.

I think there is the potential to eventually make fsnotify utilize an external FEN library.

In the interim, people who really want Solaris support could wrap fsnotify + fen libraries in some way. For example, Hugo uses both fsnotify + polling (I admit I haven't looked too closely at how they are doing it).
https://github.com/gohugoio/hugo/tree/master/watcher/filenotify

The idea of making fsnotify a very light wrapper around platform-specific libraries is something I've been thinking about for a long time. A large part of that is to have small teams that own each platform-specific library and can work freely on them.

The cross-platform aspect makes it really tricky for contributors, so I'd like to find a way to alleviate those troubles if possible.

The cross-platform aspect makes it really tricky for contributors, so I'd like to find a way to alleviate those troubles if possible.

At it's heart, fsnotify is a cross-platform abstraction layer, so it feels like that's what contributors are signing up for when they contribute. Indeed, people have been trying to add support for illumos to this library for years now:

The idea of making fsnotify a very light wrapper around platform-specific libraries is something I've been thinking about for a long time. A large part of that is to have small teams that own each platform-specific library and can work freely on them.

That sounds great, but it's not the status quo today. I think it would be unfortunate to put something that might never happen on the critical path to getting this change integrated so people can make use of it.

In the interim, people who really want Solaris support could wrap fsnotify + fen libraries in some way. For example, Hugo uses both fsnotify + polling (I admit I haven't looked too closely at how they are doing it). https://github.com/gohugoio/hugo/tree/master/watcher/filenotify

This would create a lot of extra technical and social work: rather than amend the cross-platform abstraction library, you are asking just one platform to go and reimplement that abstraction in any number of other bodies of consuming software. It seems like fsnotify is popular; that could be tens or hundreds of other bodies of software, each requiring rework and negotiation with maintainers.

I agree that we, and you, are all already resource constrained enough, which is why people have been trying to do the work just once, in the right place: here.

It isn't clear how much or what sort of additional code review is necessary before a merge. Can you outline your concerns, and what you feel remains outstanding? People in the community can likely assist with maintenance and debugging, say¸ as issues arise in future -- but it really needs to get merged so that people are actually using it in downstream software. Right now it's just sitting in a branch.

Updated it in the main branch; didn't merge it here.

So if I understand things correctly people will need a fairly recent version of illumos or it will panic? Not sure which versions of illumos distributions contain this change yet (if any?) Need to update the CI and my local test environment, as well as communicate this clearly to users.

Right now I can find the git hash abb88ab1b9516b1ca12094db7f2cfb5d91e0a135 in omnios bloody, OpenIndiana hipster (upstream illumos-gate), and the opensolaris porting project. I can't find it in illumos-joyent but there it might have a different hash as last gate merge was 5 hours ago, so unless it was specifically skipped it's in all upstrem tracking distros. In whcih release it made it already would be a question for @danmcd and @citrus-it as I do not know how to track it beyond scrubbing for it in github.

It's in illumos-omnios:master, which will eventually land in a bloody build:

nowhere(~/ws/illumos-omnios)[0]% git branch
* master
  nfs-zone
  r151030
  r151032
  r151034
  r151036
  r151038
  r151042
nowhere(~/ws/illumos-omnios)[0]% git log -1 abb88ab1b9516b1ca12094db7f2cfb5d91e0a135
commit abb88ab1b9516b1ca12094db7f2cfb5d91e0a135
Author: Robert Mustacchi <rm@fingolfin.org>
Date:   Sun Aug 7 20:20:53 2022 +0000

    14898 port_associate PORT_SOURCE_FILE doesn't update user obj properly
    Reviewed by: Nahum Shalman <nahamu+illumos@gmail.com>
    Reviewed by: Andy Fiddaman <illumos@fiddaman.net>
    Approved by: Richard Lowe <richlowe@richlowe.net>
nowhere(~/ws/illumos-omnios)[0]% git branch -a --contains abb88ab1b9516b1ca12094db7f2cfb5d91e0a135
* master
  remotes/origin/HEAD -> origin/master
  remotes/origin/master
nowhere(~/ws/illumos-omnios)[0]% 

And FWIW, you need a fairly recent SmartOS build for this fix:

kebe(~/ws/ij-cr)[0]% git branch -a --contains abb88ab1b9516b1ca12094db7f2cfb5d91e0a135
* master
  remotes/origin/HEAD -> origin/master
  remotes/origin/OS-8403
  remotes/origin/master
  remotes/origin/release-20220825
  remotes/origin/release-20220908
kebe(~/ws/ij-cr)[0]% 

So if I understand things correctly people will need a fairly recent version of illumos or it will panic? Not sure which versions of illumos distributions contain this change yet (if any?) Need to update the CI and my local test environment, as well as communicate this clearly to users.

It might panic the golang application (not the operating system.) For most uses cases it probably won't. It took a fair amount of stress testing for me to trip the bug.... If it does panic, the error message points explicitly at golang/go#54254 which will guide users to look into an OS update.

@papertigers did ping me on IRC about the need for an update for the CI OS image. Might take a little bit for that to be ready.

So if I understand things correctly people will need a fairly recent version of illumos or it will panic? Not sure which versions of illumos distributions contain this change yet (if any?) Need to update the CI and my local test environment, as well as communicate this clearly to users.

It might panic the golang application (not the operating system.) For most uses cases it probably won't. It took a fair amount of stress testing for me to trip the bug.... If it does panic, the error message points explicitly at golang/go#54254 which will guide users to look into an OS update.

papertigers did ping me on IRC about the need for an update for the CI OS image. Might take a little bit for that to be ready.

Ah right, I see your latest commit improved things a bit; I didn't look at the details before and just assumed it was 100% fixed on illumos side.

I think this is mostly good to merge then, right? We still have the issue of the CI not running here; I looked in that a few weeks ago, and it's not running because I changed:

on: ['push', 'pull_request']

to:

on:
  push:
    paths: ['**.go', 'go.mod', '.github/workflows/*']

It seems that it will only run "on" a push to this repo, but not to forks. Makes sense, but behaves different than I expected.

These "double" CI runs were causing me real grief because GitHub macOS runners are very limited, and we're spinning off 7 of them now for the various tests; I was waiting almost an hour at times for the CI to finish, so I'd rather not "just" add it back. Even with just push it's already pretty long waits at times.

What we need is something like:

on: ['push_from_anywhere_including_fork']

But I couldn't really figure out how to do that :-/

Maybe I should just change it back for now...

I think this is mostly good to merge then, right? We still have the issue of the CI not running here; I looked in that a few weeks ago, and it's not running because I changed:

The behavior on symlinks you noted worries me; I'd prefer to get it fixed before landing if possible.

The TestWatch/watch_a_symlink_to_a_file I added in #498 (and merged here too) seems to generate a lot of Write events:

--- FAIL: TestWatch/watch_a_symlink_to_a_file (0.76s)
    fsnotify_test.go:214: 
        have:
        	WRITE                "/link"
        	WRITE                "/link"
        	[.. 16578 more elided ..]
        	WRITE                "/link"
        	WRITE                "/link"
        want:
        	WRITE                "/link"

The watch_a_symlink_to_a_dir test seems fine.

The symlink behavior quirks also remind me about #387 and all the other levels of confusion that symlinks can cause relative to fsnotify semantics...

We might want to test and document our current behavior on symlinks on Linux in a bunch of different situations and make some improved test cases around them to give me more to work with while hunting it down.

I was able to trivially make the tests pass by incorrectly changing the call to reassociate a path that had fired using lstat info, but I'm pretty sure that would fail a more robust test case of multiple write events needing to properly fire.

I'm hoping to poke around today to see if I can figure out the correct fix and will update accordingly.

When I remove the calls to skip the symlink tests, sometimes I see this:

warning: GOPATH set to GOROOT (/home/nshalman/go) has no effect
--- FAIL: TestWatch (0.00s)
    --- FAIL: TestWatch/watch_a_symlink_to_a_dir (1.65s)
        helpers_test.go:384: this EventPort is already closed
        helpers_test.go:354: event stream was not closed after 1s
    --- FAIL: TestWatch/watch_a_symlink_to_a_file (1.70s)
        helpers_test.go:384: this EventPort is already closed
        helpers_test.go:354: event stream was not closed after 1s

Okay, I now understand the symlink issue (passing unix.FILE_NOFOLLOW as part of the events and a symlink path, but the FileInfo from Stat rather than Lstat seems to basically cause the kernel to fire an event immediately and it's not wrong...)

I have a fix in place, the test skips are removed, and the tests pass!

I have a reproducible test failure on illumos with go 1.19 when run as root:

--- FAIL: TestAdd (0.00s)
    --- FAIL: TestAdd/permission_denied (0.20s)
        fsnotify_test.go:889: error is nil

The test passes cleanly when run as a non-root user.
I'm guessing that on illumos the root user isn't stopped by mere permissions in that test case. 🤔

The behavior on symlinks you noted worries me; I'd prefer to get it fixed before landing if possible.

Ah yeah, I forgot about that. So many comments and hard to keep track of stuff.

I'll finish up the last few things today or tomorrow and do a new release; as far as I'm concerned it's okay to merge it after that. It doesn't need to be 100% perfect and we can create new issues and improve things before the next release. Personally I'd say that's a tad easier to keep track of things, but whatever works for you is fine with me so it's your call.

I'll finish up the last few things today or tomorrow and do a new release; as far as I'm concerned it's okay to merge it after that. It doesn't need to be 100% perfect and we can create new issues and improve things before the next release. Personally I'd say that's a tad easier to keep track of things, but whatever works for you is fine with me so it's your call.

I agree that this is in a good enough state to merge.

@arp242 I imagine you're pretty busy, but it's been a few weeks so I figured I'd check in. Is there anything I can do to help get the next release cut so that this can be merged after that? I did fix the symlink issue, so I'm feeling very good about this code. Thanks!!

Yeah, sorry; new job and some other issues I had to deal with 😅

I released v1.6.0 so we have a bit of time to shake out some potential issues before a new release.