guard / rb-inotify

A thorough inotify wrapper for Ruby using FFI.
MIT License
312 stars 64 forks source link

Better coordination/shared organisation #66

Open ioquatix opened 7 years ago

ioquatix commented 7 years ago

Hello Everyone. I did make some individual issues on these points but I thought I'd make one shared issue here to get everything in one place.

  1. Shared organisation. I'd like to propose that we have a shared organisation for rb-inotify, rb-kqueue, and rb-fsevent.

    I'm trying to bring everyone together to form a consensus.

    There are two ideas I have

    • Using the guard organisation.
    • Creating a new organisation.
  2. I'd like to propose that we consider renaming the gems. The rb- prefix is pretty ugly, and quite non-standard by modern standards. Some ideas I have include os-, fs-, sys-. Open to suggestions/discussion.

  3. Coordinated 1.0 release. I think there is an opportunity for us to all get to 1.0 at the same time. It's not essential, but it would be nice if we could invest the time to achieve this.

  4. Shared test suite. Since these gems are largely doing the same thing, if we exposed a uniform high-level interface, we could also have a shared high level rspec test suite. It's really easy to do, and the benefits would be great - confirming the same behaviour over multiple platforms.

cc @mat813 @rymai @thibaudgg

ioquatix commented 7 years ago

Other things we (could) standardise on:

ioquatix commented 7 years ago

Hmm, there is one more thing I've been working on recently - and that is a gem called async.

https://github.com/socketry/async

It would be nice if we could expose the individual underlying IO objects in a way that was easily composable with async. I know that rb-inotify can expose the underlying io object.

     # Wait 10 seconds for an event then give up
     if IO.select([notifier.to_io], [], [], 10)
       notifier.process
     end

It would be nice if we had some consistency across the different implementations w.r.t. this behaviour/api.

thibaudgg commented 7 years ago

@ioquatix thanks for starting the discussion, I'll think about it and give feedback before the end of the week.

I would like to bring @e2 (guard and listen maintainer) and @ttilley (main latest rb-fsevent version author) to the discussion.

junaruga commented 7 years ago

Thanks for starting this discussion.

  1. => I agree! The organization repository is much better than individual repository. In my opinion, using guard organization looks better. But using new organization is also fine. The benefits of using the organization repository are

    • It is good to scale the projects. Good for future's change.
    • Several maintainers are better than a maintainer for a project to act quickly. It's hard for a maintainer to get long vacation if he/she is only one maintainer in a project. :)
    • People can recognize actual maintainers from "People" area.
  2. => I can understand that the rb- prefix of the gem name is not good. But how do we notify users changing the gem name? Is there a redirect feature in rubygems.org? Redirect from https://rubygems.org/gems/rb-inotify to https://rubygems.org/gems/new-pacakge Can we write changing the gem name on rb-inotify page in rubygems.org? rb-inotify has been used as a part of Rails app's dependency. When user runs the command rails new app, rb-inotify is used, as rb-inotify is a runtime dependency of listen. Maybe Rails users and gem package maintainers who are using rb-inotify such as listen, want to know it to change their gemspec file. I want to prevent them to misunderstand "rb-inotify is dead. there is no alternative way". [1] https://github.com/rails/rails/blob/v5.1.1/railties/lib/rails/generators/rails/app/templates/Gemfile#L52

  3. => Agree.

  4. => Agree.

  5. Other things we standardise on: => Agree.

  6. async => I can not judge it.

ioquatix commented 7 years ago

On point 2, I think there is only one good solution.

We make new gem by forking existing code.

The existing gem is released as 1.0 but depends on new gem, and exposes new gem via old (current) interface. It can include suitable deprecation warning/notice.

None of the gems reached 1.x so semantically I think this isn't a bad option.

ioquatix commented 7 years ago

There is one other option to consider. I'd like to have your feedback too.

Does it really make sense for these to be separate gems?

Yes, they are really different.

But, they are basically doing the same thing. The implementation detail can be quite different, but it could be hidden within platform specific drivers. This way, people who depend on this code don't need to have their own platform specific selection/loading (e.g. what listen is doing).

I don't mind if we have one gem, it exposes several different ways to do things, but also has a set of common lowest common denominator APIs.

junaruga commented 7 years ago

Does it really make sense for these to be separate gems?

That's a great point. If we need "Shared test suite. Since these gems are largely doing the same thing", essentially these gems can be unified.

I agree with you.

junaruga commented 7 years ago

We make new gem by forking existing code.

The existing gem is released as 1.0 but depends on new gem, and exposes new gem via old (current) interface. It can include suitable deprecation warning/notice

It looks good to me.

ioquatix commented 7 years ago

An argument against having a single gem relates to dependencies.

For example rb-inotify uses ffi, while rb-fsevent uses a hand compiled binary (?). Rolling these all into one gem might be an issue? Can we have platform specific gem dependencies?

How does listen handle this?

ioquatix commented 7 years ago

Thought I'd just mention this here as I saw it: https://github.com/guard/guard/wiki/Maintainers

junaruga commented 7 years ago

In the case of listen, Maybe it manages the platform specified logic as adapter file. https://github.com/guard/listen/tree/master/lib/listen/adapter

Abut the gem dependencies, the policy is like this way. It requires platform specific gem in the adapter file. https://github.com/guard/listen/pull/417

And managing the Mac specific library rb-fsevent in listen.gemspec like below URL. https://github.com/guard/listen/blob/v3.1.5/listen.gemspec#L33

My suggestion for this situation is that I want comment in listen.gemspec. Such as

# rb-fsevent is required for Mac platform.
 s.add_dependency 'rb-fsevent', '~> 0.9', '>= 0.9.4'

Because below file is to create listen's RPM pacakge on Fedora Project (One of the Linux Distribution).

http://pkgs.fedoraproject.org/cgit/rpms/rubygem-listen.git/tree/rubygem-listen.spec L44

sed -i '/^..$/ s/^/#/' .%{gem_spec}

This line in the file is to comment out rb-fevent inserting # to the top of line, not to download Mac platform specified rb-fsevent library. I am a person who is working on Fedora Project.

I am fine for this hack. However above comment is helpful for packaging guys who are managing the platform specified package for Ubuntu, Fedora, or Mac Brew to recognize those easily.

thibaudgg commented 7 years ago

Hey, here my thoughts/comments:

  1. Sounds like a good idea, I would be fine moving them under the Guard authorization. You can ask me for the authorization.

  2. That one is tricky, I think it would be OK to "rename" the gems (fs-macos, fs-linux ?) by updating the current gems to 0.99 with the existing code and a post_install_message about the new gem name and the deprecation of the current name. The new gem would be released with 1.0 and gems like listen would then use directly the new ones. WDYT?

  3. Would be nice.

  4. Would be nice as well, although I'm not sure what would be the best way to share the specs between each gem.

I would also point out that each filesystem has its own set of functionalities, some inotify features are not supported by fsevent and vice-versa. For that reason, I think that it makes more sense to keep one gem per filesystem, and IMHO the listen gem is already taking care of unifying all the shared features under one roof. Sometimes you also need to just support one specific platform and it's handy to be able to use one gem that supports all the filesystem features.

ioquatix commented 7 years ago

@thibaudgg Thanks for your input. I'd still like to hear from other interested parties before making any general plan of action, but your ideas are good.

ioquatix commented 7 years ago

Hello again. I've spent a few hours modernising the rb-inotify gem to use bundler, merged in many of the outstanding bug fix PRs, and other things. Unfortunately the test suite is small. I'll try to work on that.

ioquatix commented 7 years ago

Just an idea: What about naming gems e.g. listen-inotify. We can expose platform specific code, but also an adapter. I would like to believe the listen gem has got a nice test suite already. listen-inotify can sit inside module Listen::INotify and the gem itself could provide the generic adaptor some how.

I wonder if this is coupling too much to listen? However, that gem is basically the primary consumer of rb-inotify right?

ioquatix commented 7 years ago

Additionally, I've just been given access to guard org, so I'll be moving this gem to there.

thibaudgg commented 7 years ago

That would be a solution, @rymai / @e2 what do you think of that?

ioquatix commented 7 years ago

After moving this repo to guard org, I no longer have the ability to change any of it's settings. Can someone fix that?

ioquatix commented 7 years ago

I suggest we use the guard org's listen team, and make everyone here part of that, but yeah it would be great if I can still admin the project :D

ioquatix commented 7 years ago

Okay, so I've done some tests. I found in my case, the best way to test rb-inotify was to install listen source code, rake install for rb-inotify, bundle update for listen, and then rspec in listen, which appears to hammer the rb-inotify code base quite a bit. One option might be embedding this into rb-inotify so that travis runs a decent number of tests.

ioquatix commented 7 years ago

Found some old dirt:

https://github.com/guard/listen/issues/274 https://github.com/guard/listen/issues/25

Maybe worth revisiting too.

rymai commented 7 years ago

@ioquatix Thanks!

listen-inotify as well as https://github.com/guard/rb-inotify/issues/66#issuecomment-302695991 and https://github.com/guard/rb-inotify/issues/66#issuecomment-302873496 also make sense to me.

ioquatix commented 7 years ago

I'm leaning towards listen-inotify, listen-fsevent and listen-kqueue. That doesn't mean someone can't depend on them independently, if they only want a subset of functionality. Can we have a vote on this? Thumbs up for support, thumbs down for against.

ioquatix commented 7 years ago

On point 4, I've been trying to figure out how we can do this.

The listen gem has an awesome test suite. I think we can leverage this (and perhaps guard too). When travis runs tests for listen-inotify, I'd also like it to pull the source for listen and guard and run those full test suites, using the current checkout of listen-inotify. I think if we standardise on this, we can just build on top of listen and guard tests. And, let's face it, if we deploy an updated listen-$adaptor gem, we definitely want to make sure it doesn't break listen/guard so I think it's appropriate. I'll investigate how to best do this. If anyone has any ideas, let me know.

thibaudgg commented 7 years ago

:+1: for guard/listen-* gem names, let me know when to create the repositories and at who give admin access. Please note, that I'll not have a lot of time to migrate rb-fsevent to listen-fsevent before the end of June.

ttilley commented 7 years ago

I would like to apologize for not participating in this conversation up until now. I haven't had internet access for well over a week and to say that life has been complicated is a massive understatement.

@thibaudgg I'll be better able to catch up on this topic tomorrow after 4-5PM EST. Tell me exactly what you need from me and I'll make it a personal priority. rb-fsevent has suffered from neglect for some time now. If making it into a much more flexible library that's usable via FFI is beneficial to you, it wouldn't be quite as painful for me to accommodate that request as you may be thinking (though I would still provide an fsevent_watch binary for use outside ruby, as I have made use of this raw frontend on its own on more than one occasion outside ruby).

I would be able to provide this to you well before your personal end of June target, as long as I have a very clear idea of what you need from me.

Keep in mind that rb-fsevent is doing very horrible and dangerous things to avoid bugs in MacOS 10.7-10.11, and making use of FFI would duck punch some very low level system APIs for the entirety of the process rather than just the external subprocess. This will change the behavior of ruby itself after loading rb-fsevent wherever realpath is made use of... and the replacement is much much slower.

ioquatix commented 7 years ago

I haven't had internet access for well over a week and to say that life has been complicated is a massive understatement.

I'm sure we are all sorry to hear that and I hope whatever is causing your complexity in your life is soon behind you and you can move forward in a positive light.

thibaudgg commented 7 years ago

@ttilley I also sorry to hear about your complicated situation, please take all the time you need before replying to that one.

Using FFI would be nice. If we rename the gem and upgrade the version I would be also fine dropping some old macOS versions to avoid horrible and dangerous hacks 😱. I think the best would be to list advantage/drawback on only supporting recent version and switching to FFI directly on a new rb-fsevent issue, could you start that one?.

We could also wait on the shared test setup proposition from @ioquatix to see how well it could fit listen-fsevent.

Looking forward your feedback, take care.

ttilley commented 7 years ago

​Please forgive me if I'm slightly stream-of-consciousness here in this reply... It was written on and off over the course of a few hours with distractions in between, then re-posted because replying from gmail destroyed the formatting.

Using FFI would be nice. If we rename the gem and upgrade the version I would be also fine dropping some old macOS versions to avoid horrible and dangerous hacks 😱.

​Rather than getting rid of these hacks entirely, I could split them out into a separate dylib since with FFI that's absolutely doable. They are, unfortunately, still necessary on 10.7-10.11 no matter how ugly and potentially dangerous they are. ​My personal thoughts on the matter:

​The vast majority of our user base is already on 10.12, making this a completely invisible problem for most people. I just don't want to cause anyone undue stress, or waste anyone's time debugging a known issue (even if those hours are billable), by not providing a solution we already have. I believe it's really important to have some empathy for the user here... especially when it took years of frustration to develop that solution and get Apple to fix the underlying cause. 😫

switching to FFI directly on a new rb-fsevent issue, could you start that one?.

​Absolutely. I still need a few hours, but it is now officially on my todo list for today. I will also mark anything else up there as "will not fix" because it's the only honest response.

Given the drastic change of switching from an external pre-compiled binary to FFI, i'm not sure it's a good idea to do so within rb-fsevent. Many people depend on it, and I'm guessing most (pretty much all) of them don't want to have to care that rb-fsevent even exists. Let's give them the option of not having to care and just continue to use what works for them (until whatever underlying thing they work with makes the move for them, be it guard or something else).

My thoughts:

  1. do the work on a branch of rb-fsevent that isn't really intended to see the light of day as an rb-fsevent release
  2. have the FFI version be its own thing as listen-fsevents when ready (the API name is plural and it'll relieve my OCD to have the library name match)
  3. let things settle a bit and ensure any edge cases or oddities are covered by tests
  4. after we (the guard team) migrate to the new library, switch the current content of rb-fsevent to a branch named "stable" and replace "master" with nothing more than a readme explaining the situation, giving any potential user the option of migrating to listen-fsevents or just using the "stable" branch

Though, to be fair, your input will ultimately matter the most as I'm somewhat of a comet when it comes to this project... I come and go erratically and my involvement cannot always be depended on in the same way that yours can.

We could also wait on the shared test setup proposition from @ioquatix to see how well it could fit listen-fsevent.

​I look forward to any developments that come from this. What we want to have as "correct behavior" is absolutely not platform specific, even if each backend behaves differently. Anything not directly related to OS quirks should be shared (and trust me, there are MacOS quirks... especially with the coming migration to APFS).

​...and with that, I'll stop cluttering up this issue until I have one open on rb-fsevent I can link to.​

ttilley commented 7 years ago

just so nobody absolutely has to read that massive wall of text, i'd like to veto listen-fsevent in favor of listen-fsevents, as the underlying API uses a plural name and there's no reason for the library name to be singular. it might even be a bit confusing. Other than that, you have both my thumbs emphatically pointed in an upward direction.

ioquatix commented 7 years ago

have the FFI version be its own thing as listen-fsevents when ready (the API name is plural and it'll relieve my OCD to have the library name match)

I'm okay with this form of OCD, I even support it.

ttilley commented 7 years ago

another brief so you don't have to read that wall of text is that i am very much AGAINST releasing a version of rb-fsevent that's essentially the new version, but with a post install message to move on. for this library, the situation is somewhat unique as migrating to FFI is a bigger deal than the changes the other backends are undergoing. in the interests of empathy and keeping stress levels of users to an absolute minimum, I want to keep rb-fsevent as-is for anyone it works just fine for. a post install message informing of the deprecation and existence of a new library fits my particular situation much more kindly. I also have one bug I want to fix in rb-fsevent despite the fact it would never exist in an FFI based version...

besides, the low level backend libraries don't have many direct users, so the issue isn't nearly as large as it might seem at first glance. anyone who cares in the least about portability is already using a higher level library and should never need to care (and will probably never notice) that a change has even occurred.

ioquatix commented 7 years ago

I think it's okay to continue maintaining the existing gems, hell, even a 1.0 release might make sense, but with a deprecation notice - this is no longer being maintained - move to listen-*

ioquatix commented 7 years ago

I can't really make much progress on this until someone makes a team and gives me admin access over the repo I migrated - is that possible?

junaruga commented 7 years ago

@ioquatix

gives me admin access over the repo I migrated

You mean that you want admin access to below repositories and to entire guard projects, right?

https://github.com/guard/listen https://github.com/mat813/rb-kqueue https://github.com/thibaudgg/rb-fsevent

ioquatix commented 7 years ago

@junaruga Ideally that would be great.

ttilley commented 7 years ago

If a guard/listen-fsevents or some such repo is created, please let me know and give me access. what do we want to do for normalizing the module namespaces of backends? @ioquatix

ttilley commented 7 years ago

or should I wait and just take the example of guard/listen-inotify?

ioquatix commented 7 years ago

My suggestion is to follow the rubygems naming guide

So, module Listen::INotify, module Listen::FSEvent, module Listen::KQueue.

I think a class with duck type interface module Listen::X::Monitor would be great.

ioquatix commented 7 years ago

Ideally, I'd like to see class Monitor have a function to_io which allows us to use it with a select loop.

thibaudgg commented 7 years ago

@ioquatix @ttilley

I've created the listen-inotify, listen-fsevents, and listen-kqueue repositories and give the admin right to the Listen team (which you're part of).

Please let me know if you need more rights. :smile:

ioquatix commented 7 years ago

@thibaudgg thanks can you please also add https://github.com/guard/rb-inotify to the same team.

thibaudgg commented 7 years ago

@ioquatix done!

ioquatix commented 7 years ago

I've just release rb-inotify with a few bug fixes and improvements from PRs. I think it may be the last release - if it proove stable I will release as 1.0

ioquatix commented 7 years ago

I would like to propose that listen-* gems don't have any internal asynchronous behaviour - they are entirely synchronus/blocking. If user wants non-blocking operation, they should provide a wrapper IO which implements nonblocking read. JRuby also now supports to_io I believe so we can leverage that.

ioquatix commented 6 years ago

Since this issue has been revived from the dead, I'll give an update of what I've been working on.

https://github.com/socketry/async is a concurrency library for Ruby.

As above, I mentioned the listen-* gems should be entirely synchronous - I still agree with that to some extent, it's a good baseline that can be integrated into higher level abstractions (e.g. threads, reactors, callbacks, or as linked, async).

That being said, it might be a good baseline for guard to become asynchronous. I don't know how it current works internally, but it's something I've been thinking about.

ioquatix commented 5 years ago

I've started working on bringing these gems up to date for Ruby 2.6

I'd like to revisit these issues.

Does anyone else still have bandwidth for this project?