Does not work in powerpc64 or arm64

[NightTsarina]

Hi,

I package prometheus for Debian. And I am getting build errors in ppc64el, which seem to originate in fsnotify. If I try to run fsnotify's tests there I get all kind of failures, and a hang:

github.com/go-fsnotify/fsnotify
   dh_auto_test -O--buildsystem=golang
    go test -v github.com/go-fsnotify/fsnotify
=== RUN   TestPollerWithBadFd
--- PASS: TestPollerWithBadFd (0.00s)
=== RUN   TestPollerWithData
--- FAIL: TestPollerWithData (0.00s)
    inotify_poller_test.go:85: expected poller to return true
=== RUN   TestPollerWithWakeup
--- PASS: TestPollerWithWakeup (0.00s)
=== RUN   TestPollerWithClose
--- FAIL: TestPollerWithClose (0.00s)
    inotify_poller_test.go:119: expected poller to return true
=== RUN   TestPollerWithWakeupAndData
--- FAIL: TestPollerWithWakeupAndData (0.00s)
    inotify_poller_test.go:140: expected poller to return true
=== RUN   TestPollerConcurrent
--- FAIL: TestPollerConcurrent (0.05s)
    inotify_poller_test.go:197: expected true
=== RUN   TestInotifyCloseRightAway
--- PASS: TestInotifyCloseRightAway (0.05s)
=== RUN   TestInotifyCloseSlightlyLater
--- PASS: TestInotifyCloseSlightlyLater (0.10s)
=== RUN   TestInotifyCloseSlightlyLaterWithWatch
--- PASS: TestInotifyCloseSlightlyLaterWithWatch (0.10s)
=== RUN   TestInotifyCloseAfterRead
--- PASS: TestInotifyCloseAfterRead (0.10s)
=== RUN   TestInotifyCloseCreate
--- FAIL: TestInotifyCloseCreate (0.05s)
    inotify_test.go:136: Took too long to wait for event
=== RUN   TestInotifyStress
--- FAIL: TestInotifyStress (5.00s)
    inotify_test.go:238: Expected at least 50 creates, got 0
=== RUN   TestInotifyRemoveTwice
--- PASS: TestInotifyRemoveTwice (0.00s)
=== RUN   TestInotifyInnerMapLength

signal: terminated
FAIL    github.com/go-fsnotify/fsnotify 348.362s

[NightTsarina]

This also happens in normal ppc64 (bigendian)

[NightTsarina]

In arm64, it just fails with "function not implemented":

=== RUN   TestPollerWithData
--- FAIL: TestPollerWithData (0.13s)
    inotify_poller_test.go:82: poller failed: function not implemented
=== RUN   TestPollerWithWakeup
--- FAIL: TestPollerWithWakeup (0.13s)
    inotify_poller_test.go:101: poller failed: function not implemented
=== RUN   TestPollerWithClose
--- FAIL: TestPollerWithClose (0.13s)
    inotify_poller_test.go:116: poller failed: function not implemented
=== RUN   TestPollerWithWakeupAndData
--- FAIL: TestPollerWithWakeupAndData (0.13s)
    inotify_poller_test.go:137: poller failed: function not implemented
=== RUN   TestPollerConcurrent
--- FAIL: TestPollerConcurrent (0.13s)
    inotify_poller_test.go:180: poller failed: function not implemented
    inotify_poller_test.go:197: expected true

[nathany]

Hi Martín. Thanks for reporting the issues. Unfortunately I don't have access to Linux on these architectures to test with nor do we have CI/builders on those architectures to catch the issues.

The poller code that is failing was submitted by @PieterD in #66 and switched to epoll_create1 by @suihkulokki in #100 in an effort to make it work on arm64. Maybe they will have some insight into why it's failing.

[suihkulokki]

Indeed looks like wrapping epoll_wait to epoll_pwait is needed for arm64. just adding epoll_create1 wasn't enough. I think in go users shouldn't mess with signals, we should only ever pass sigmask of Null to epoll_pwait, which is then identical to epoll_wait. I'll study a bit and propose fix(es).

[PieterD]

I also don't have those architectures, sorry.

[nathany]

thanks Riku

as to the errors on PowerPC, I really have no idea 😢

[NightTsarina]

Thanks to all for taking a look!

I have access to porter machines in these architectures, as a Debian Developer, but it is not very easy to request access for third parties. I can test whatever you need though.

[laboger]

I opened this golang issue for the failures on ppc64le: golang/go#15135.

[nathany]

Thanks @laboger. Are you planning to do a CL for syscall?

Should fsnotify switch from using standard libraries syscall to https://godoc.org/golang.org/x/sys?

[laboger]

Yes I will create a CL and test it for ppc64 & ppc64le. I'm not sure if this fixes arm64 and can't test it there. There might be more than one issue to fix, I'll have to see once I can try it.

I think it is best to fix the Go standard libraries syscall and use that. I'm not familiar with what's in golang.org/x/sys or why you'd use it instead.

[nathany]

Fixing the standard libraries syscall would be preferable. I just thought it was frozen since Go 1.3, but maybe bugfixes are still okay. Thanks.

[laboger]

I have been testing a patch on ppc64 and ppc64le. All the tests say PASS but when it gets to the end it says FAIL and I'm not sure why. If I run it multiple times in a row it will sometime say PASS at the end. Any clues?

[nathany]

Is it timing out? Many of the tests only wait so long for the notifications to occur.

[NightTsarina]

@laboger Care to share that patch? I could run some tests too.

[suihkulokki]

I've submitted a proposed fix for the arm64 side to:
https://go-review.googlesource.com/#/c/21971/
@TheTincho how do I test the prometheus error?

[NightTsarina]

@suihkulokki To test that, get the current prometheus source from Debian sid, remove the 07-Disable_fsnotify.patch patch from the series and just try to build the package.

Thanks for working on this!

[NightTsarina]

Alternatively, you can just download and build prometheus from upstream, which will run al the tests (fsnotify is not yet disabled upstream, but there is a pending PR)

[nathany]

@TheTincho Can you test arm64 with v1.3.0. It should be working now that @suihkulokki's patch is in: https://go-review.googlesource.com/#/c/21971/

[nathany]

We really need to get builders #136 setup for these environments. That way we can ensure they are continuing to work with other changes, and we could also see any test failures that occur.

[nathany]

@laboger Are you still seeing test failures sometimes? If you are running with the verbose (-v) option it can be difficult to see the failure amidst all the other logs.

[laboger]

@nathany I am unable to build the fsnotify test now on ppc64le with the latest golang.

src/github.com/fsnotify/fsnotify/inotify.go:39: undefined: unix.InotifyInit
src/github.com/fsnotify/fsnotify/inotify_poller.go:48: undefined: unix.Pipe2

Looks like the switch was made to use golang.org/x/sys/unix but that is missing some functions for ppc64le and ppc64. (Same errors in both)

[nathany]

@laboger Apologies. I merged that change before looking at the work you had done on the syscall library. Would you be willing to submit your syscall changes and any other necessary changes as a CL on golang.org/x/sys/unix? That will enable powerpc to work even on Go 1.5 rather than requiring 1.6.2.

I'm looking into getting Go's builder machines in place for fsnotify. That way we can ensure it is tested across linux/arm64 and ppc to avoid such issues in the future.

[laboger]

I created this golang issue to make the necessary updates to golang.org/x/sys/unix. golang/go#15393.

[nathany]

Thanks Lynn.

[suihkulokki]

Sorry @laboger, I didn't notice ppc was fixed in syscall instead of x/sys. I ended up patching x/sys since some earlier syscall changes I had proposed were rejected and I was told to update x/sys instead.

[laboger]

@suihkulokki, can you clarify what you mean that previous syscall changes were rejected? Do you mean by the golang community or fsnotify?

BOTH golang syscall and golang.org/x/sys/unix had incorrect implementations of the structure EpollEvent for use with epoll syscalls on ppc64x and both need to be fixed if they are going to be used. I'm not sure why you'd fix one and not the other.

[nathany]

@laboger syscall was supposedly frozen when x/sys was extracted.

[nathany]

@laboger Here is the relevant proposal and mailing list thread

https://docs.google.com/document/d/1QXzI9I1pOfZPujQzxhyRy6EeHYTQitKKjHfpq0zpxZs/edit

https://groups.google.com/forum/#!topic/golang-dev/gyOoSwxBkCA

We therefore propose to freeze the package as of Go 1.3

[nathany]

This whole thing has brought to light something I didn't realize. There are quite a lot of changes to the "frozen" syscall package since September 2014 (when Go 1.4 was released and syscall was "frozen"). Perhaps it just was the pragmatic thing to do to keep things working, but it certainly isn't frozen.

The copy to x/sys happened in August 2014 and x/sys has seen a lot of changes too. But it does make me wonder if there aren't fixes and improvements in syscall that haven't made it so x/sys.

https://github.com/golang/go/commits/master/src/syscall

[nathany]

So I've posted a message to golang-dev regarding this whole confusing situation with syscall vs. x/sys: https://groups.google.com/forum/#!topic/golang-dev/cEASnHIXmLI

I still think the switch to x/sys is a good move simply to support previous versions of Go, but I'm somewhat inclined to do a diff of the two libraries to see if we're missing any syscall patches.

[suihkulokki]

According to git log on syscall, the following changes there are missing from x/sys:

• s390x support
• the epollevent struct fix for ppc64x already mentioned.

[nathany]

There is an open CL for ppc from @clnperez.

https://go-review.googlesource.com/#/c/22605/
golang/go#15393

I'm not sure about s390x.

[nathany]

The change list (CL) has been submitted. Thanks Christy.

go get -u golang.org/x/sys/unix

I don't think any changes are need to fsnotify itself for this update.

@TheTincho Can you please test it out.

[clnperez]

Np @nathany. I ran the fsnotify tests on ppc64le as part of my patch validation before submitting and things seemed fine, but more testing is always a good thing.

[NightTsarina]

Thanks everybody! I am preparing updated packages for x/sys and fsnotify. Will update with test results as soon as I have them.

[NightTsarina]

I have tested this in the 3 affected arches, and everything seems to work correctly now. Thanks to everybody involved!!