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

fix for Go 1.10 #34

Merged
merged 4 commits into from
Feb 23, 2018
Merged

fix for Go 1.10 #34

merged 4 commits into from
Feb 23, 2018

Conversation

wadey
Copy link
Contributor

@wadey wadey commented Feb 20, 2018

Fixes #33 and #31.

A couple of fixes so that fsevents builds in Go 1.10. Sadly this breaks
compatibility with Go 1.9 and below, but there doesn't seem to be any
way to avoid that. I have not fully vetted it, just confirmed that it builds and the example runs.

  • the kFSEventStreamEventIdSinceNow type is now correctly unsigned, so
    the the need to convert from a signed integer is no longer needed.

  • nil must be changed to 0 in a few places now.

  • We must cast to the correct C type in a few places.

A couple of fixes so that fsevents builds in Go 1.10. Sadly this breaks
compatibility with Go 1.9 and below, but there doesn't seem to be any
way to avoid that.

- the `kFSEventStreamEventIdSinceNow` type is now correctly unsigned, so
the the need to convert from a signed integer is no longer needed.

- nil must be changed to 0 in a few places now.

- We must cast to the correct C type in a few places.
@nilleb
Copy link

nilleb commented Feb 20, 2018

we could perhaps use the flag go1.10 changing a little the file organization (in order not to break the backwards compatibility? I volunteer to do that)

@wadey
Copy link
Contributor Author

wadey commented Feb 20, 2018

For a quick fix could just make a complete copy of the old wrap.go and put it behind the build flag and not waste time on reorganization

@nilleb
Copy link

nilleb commented Feb 20, 2018

mh, no, unfortunately it won't work. unless we don't modify the before_script part of the job
(in fact, the go1.9 means from go 1.9 onward, thus will be always compiling this file -- creating conflicts with the 1.10 one)

There were backwards incompatible changes to cgo in Go 1.10:

- https://golang.org/doc/go1.10#cgo
@wadey
Copy link
Contributor Author

wadey commented Feb 20, 2018

I pushed a change that should build for all versions of Go, the travis build passed for latest Xcode:

It failed for the oldest version of Xcode though, for what seems like an unrelated issue. I'm testing building with multiple versions of Go now.

Test with Go 1.8, 1.9, 1.10 and latest stable (in case we forget to
update this later). sw_vers is only present on the osx_image builds so
make the output for it conditional.
@codecov-io
Copy link

codecov-io commented Feb 20, 2018

Codecov Report

Merging #34 into master will decrease coverage by 0.32%.
The diff coverage is 41.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #34      +/-   ##
=========================================
- Coverage   43.03%   42.7%   -0.33%     
=========================================
  Files           2       3       +1     
  Lines         165     288     +123     
=========================================
+ Hits           71     123      +52     
- Misses         84     147      +63     
- Partials       10      18       +8
Impacted Files Coverage Δ
wrap.go 42.51% <20%> (ø) ⬆️
wrap_deprecated.go 42.27% <42.27%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ceee05...a2c2f90. Read the comment docs.

@wadey
Copy link
Contributor Author

wadey commented Feb 20, 2018

Ok, I've pushed some changes to the Travis config so that it tests with Go 1.8, 1.9 and 1.10. All builds pass: https://travis-ci.org/fsnotify/fsevents/builds/344036544

@nilleb
Copy link

nilleb commented Feb 21, 2018

I hope this patch will be merged soon! :-)

@drewwells
Copy link
Contributor

wrap_go1.9 probably would have been sufficient :D

@drewwells
Copy link
Contributor

Testing this in my project, tests look good. We're losing some coverage b/c of the duplication of code

@drewwells
Copy link
Contributor

great work, thank you for your contribution 🎉 🎉 🎉

@drewwells drewwells merged commit 5cff8fb into fsnotify:master Feb 23, 2018
mlarraz added a commit to enova/puma-dev that referenced this pull request Mar 25, 2018
This pulls in fsnotify/fsevents#34,
which fixes Go 1.10+ support on mac OS.
mlarraz added a commit to enova/puma-dev that referenced this pull request Mar 25, 2018
This pulls in fsnotify/fsevents#34,
which fixes Go 1.10+ support on mac OS.
andrewkroh pushed a commit to elastic/fsevents that referenced this pull request Apr 5, 2018
* fix for Go 1.10

A couple of fixes so that fsevents builds in Go 1.10. Sadly this breaks
compatibility with Go 1.9 and below, but there doesn't seem to be any
way to avoid that.

- the `kFSEventStreamEventIdSinceNow` type is now correctly unsigned, so
the the need to convert from a signed integer is no longer needed.

- nil must be changed to 0 in a few places now.

- We must cast to the correct C type in a few places.

* add wrap_deprecated.go for Go 1.9 and older

There were backwards incompatible changes to cgo in Go 1.10:

- https://golang.org/doc/go1.10#cgo

* Update travis.yml

Test with Go 1.8, 1.9, 1.10 and latest stable (in case we forget to
update this later). sw_vers is only present on the osx_image builds so
make the output for it conditional.

* fix to work with Go 1.9.2+
andrewkroh pushed a commit to elastic/fsevents that referenced this pull request Apr 5, 2018
* fix for Go 1.10

A couple of fixes so that fsevents builds in Go 1.10. Sadly this breaks
compatibility with Go 1.9 and below, but there doesn't seem to be any
way to avoid that.

- the `kFSEventStreamEventIdSinceNow` type is now correctly unsigned, so
the the need to convert from a signed integer is no longer needed.

- nil must be changed to 0 in a few places now.

- We must cast to the correct C type in a few places.

* add wrap_deprecated.go for Go 1.9 and older

There were backwards incompatible changes to cgo in Go 1.10:

- https://golang.org/doc/go1.10#cgo

* Update travis.yml

Test with Go 1.8, 1.9, 1.10 and latest stable (in case we forget to
update this later). sw_vers is only present on the osx_image builds so
make the output for it conditional.

* fix to work with Go 1.9.2+
ruflin pushed a commit to elastic/fsevents that referenced this pull request Apr 6, 2018
Cherry pick of fsnotify#34 to enable Go 1.10 compilation.

> * fix for Go 1.10
> 
> A couple of fixes so that fsevents builds in Go 1.10. Sadly this breaks
> compatibility with Go 1.9 and below, but there doesn't seem to be any
> way to avoid that.
> 
> - the `kFSEventStreamEventIdSinceNow` type is now correctly unsigned, so
> the the need to convert from a signed integer is no longer needed.
> 
> - nil must be changed to 0 in a few places now.
> 
> - We must cast to the correct C type in a few places.
> 
> * add wrap_deprecated.go for Go 1.9 and older
> 
> There were backwards incompatible changes to cgo in Go 1.10:
> 
> - https://golang.org/doc/go1.10#cgo
> 
> * Update travis.yml
> 
> Test with Go 1.8, 1.9, 1.10 and latest stable (in case we forget to
> update this later). sw_vers is only present on the osx_image builds so
> make the output for it conditional.
> 
> * fix to work with Go 1.9.2+
nathany added a commit that referenced this pull request Aug 25, 2018
Go-version specific files.

reworks #34 which resolves int overflow #31
nathany added a commit that referenced this pull request Aug 25, 2018
Go-version specific files.

reworks #34 which resolves int overflow #31
nathany added a commit that referenced this pull request Aug 26, 2018
Go-version specific files.

reworks #34 which resolves int overflow #31
nathany added a commit that referenced this pull request Aug 26, 2018
Go-version specific files.

reworks #34 which resolves int overflow #31
nathany added a commit that referenced this pull request Aug 26, 2018
Go-version specific files.

reworks #34 which resolves int overflow #31
nathany added a commit that referenced this pull request Sep 5, 2018
fixes #40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks #34 which resolves int overflow #31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
nathany added a commit that referenced this pull request Sep 5, 2018
fixes #40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks #34 which resolves int overflow #31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
vjsamuel pushed a commit to vjsamuel/fsevents that referenced this pull request Oct 27, 2018
fixes fsnotify#40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks fsnotify#34 which resolves int overflow fsnotify#31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
vjsamuel pushed a commit to vjsamuel/fsevents that referenced this pull request Oct 27, 2018
fixes fsnotify#40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks fsnotify#34 which resolves int overflow fsnotify#31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
andrewkroh pushed a commit to elastic/fsevents that referenced this pull request Oct 29, 2018
fixes fsnotify#40

Cannot use nil for CFType (Go 1.10+), round 2

reducing code changes between Go versions

conversions can occur in older Go versions too

extract eventIDSinceNow constant to other files

Go-version specific files.

reworks fsnotify#34 which resolves int overflow fsnotify#31

minimize code differences between Go versions

Work around bizarre bugs in 1.10.3 (not 1.10.4)

golang/go#24161

use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)

thanks to @havoc-io for the tip

reduce duplication between go 1.10 before/after

thanks to C.kCFAllocatorDefault change
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 this pull request may close these issues.

Go 1.10 support
4 participants