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 error notification when using host mode (no device) #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

patsoffice
Copy link

Changes

  • When creating an EventStream with no device (dev number is 0) there would be
    a failed assertion message to stderr. The event stream device uuid is only
    gotten if the device number if non-zero.
  • Added unit test to create an event stream in host mode
  • Made temp file permissions non-executable

What does this pull request do?

When creating an EventStream with no device (dev id 0), there would be a failed assertion message output.

2018-07-17 20:48 fsevents.test[21765] (FSEvents.framework) FSEventsCopyUUIDForDevice(): failed assertion 'dev > 0'

This pull request changes the package to only get a device uuid if the device id is non-zero.

How should this be manually tested?

This can be tested by running:

go test -v

- When creating an EventStream with no device (dev number is 0) there would be
  a failed assertion message to stderr. The event stream device uuid is only
  gotten if the device number if non-zero.
- Added unit test to create an event stream in host mode
- Made temp file permissions non-executable
@patsoffice
Copy link
Author

This is a duplicate of #32 although it solves the problem differently.

@patsoffice patsoffice closed this Jul 18, 2018
@patsoffice
Copy link
Author

#32 does not appear to fix the issue. I closed this PR in haste apparently (re-opening).

@patsoffice patsoffice reopened this Jul 18, 2018
@codecov-io
Copy link

codecov-io commented Jul 18, 2018

Codecov Report

Merging #38 into master will increase coverage by 1.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   71.18%   73.01%   +1.82%     
==========================================
  Files           3        3              
  Lines         288      289       +1     
==========================================
+ Hits          205      211       +6     
+ Misses         61       57       -4     
+ Partials       22       21       -1
Impacted Files Coverage Δ
fsevents.go 79.48% <100%> (+0.53%) ⬆️
wrap_deprecated.go 71.54% <0%> (+1.62%) ⬆️
wrap.go 72.44% <0%> (+2.36%) ⬆️

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...c1ae385. Read the comment docs.

@nathany
Copy link
Contributor

nathany commented Aug 26, 2018

@patsoffice Thanks for the patch. This will need a little more work to support multiple versions of Go. Let's wait for #41 to be reviewed and merged and then we can tackle this.

@@ -158,7 +158,9 @@ func (es *EventStream) Start() {
// in C callback
cbInfo := registry.Add(es)
es.registryID = cbInfo
es.uuid = GetDeviceUUID(es.Device)
if es.Device != 0 {
Copy link
Contributor

@nathany nathany Jan 5, 2019

Choose a reason for hiding this comment

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

I'm not sure if this will need to be revised in light of the changes in #41.

We need to rebase this against master and test it with multiple versions of Go.

@nathany
Copy link
Contributor

nathany commented Oct 8, 2019

@patsoffice Do you want to take another look at this?

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.

3 participants