Create aix.go #314
Create aix.go #314
Conversation
Empty implementation, just to allow build
| ) | ||
|
|
||
| var ( | ||
| ErrWatchedFileDeleted = errors.New("error: watched file or folder deleted") |
There was a problem hiding this comment.
exported var ErrWatchedFileDeleted should have comment or be unexported
To take into account travis complaint
|
We're looking forward to using this change in https://github.com/sensu/sensu-go! Is there a timeline available for merging it? |
I wish it can be merged asap, but we need a review to be done |
|
@nathany any chance you can take a look at this soon? |
|
@tklauser do you have the power to get this merged? |
|
Unfortunately I don‘t. Maybe @cpuguy83 can help getting this merged? |
|
I don't understand why do we need the empty implementation? |
|
The empty implementation is needed so that programs that depend on fsnotify can be built on AIX. The fsnotify support won't work, but in a lot of cases that's better than the program failing to build at all. I think the empty implementation could probably be simplified. It looks like there's some unnecessary code that's not really doing anything. |
|
If fsnotify doesn't compile on a platform, why is it imported there? |
|
fsnotify in many cases is a dependency of a dependency of a dependency. If it doesn't build on AIX, then every package that imports it has to create build workarounds for AIX, should they wish to support AIX. You might argue that it's not fsnotify's responsibility to provide this convenience. I agree, but if compatibility can be offered here, it would save rather a lot of work for rather a lot of people! :) |
|
I'd be fine if a |
|
Hi guys, I've submitted sometimes ago a PR which enables fsnotify on AIX (#326). It would be better to have this version even if not perfect than to disable fsnotify completely. |
| // Use of this source code is governed by a BSD-style | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| // +build aix |
There was a problem hiding this comment.
should add the new style build tags too
| ) | ||
|
|
||
| var ( | ||
| ErrWatchedFileDeleted = errors.New("error: watched file or folder deleted") |
| var wg sync.WaitGroup | ||
| wg.Add(1) | ||
|
|
||
| w := &Watcher{ | ||
| Events: make(chan Event), | ||
| Errors: make(chan error), | ||
| closed: make(chan struct{}), | ||
| close: make(chan struct{}), | ||
| mu: new(sync.Mutex), | ||
| wg: &wg, | ||
| files: make(map[string]os.FileInfo), | ||
| names: make(map[string]bool), | ||
| } | ||
| go w.readEvents() | ||
| return w, nil |
There was a problem hiding this comment.
I don't know that an empty implementation should do this
There was a problem hiding this comment.
Why not just return errors.New("Not implemented")?
In cases where fsnotify is a transitive dependency of a package, it might need to be built on platforms not supported (yet), e.g. aix. Provide an empty implemention for these cases, so depending packages don't need to add workarounds. Replaces fsnotify#314
In cases where fsnotify is a transitive dependency of a package, it might need to be built on platforms not supported (yet), e.g. aix. Provide an empty implemention for these cases, so depending packages don't need to add workarounds. Replaces fsnotify#314
|
FWIW, I've opened #424 adding a minimal |
In cases where fsnotify is a transitive dependency of a package, it might need to be built on platforms not supported (yet), e.g. aix. Provide an empty implemention for these cases, so depending packages don't need to add workarounds. Replaces #314
Empty implementation, just to allow build
What does this pull request do?
It proposes an empty implementation, so that this can bu built on AIX
Where should the reviewer start?
Only one file to review, rather simple as all methods return either nil, or errors.New("not implemented")
How should this be manually tested?
This new file allows you to build the package on AIX.
This fsnotify package is used as a dependency but the runtime is not necessary.