rubygems / rfcs

RubyGems + Bundler RFCs
45 stars 40 forks source link

Add extended error management RFC #17

Closed jules2689 closed 3 years ago

jules2689 commented 5 years ago

The https://github.com/jules2689/extended_bundler-errors plugin can help all Ruby developers who use Bundler. Instead of hiding it in a plugin that no one knows how to install, let's make it a core feature.

This will introduce extended error management in Bundler.

Rendered

indirect commented 5 years ago

I really like this as a project. It fits in extremely well with Bundler's long-term philosophy of "error messages should tell you what to do to fix them", and improves a long-standing pain point for all Bundler users.

My primary question is around the handlers—as described in this RFC, it seems like:

1) the Bundler team will have to take on all the work of mapping compiler errors to explanations, and 2) the error mappings will be static in every released version of Bundler

Those things aren't necessarily showstoppers, but it seems like we might be able to brainstorm ways to improve those things before agreeing to add this to core.

As for adding a plugin hook directly to handle this particular use-case, that seems completely reasonable and like something we can just do right away without an RFC process. :+1:

indirect commented 5 years ago

One other possibility is that we could keep the plugin itself outside of core to address the issues above, but add a message if any gem fails to build that says something like “Your gem failed to build! Try installing the compile-help plugin to get suggestions about how to fix failing gems builds”.

skalee commented 5 years ago

I understand that differently: That error handlers are defined by gem developers, they are contained in these gems (under the glob lib/extended_bundler/handlers/*.yml relative to gemspec), and confined to these gems.

There are few things I disagree with though:

  1. Handlers are placed under /lib. That looks odd to me. Is there any reason not to use bundler/error_handlers or gem/installation_error_handlers or something instead?
  2. I don't see a reason for the versions key in handler definition. IMHO only relevant versions of given gem should contain the handler, and that's all. However, ruby_versions could be added.
  3. I am not sure if it breaks compatibility with previous Bundler versions in any way or not. If yes, then perhaps this feature could be enabled/disabled from gemspec's metadata. For example, spec.metadata = {bundler_errors: true}.
  4. But most importantly, I think that it should be a RubyGems feature rather than Bundler's.
jules2689 commented 5 years ago

@indirect

the Bundler team will have to take on all the work of mapping compiler errors to explanations, and the error mappings will be static in every released version of Bundler

yea, that's one of the key points for discussion here. My intention is to have a collection of handlers up front and allow overriding via plugin, gems inclusion of handlers, etc.

@skalee

That error handlers are defined by gem developers

This is not the current design at all. I also don't want it to be the only design. A good case study is that you can include pre-compiled gem extensions, yet nothing does (e.g. nokogiri). If gem developers are expected to add these, we might as well not add this feature whatsoever as I doubt anyone will take the time to add these errors.

Handlers are placed under /lib. That looks odd to me. Is there any reason not to use bundler/error_handlers or gem/installation_error_handlers or something instead?

This was the easiest location in a plugin gem. I wouldn't put too much emphasis on locations in the RFC as that is more of an implementation detail I see for PR review, but I think what you're saying makes sense.

I don't see a reason for the versions key in handler definition. IMHO only relevant versions of given gem should contain the handler, and that's all. However, ruby_versions could be added.

Gems will not be the only thing adding (if they even add them at all) handlers. As such, the handler must dictate the versions they match against. To note, as this was a plugin gem before, it is required to specify versions.

I am not sure if it breaks compatibility with previous Bundler versions in any way or not. If yes, then perhaps this feature could be enabled/disabled from gemspec's metadata. For example, spec.metadata = {bundler_errors: true}.

I'll keep this in mind. Thanks!

I think that it should be a RubyGems feature rather than Bundler's.

I want to dive into this a bit more. Can you explain why you feel this? I've always felt Rubygems was more of a "get it on your system no fancy stuff"

skalee commented 5 years ago

yea, that's one of the key points for discussion here. My intention is to have a collection of handlers up front and allow overriding via plugin, gems inclusion of handlers, etc.

Apparently I have misunderstood this idea. Now I totally agree with @indirect. With thousands of gems with native extensions which are uploaded to Rubygems, maintaining a reliable list of handlers sounds like a Sisyphean task. A set of handlers defined in the plugin you are referencing to is quite short, but I suppose it is far from being complete.

I think that a likely source of constant issues should not become a core feature of Bundler. Perhaps the plugin should be popularized instead.

This is not the current design at all. I also don't want it to be the only design. A good case study is that you can include pre-compiled gem extensions, yet nothing does (e.g. nokogiri). If gem developers are expected to add these, we might as well not add this feature whatsoever as I doubt anyone will take the time to add these errors.

I suppose it will be true for the least popular gems, but IMHO it is the only manageable solution (apart from keeping it all in plugin). Also, perhaps some common handlers can be extracted e.g. to another gem, so that gem developers can re-use them.

Gems will not be the only thing adding (if they even add them at all) handlers. As such, the handler must dictate the versions they match against. To note, as this was a plugin gem before, it is required to specify versions.

Got it. I stated this opinion because I misunderstood your suggestions.

I want to dive into this a bit more. Can you explain why you feel this? I've always felt Rubygems was more of a "get it on your system no fancy stuff"

Now when I understand what you mean, I agree that it should not be implemented in RubyGems.

jules2689 commented 5 years ago

@skalee Thanks for the feedback!

I think in an ideal world I agree that a plugin continues to make sense, but I don't think that's feasible. This would require people to install the plugin, or they don't get the benefit. Which I really think we won't get to happen.

Instead, I think it makes sense to include the core parts of the plugin in core and do the following:

To note, I also want to overhaul the plugin system as I have likely the most experience using it in a "production" setting and noticed a lot of calluses. @indirect has asked me to write up a post about that, and I do intend to, I just haven't gotten to it yet. This would touch on discoverability.

WDYT?

colby-swandale commented 5 years ago

From a maintainers point of view, I'm worried about having to maintain these sets of documents (and them becoming a chore for maintainers) to make sure they're correct for users. The set of documents would also become fairly coupled to the bundler version, and soon to be, rubygems version. (if we want to fix or add a new bundler extended error, we would have to release a new bundler version)

What if instead of accept the plugin as part of the bundler org and look into fixing these mentioned plugin issues?

indirect commented 5 years ago

I would definitely like to improve the plugin system based on feedback regardless of whether we merge the errors directly into Bundler or not. 👍

I agree with Colby's concerns about hard-coded explanations (which might become wrong over time). Maybe we could provide an HTTP API that accepts the name and version of the gem, and returns a list of possible compilation helper messages? In that case, Bundler would not need to be updated to provide updated messages. If gems start supplying their own help, we could even have RubyGems.org add the messages from gems to the list when they are gem pushed.

It also has the benefit of potentially being something we could evangelize for gems to provide themselves, hopefully meaning that the Bundler/RubyGems team wouldn't have to maintain the messages themselves. 😅

jules2689 commented 5 years ago

That sounds like a good plan. I was trying to avoid the whole maintain a service thing haha

I wonder, if an HTTP index provides the content, then perhaps we can still make the plugin a part of core. One of the issues of the plugin system is discoverability, which I am concerned can't really be fixed without significant effort. <<< Still planning on writing that page up!

indirect commented 5 years ago

@jules2689 I don't super want to maintain another service, but I think we could provide the messages from a RubyGems.org API, especially if one of the sources is the gems themselves. 👍🏻

If the messages themselves are coming from a service, I think it makes sense to add this to core—one of the core principles of Bundler has always been that any user being confused or not knowing what to do is a bug that we should try to resolve. I think that better error messages for compilation fits in really well with that overall goal, and would be great to have.

jules2689 commented 5 years ago

That makes sense @indirect ... I'll think of an API we can add and propose a solution or two. Somewhat of a side note, but is there a documentation explaining the separation of concerns between RubyGems, Bundler, Rubygem.org, etc? It's not super clear to me anymore.

indirect commented 5 years ago

@jules2689 I'm not sure if there's anything written down explicitly, but the very short version is:

RubyGems.org: provides 1) a list of all gems, 2) gemspecs, and 3) the .gem files. There are a few different formats for that data, but that's the gist.

RubyGems: provides 1) the gem command, 2) a monkeypatch to require that knows how to find installed gems, 3) the Gem class and everything underneath that.

Bundler: provides 1) the bundle command, and 2) the bundler/setup Ruby file that monkeypatches require (and a lot of other parts of RubyGems) to limit it just to the single application bundle, and 3) the Bundle class and everything underneath that.

As you can maybe imagine, Bundler depends on lots of RubyGems internals, and more recently we have been starting to consolidate places where RubyGems and Bundler have two different classes to do the same thing with a single class that both of them use.

I hope that helps!

segiddins commented 5 years ago

If gems start supplying their own help, we could even have RubyGems.org add the messages from gems to the list when they are gem pushed.

Could they provide this in their metadata, so nothing needs to depend on a service?

indirect commented 5 years ago

Yeah, I'm basically imagining the "HTTP service" as either an existing or new RubyGems.org API endpoint that returns something provided by the gem. If it's an official part of RubyGems.org, we can provide the current error helpers to the upstream gems for publication in their own .gem packages.

jules2689 commented 5 years ago

https://github.com/bundler/rfcs/pull/17#issuecomment-487328746

@indirect That definitely did help.

Could they provide this in their metadata, so nothing needs to depend on a service?

@segiddins I don't want this to be the only way as I see this going about as well as people adding pre-compiled system extensions and the like. When it's optional, it doesn't happen. And when it's in the gemspec, it cannot be retroactively applied to old gem versions. So if someone realizes they introduced something that needs error handling after they've submitted, they'd be out of luck.

"HTTP service" as either an existing or new RubyGems.org API

I like this idea.

jules2689 commented 5 years ago

How about this for where the errors go:

  1. We add a declaration for handlers to gemspecs (there may need to be validation around those and some thought put in around how to implement a handler)
  2. We add a plugin hook for error messages as well, this allows a plugin to add error messages for other gems that won't add the data to their specs or are already on rubygems
  3. We can create a plugin that has an "index" of errors, and in bundler core, on error, simply suggest that installing that plugin may provide some benefit on providing more context these kinds errors for some cases

This allows gemspecs to handle things where they can, but also provides us an avenue to add an index of errors for other cases where the version is already published or the authors won't add the content to their specs. It also addresses concerns around plugins not being known because it directly tells people to install it

This comes with the added benefit of advertising the plugin system. It might be better to implement https://github.com/bundler/bundler/issues/7152#issuecomment-494588187 first before mass pushing people to plugins though 😅

We may also want to implement this directly in Rubygems, which I feel is better left to the core team to decide as they know that part of the system better than I do

indirect commented 5 years ago

I think what @jules2689 suggests above is a good way to go if we decide to implement this inside Bundler.

That said, I think I'm leaning towards improving the error messages and output in RubyGems, since then both gem install and bundle install could theoretically benefit from the improved error messages. @bundler/maintainers any input here on adding extended error messages and compilation help to Bundler vs to RubyGems?

jules2689 commented 5 years ago

Rubygems has plugins too, IIRC, @indirect ?

deivid-rodriguez commented 5 years ago

I agree that it makes more sense to try to add this to rubygems, since the extended error management is targeted to the installation of specific gems (not to other more aggregated kind of errors), and bundler just delegates to rubygems the actual installation of specific gems.

I think you might be able to use "rubgems hooks" for this? See https://github.com/rubygems/rubygems/blob/bc6d07d0d9690003c86664b76fb0d8eee5206492/lib/rubygems.rb#L777-L847.

jules2689 commented 3 years ago

I dont intend to pick this back up. Could be a cool project one day though!