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

Better coordination/shared organisation #66

Open
ioquatix opened this issue May 17, 2017 · 47 comments
Open

Better coordination/shared organisation #66

ioquatix opened this issue May 17, 2017 · 47 comments

Comments

@ioquatix
Copy link
Member

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
Copy link
Member Author

ioquatix commented May 17, 2017

Other things we (could) standardise on:

  • RSpec
  • Travis testing
  • RuboCop
  • Naming conventions
  • High level interface/shared specs
  • Modern gem structure (require_relative, bundler, etc)

@ioquatix
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Collaborator

junaruga commented May 17, 2017

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
Copy link
Member Author

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
Copy link
Member Author

ioquatix commented May 17, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

ioquatix commented May 17, 2017

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
Copy link
Member Author

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

@junaruga
Copy link
Collaborator

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.
guard/listen#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
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@thibaudgg
Copy link
Member

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

@ioquatix
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

Found some old dirt:

guard/listen#274
guard/listen#25

Maybe worth revisiting too.

@rymai
Copy link
Member

rymai commented May 21, 2017

@ioquatix Thanks!

listen-inotify as well as #66 (comment) and #66 (comment) also make sense to me.

@ioquatix
Copy link
Member Author

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
Copy link
Member Author

ioquatix commented May 21, 2017

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
Copy link
Member

👍 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
Copy link
Member

ttilley commented May 23, 2017

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
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member

ttilley commented May 23, 2017

​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:

  • write a wiki page describing the problem, why such insane low level trickery is required, and also why changing the behavior of realpath() is problematic considering it'll affect the entirety of ruby and not just this tiny library. this page should contain a strongly worded suggestion to upgrade to 10.12 unless you have an extremely compelling reason not to... and it damn well better be good.
  • output a warning to stderr when running on 10.7-10.11 that's as brief as possible and contains a link to said wiki page... if, and only if, the call is made to start watching files before the hack is loaded (messages to stderr get annoying very quickly)
  • don't do a damn thing on 10.12 (if we want to get technical here, 10.12 contains security enhancements that prevent such low level hacks in the first place)
  • don't load the hack by default, since the stakes are higher when we're altering the behavior of ruby itself. this difference in behavior from rb-fsevent should be noted in the documentation somewhere... probably in both the wiki and readme
  • add either a configurable option or a specific method that loads the hack (but do nothing on 10.12... preferably silently)
  • have a commandline option for loading said hacks in the bundled fsevent_watch binary and anything else making use of the library. whether or not this option is hidden from --help when running on 10.12 is something to think about, but I have no strong opinions either way.
  • potentially use the shaming equivalent of syntactic vinegar when naming that commandline option. I don't want to be too harsh here, so nothing like --i-refuse-to-upgrade-macos or --i-hate-safe-filesystem-operations, but you get my point. ::grin::

​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
Copy link
Member

ttilley commented May 24, 2017

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
Copy link
Member Author

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
Copy link
Member

ttilley commented May 24, 2017

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Collaborator

@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
Copy link
Member Author

@junaruga Ideally that would be great.

@ttilley
Copy link
Member

ttilley commented Jun 14, 2017

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
Copy link
Member

ttilley commented Jun 14, 2017

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

@ioquatix
Copy link
Member Author

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
Copy link
Member Author

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

@thibaudgg
Copy link
Member

@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. 😄

@ioquatix
Copy link
Member Author

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

@thibaudgg
Copy link
Member

@ioquatix done!

@ioquatix
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

ioquatix commented May 8, 2018

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
Copy link
Member Author

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?

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

No branches or pull requests

5 participants