samvera-labs / hydra_plugins_wg

Work area for the Hydra Plugins Working Group
Apache License 2.0
1 stars 0 forks source link

Adds abort installation if required dependencies are missing justification #54

Closed mbarnett closed 7 years ago

mbarnett commented 7 years ago

resolves #40

afred commented 7 years ago

@mbarnett I was wondering why we added this guideline, and then remembered it's more for non-gem dependencies. So I tried making that a little clearer.

I also think that graceful error handling could be an acceptable alternative to checking a system for all dependencies before hand. Thoughts? @projecthydra-labs/hydra_plugins_wg

mbarnett commented 7 years ago

I like your modifications as far as they go. Writing the justification, this felt really redundant with "print meaningful error messages in case of a failure during installation".

I don't think I recall the context we were thinking of when it came to non-gem dependencies. What did we have in mind for that?

afred commented 7 years ago

I don't recall a specific case. I was probably thinking of non-gem dependencies like FFMPEG or FITS, which are listed in the README, but afaik, there aren't any warnings or errors thrown when they are not installed. You just get errors when Hyrax tries to use them. They bubble up, and you hope that they are clear enough.

The other situation that this guideline might help prevent is...

  1. Hyrax has a non-gem dependency on, say, very_useful_tool.
  2. MyPlugin has a dependency on Hyrax.
  3. MyPlugin also uses very_useful_tool, but doesn't bother ensuring that very_useful_tool is installed, because it assumes it's there by virtue of Hyrax.
  4. Hyrax replaces very_useful_tool with even_better_tool, but it does not consider it a major release, because it's properly following Liskov's substitution principle.
  5. MyPlugin now breaks when upgrading to the latest point release of Hyrax, because it still depends on very_useful_tool, but the installation of Hyrax no longer guarantees it's presence on the host.

I think another way to look at it would be... Bundler works great for gems, but there is no Bundler for non-gem dependencies. So instead of getting surprised by missing non-gem dependencies, check to make sure they're all there first.

IDK.. if it feels to redundant.. or if the same problem can be solved by a combination of other guidelines.. i'm fine with leaving it off.

afred commented 7 years ago

Or... if this is too much of a burden to do check for non-gem dependencies during installation in addition to following the exception handling guidelines (which I think is more important), perhaps the guideline could be changed to say something like...

A plugin must document all dependencies in it's README that will not be installed by Bundler.

mbarnett commented 7 years ago

IDK.. if it feels to redundant.. or if the same problem can be solved by a combination of other guidelines.. i'm fine with leaving it off.

No, I think you've identified a good use-case for it. I wonder if we shouldn't just spell it out a little more. Something like:

Justification: Non-gemspec dependencies, like FFMPEG, are by their nature hard to enforce the presence of. By detecting their absence and failing fast, rather than later on during Application startup (or worse, during the runtime of a request) a plugin makes it easier to catch and resolve errors. Plugin usage becomes much more maintainable for the implementer when plugins can be relied upon to signal errors as early as possible, rather than failing in hard-to-predict ways at runtime.

An acceptable alternative to aborting installation of the plugin, would be to ensure graceful failure in the absence of required dependencies.

mbarnett commented 7 years ago

Pushed that change

afred commented 7 years ago

This has been obviated by #73.

Per feedback discussion at Hydra Developer Congress (March 30-31, 2017 at Stanford), ticket #71 was filed to get rid of the "abort installation" guideline altogether for two primary reasons:

  1. keep development environments flexible for the plugin implementers. They shouldn't have to install all dependencies on their dev machines. Instead, the expectation shoudl be that their application (which uses the plugin) is sufficiently tested, including with all required non-gem dependencies, before going into production.
  2. If plugin developers are following the guidelines around providing useful exception handling, then that should be sufficient for plugin implementers to catch any unmet non-gem dependencies in their own test suites, however they choose to set it up.