Add support for solaris FEN #196

gfrey · 2017-02-05T16:24:39Z

Fixes #12
I've signed the CLA.

What does this pull request do?

This is a basic implementation of fsnotify for Solaris FEN. It at least passes all integration tests.

Where should the reviewer start?

I used the following two resource to get a basic understanding how this should work:

How should this be manually tested?

The available integration tests are enabled for solaris now and succeed.

This is a basic implementation of fsnotify for Solaris FEN. It at least passes all integration tests.

houndci-bot · 2017-02-05T16:24:46Z

fen.go

+	var finfo C.struct_file_info
+	finfo.mode = C.uint(stat.Mode())
+
+	var mode C.int = C.FILE_MODIFIED | C.FILE_ATTRIB | C.FILE_NOFOLLOW


should omit type C.int from declaration of var mode; it will be inferred from the right-hand side

houndci-bot · 2017-02-05T16:24:46Z

fen.go

+	}
+}
+
+func (w *Watcher) Remove(path string) error {


exported method Watcher.Remove should have comment or be unexported

houndci-bot · 2017-02-05T16:24:46Z

fen.go

-// Add starts watching the named file or directory (non-recursively).
-func (w *Watcher) Add(name string) error {
-	return nil
+func (w *Watcher) Add(path string) error {


exported method Watcher.Add should have comment or be unexported

gfrey · 2017-02-05T21:01:33Z

fen.go

@@ -231,7 +231,7 @@ func (w *Watcher) associateFile(path string, stat os.FileInfo) error {
 	var finfo C.struct_file_info
 	finfo.mode = C.uint(stat.Mode())

-	var mode C.int = C.FILE_MODIFIED | C.FILE_ATTRIB | C.FILE_NOFOLLOW
+	mode := C.FILE_MODIFIED | C.FILE_ATTRIB | C.FILE_NOFOLLOW


Well type inference doesn't seem to work as houndci-bot expected. Now I need to cast the usage of mode. Not sure I like that better. Will have a closer look later.

While the `var mode C.int = ...` code isn't the nicest, at least it was working. Type inference alone didn't work. Hopefully houndci will accept this solution with the explicit cast.

nathany · 2017-02-06T23:22:02Z

@4ad @amitkris Please review.

nathany · 2017-02-06T23:24:04Z

@gfrey We don't currently have a good way to test Solaris, either with a CI service or locally from a different OS (I use Vagrant Gopher for Linux/BSD). Any suggestions?

gfrey · 2017-02-07T06:22:38Z

@nathany I used VirtualBox with an OpenIndiana hipster install. I'll try and add support for solaris to your Vagrant Gopher, just give me a few days.

nathany · 2017-02-18T04:42:47Z

@gfrey Thanks both for this pull request and your work on Vagrant Gopher to test it.

❯ vagrant ssh solaris -c 'go version; cd fsnotify/fsnotify; go test ./...'
go version go1.8 solaris/amd64
/export/home/vagrant/src/github.com/fsnotify/fsnotify
ok      github.com/fsnotify/fsnotify    8.332s
Connection to 127.0.0.1 closed.

(unfortunately no race detector on Solaris)

I'm still hoping @4ad and/or @amitkris provide a review, but I will also take a closer look in the coming days.

fazalmajid · 2017-05-18T20:39:04Z

What can we do to get this PR accepted and merged into the master branch?

This is blocking getting https://github.com/spf13/hugo and https://github.com/prometheus/alertmanager (via https://github.com/stuartnelson3/guac) running on Solaris.

I confirmed that this branch merged into master works on SmartOS inside Hugo (running hugo --watch) whereas without it it just exits with the message FEN based watcher not yet supported for fsnotify. Even if gfrey's code has bugs, it can't be any more broken on Solaris than the stub in the master branch. Furthermore, fen.go is marked as +build solaris so there is zero risk of regression on other platforms.

bep · 2017-05-22T14:52:38Z

@4ad @amitkris @nathany This would be really good for Hugo to get in. @fazalmajid has a valid point about the this cannot break it worse than it already is and the breakage is limited to Solaris. I have quickly browsed through the code and cannot find any obvious mistakes.

tiwaana · 2017-06-09T16:55:24Z

We in DTC golang meetup reviewed the code, not all it. But we have few questions
a) is using C required
b) In close() things are not mutex protected.
its only limited to Solaris, so should not break other things.
Anand

binarycrusader · 2017-06-09T17:18:38Z

I'm not the author, but Solaris requires use of cgo, in general, since libc is the provider of system interfaces. The only other comment I would add is that this should be using golang.org/x/sys/unix instead of syscall. Per the official Go documentation, use of syscall outside of the Go standard runtime has been deprecated since 1.4:

NOTE: This package is locked down. Code outside the standard Go repository should be migrated to use the corresponding package in the golang.org/x/sys repository. That is also where updates required by new systems or versions should be applied. See https://golang.org/s/go1.4-syscall for more information.
https://golang.org/pkg/syscall/

As far as I can tell, this should work on commercial releases of Solaris, although I haven't tried it myself. It appears to be using functionality shared between Solaris and OpenSolaris-based derivatives such as Illumos/SmartOS.

4ad · 2017-06-09T17:23:07Z

I don't care about fsnotify, so you'll have to find someone else to review this, but I will point out that using cgo here is an unnecessary mistake. This should all be done using golang.org/x/sys/unix (probably after you update it).

quenelle · 2017-06-15T05:35:22Z

Forgive any mistakes here, I'm very new to Go. For this kind of code there seems to be a good justification for using the C pass through interfaces. Probably lots of things don't have native Go interfaces yet. Using "syscall" is another matter. It's explicitly deprecated. I see two places where syscall is used explicitly in the code. 1) It's used to access the enumeration of errno values. When I visit x/sys/unix, it says that the official place to get errno values from is syscall.Errno. (Check the top of https://godoc.org/golang.org/x/sys/unix) So that seems legitimate. 2) The other place it's used is to access the C structure for stat_t. The code starts by using os.FileInfo, then goes through Sys() to get the stat_t. It does this because the FileInfo doesn't include atime or ctime members. (access time / create time). Only mtime (modification time) is available through FileInfo according to the docs. So I don't see any way the code could be changed to avoid the existing uses of syscall.

In theory you could argue that atime and ctime should be added FileInfo, but that seems like a change that should take into account the availability of those interfaces on other platforms. I'm not sure how widely they're supported on all the Go platforms. It doesn't seem appropriate to ask the submitter to take that on at this point.

So with that in mind, I don't see any issues with the use of cgo or syscall.

I also ran the fsnotify package test on x86 Oracle Solaris (latest internal build) with the bundled Go 1.5, and it passed. I also ran it on the same Solaris build on SPARC with the latest sparc compiler from https://github.com/4ad/go/tree/sparc64.go1.8 and it passed.

I'm not that familiar with file events in Solaris, but if additional code review is what's holding this up, I can find an Oracle Solaris engineer who is familiar with file events to check over the logic. They won't be familiar with Go, but they could vouch for the usage of Solaris interfaces.

maxmeyer · 2017-08-18T07:46:20Z

@dago Are you still into OpenCSW? Can their build farm help to solve the CI "problem" in this issue?

isaacdavis · 2018-08-28T18:27:52Z

Hi @gfrey! I work at Joyent. We maintain SmartOS, and we need fsnotify support.

Thank you for doing this work! I took a look at your pull request and made some changes. I added more robust support for cancellation to fix a race condition exhibited by the TestWatcherClose test, and I altered the interpretation of FEN events to more accurately implement fsnotify's expected behavior, as well as other small improvements.

I intend to submit a new pull request for this work. Given that my request would still substantially contain your code, how would you like to be credited? I could add you to the AUTHORS file or anything else you might prefer. Thanks!

To everyone else on this thread - I would be happy to provide additional resources for testing fsnotify on SmartOS. Please let me know!

gfrey · 2018-08-28T18:39:13Z

@isaacdavis I'm fine with being mentioned in the AUTHORS file. Nice seeing this getting some traction again!

dago · 2018-08-28T19:23:16Z

@maxmeyer Sure. I have briefly reviewed the comments in this PR, is there something specific I should test?

maxmeyer · 2018-08-28T19:27:25Z

@dago What about setting about an automatic build on the opencsw build farm for this project?

dago · 2018-08-28T19:33:40Z

@maxmeyer That would be doable, we already have a number of projects in a buildbot setup which is triggered by hooks, most notable curl which has full CI integration in GitHub with different builders (click on the green checkmarks for CI build details). However, I need to update some stuff to recent Go compilers and make it compile manually first. Please PM me for details.

maxmeyer · 2018-08-28T19:50:38Z

@isaacdavis can you help @dago with details to make it build automatically for csw?

isaacdavis · 2018-08-28T23:23:26Z

@maxmeyer - Unfortunately, I'm unfamiliar with csw, and my scope of knowledge is centered on illumos/SmartOS rather than Oracle Solaris. With that said, I had no issues building fsnotify on SmartOS, so hopefully the csw build should be equally straightforward. @dago - if you run into any issues please let me know and I will try to help.

Note that I now have a new pull request open (#263) with an updated version of this code that fixes some issues. You may want to have a look!

nathany · 2018-08-30T21:48:13Z

Closed in favour of #263.

nathany · 2021-08-18T22:53:18Z

See #371

Add support for solaris FEN

34a9f2e

This is a basic implementation of fsnotify for Solaris FEN. It at least passes all integration tests.

houndci-bot reviewed Feb 5, 2017

View reviewed changes

fen.go Outdated

}

}

func (w *Watcher) Remove(path string) error {

Copy link

houndci-bot Feb 5, 2017

Choose a reason for hiding this comment

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

exported method Watcher.Remove should have comment or be unexported

houndci-bot reviewed Feb 5, 2017

View reviewed changes

Gereon Frey added 2 commits February 5, 2017 16:33

Add comments to exported functions

94bae70

Fix houndci-bot finding: use type inference

312ce4b

gfrey commented Feb 5, 2017

View reviewed changes

Fix houndci wrong finding

f72044f

While the `var mode C.int = ...` code isn't the nicest, at least it was working. Type inference alone didn't work. Hopefully houndci will accept this solution with the explicit cast.

nathany added help-wanted illumos labels Feb 6, 2017

siepkes mentioned this pull request Mar 10, 2017

Added SmartOS / Solaris support. mattermost/mattermost#5718

Closed

7 tasks

nathany mentioned this pull request Mar 14, 2017

Maintainers wanted #183

Closed

bep mentioned this pull request May 22, 2017

Add Solaris build gohugoio/hugo#3500

Closed

isaacdavis mentioned this pull request Aug 28, 2018

add Solaris FEN support #263

Closed

nathany closed this Aug 30, 2018

fazalmajid mentioned this pull request Sep 12, 2019

Azure support breaks Solaris compatibility gohugoio/hugo#6324

Closed

nshalman mentioned this pull request Jul 8, 2021

Add Solaris FEN using EventPorts from x/sys/unix #371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for solaris FEN #196

Add support for solaris FEN #196

gfrey commented Feb 5, 2017

houndci-bot Feb 5, 2017

houndci-bot Feb 5, 2017

houndci-bot Feb 5, 2017

gfrey Feb 5, 2017

nathany commented Feb 6, 2017

nathany commented Feb 6, 2017

gfrey commented Feb 7, 2017

nathany commented Feb 18, 2017

fazalmajid commented May 18, 2017

bep commented May 22, 2017

tiwaana commented Jun 9, 2017

binarycrusader commented Jun 9, 2017

4ad commented Jun 9, 2017 •

edited

quenelle commented Jun 15, 2017

maxmeyer commented Aug 18, 2017

isaacdavis commented Aug 28, 2018

gfrey commented Aug 28, 2018

dago commented Aug 28, 2018

maxmeyer commented Aug 28, 2018

dago commented Aug 28, 2018 •

edited

maxmeyer commented Aug 28, 2018

isaacdavis commented Aug 28, 2018

nathany commented Aug 30, 2018

nathany commented Aug 18, 2021

Add support for solaris FEN #196

Add support for solaris FEN #196

Conversation

gfrey commented Feb 5, 2017

What does this pull request do?

Where should the reviewer start?

How should this be manually tested?

houndci-bot Feb 5, 2017

Choose a reason for hiding this comment

houndci-bot Feb 5, 2017

Choose a reason for hiding this comment

houndci-bot Feb 5, 2017

Choose a reason for hiding this comment

gfrey Feb 5, 2017

Choose a reason for hiding this comment

nathany commented Feb 6, 2017

nathany commented Feb 6, 2017

gfrey commented Feb 7, 2017

nathany commented Feb 18, 2017

fazalmajid commented May 18, 2017

bep commented May 22, 2017

tiwaana commented Jun 9, 2017

binarycrusader commented Jun 9, 2017

4ad commented Jun 9, 2017 • edited

quenelle commented Jun 15, 2017

maxmeyer commented Aug 18, 2017

isaacdavis commented Aug 28, 2018

gfrey commented Aug 28, 2018

dago commented Aug 28, 2018

maxmeyer commented Aug 28, 2018

dago commented Aug 28, 2018 • edited

maxmeyer commented Aug 28, 2018

isaacdavis commented Aug 28, 2018

nathany commented Aug 30, 2018

nathany commented Aug 18, 2021

4ad commented Jun 9, 2017 •

edited

dago commented Aug 28, 2018 •

edited