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

Improve handling of different Go versions, address compile failure in Go 1.10.4+ #41

Merged
merged 5 commits into from
Sep 5, 2018

Conversation

nathany
Copy link
Contributor

@nathany nathany commented Aug 25, 2018

What does this pull request do?

@wadey Thanks for your work on #34. This pull request builds on that work and fixes #40.

Yesterday's release of Go 1.10.4 and Go 1.11 brought another related breakage (see golang/go#24161). The specific compiler error is cannot use nil as type _Ctype_CFAllocatorRef. This requires passing C.CFAllocatorRef(0) instead of nil to a few functions that we use.

In this pull request, I also made an effort to minimize the amount of code we need to maintain across different Go versions, replacing wrap_deprecated.go with a few new (smaller) files for different Go versions.

The fix that worked for Go 1.10.4 or better didn't work as expected in Go 1.10.3 (see https://travis-ci.org/fsnotify/fsevents/jobs/420640270). Declaring the C.CFAllocatorRef(0) constant in the same file works in all 1.10.x versions, but when I extract that constant to a different file (go_1_10_after.go) it continues to work on Go 1.10.4+ but fails in 1.10.3.

To get 1.10.x versions prior to 1.10.4 working, I had to extract larger amounts of code into Go version specific files. (all of GetDeviceUUID, a new MakeCFString and CopyCFString functions). It's truly odd, but it is working now.

Please take a look and let me know if it's okay for me to merge this.

Where should the reviewer start?

go_1_10_after.go

How should this be manually tested?

go test should suffice but certainly feel free to test in a tool that uses this if you have one.

@codecov-io
Copy link

codecov-io commented Aug 25, 2018

Codecov Report

Merging #41 into master will increase coverage by 1.23%.
The diff coverage is 73.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   71.18%   72.41%   +1.23%     
==========================================
  Files           3        4       +1     
  Lines         288      174     -114     
==========================================
- Hits          205      126      -79     
+ Misses         61       35      -26     
+ Partials       22       13       -9
Impacted Files Coverage Δ
go_1_10_before.go 60% <60%> (ø)
go_1_10_after.go 60% <60%> (ø)
wrap.go 71.42% <88.88%> (+1.34%) ⬆️

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 ed9a50f...9569b2a. Read the comment docs.

@nathany nathany force-pushed the fix-test branch 7 times, most recently from fad77b0 to 868cf81 Compare August 25, 2018 22:38
deprecated and 2.0 doesn’t appear to support Go on macOS (though may be able to get it to work later).

https://circleci.com/docs/2.0/testing-ios/
@nathany nathany force-pushed the fix-test branch 3 times, most recently from 50d3b75 to 0e9af6b Compare August 25, 2018 23:50
@nathany nathany changed the title Cannot use nil for CFType (Go 1.10+) [WIP] Cannot use nil for CFType (Go 1.10+) Aug 25, 2018
@nathany nathany force-pushed the fix-test branch 3 times, most recently from a938102 to 7db57d0 Compare August 26, 2018 01:16
@nathany nathany changed the title [WIP] Cannot use nil for CFType (Go 1.10+) Improve handling of different Go versions, address compile failure in Go 1.10.4+ Aug 26, 2018
@xenoscopic
Copy link

Would it make more sense to use C.kCFAllocatorDefault instead of C.CFAllocatorRef(0)? The former is a bit more explicit in terms of desired behavior, especially since you're just using a constant anyway. This is what was done in rjeczalik/notify#162 to fix this issue.

On a side note: Thanks for keeping this project alive! It's super useful to be able to do native recursive watches, even if it doesn't currently fit into fsnotify's API.

@nathany
Copy link
Contributor Author

nathany commented Sep 4, 2018

@havoc-io Thanks for the tip. Indeed kCFAllocatorDefault would make more sense.

@nathany
Copy link
Contributor Author

nathany commented Sep 4, 2018

@havoc-io How does this look now? I've pushed 2 new commits based on your feedback.

@nathany
Copy link
Contributor Author

nathany commented Sep 4, 2018

Other than squashing some commits, are there any other changes needed before merging this?

@xenoscopic
Copy link

@nathany That looks good to me as far as the switch to kCFAllocatorDefault goes. Thanks again for this update.

travis: update golint location

travis: macOS 10.10

update Travis CI config

travis: update golint location

travis: macOS 10.10
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 nathany merged commit 0739535 into master Sep 5, 2018
@nathany nathany deleted the fix-test branch September 5, 2018 15:51
lmars added a commit to lmars/puma-dev that referenced this pull request Jan 2, 2019
To pull in the following fix:

fsnotify/fsevents#41

Signed-off-by: Lewis Marshall <lewis@lmars.net>
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.

Failing test on Go 1.10+: cannot use nil as type...
3 participants