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

Plugins exception #28

Merged
merged 2 commits into from
Aug 12, 2014
Merged

Plugins exception #28

merged 2 commits into from
Aug 12, 2014

Conversation

fabiopelosin
Copy link
Member

To reasonably desume the plugins from the backtrace it is necessary to determine the root of the gem of the plugin. For this reason this patch changes the convention that the require file should be under the ./lib dir (e.g. ./lib/cocoapods-plugin.rb). This is coherent with RubyGems conventions and I don't expect related issues thus.

/c @alloy

Closes #4.

@fabiopelosin
Copy link
Member Author

The logic needs to be fixed be fixed because the plugins message is always printed even if there are no exceptions.

@alloy
Copy link
Member

alloy commented May 19, 2014

Can you paste an example of the output? Because I don’t know for sure if I understand everything this is about correctly.

@fabiopelosin
Copy link
Member Author

The output just prints that some plugins might be the cause of the crash, my question was about wether it is safe to assume the the plugins loader file should be in lib (like ./lib/cocoapods-plugin.rb)

@alloy
Copy link
Member

alloy commented May 19, 2014

my question was about wether it is safe to assume the the plugins loader file should be in lib (like ./lib/cocoapods-plugin.rb)

Instead of?

All ruby files in a gem should normally be in the lib dir, so I think that’s a safe assumption.

@fabiopelosin
Copy link
Member Author

Instead of ./lib/**/cocoapods-plugin.rb (i.e. I mean that we assumed that they are a direct child of lib)

@fabiopelosin
Copy link
Member Author

ping

@alloy
Copy link
Member

alloy commented May 26, 2014

I don’t know if there is a reason to not do this. Have you looked into other conventions that also use the RubyGems find files system?

@fabiopelosin
Copy link
Member Author

I haven't looked into other conventions (not sure about which examples to look at). I think that it is a reasonable requirement. The only concern is wether some existing plugins might break because they don't respect it. However I don't think that this would be the case because they should have been bootstrapped from the code of the original examples.

@alloy
Copy link
Member

alloy commented May 26, 2014

I think Rails plugins have a convention, which is to add an init file outside of lib. And otherwise the RubyGems CLI plugins should provide a good example of what works best for RubyGems.

@fabiopelosin
Copy link
Member Author

As of RubyGems 1.3.2, RubyGems will load plugins installed in gems or $LOAD_PATH. Plugins must be named ‘rubygems_plugin’ (.rb, .so, etc) and placed at the root of your gem’s #require_path. Plugins are discovered via Gem::find_files then loaded. Take care when implementing a plugin as your plugin file may be loaded multiple times if multiple versions of your gem are installed.

from http://guides.rubygems.org/plugins

Indeed it does. However, I don't think that there any concrete advantages over our current approach (other than elegance) and thus I would prefer to keep our convention in place because changing at this point is not worth it.

To sum up, the question of this pull request is:

  • Currently the plugin loader file can be specified anywhere in the tree descending from the ./lib dir of the gem.
  • With this approach identifying the root of the gem requires heuristics (like traversing the path upward looking for a gemspec file).
  • However all the plugins that we have developed (and likely the others from third parties) store the plugin loader file in the ./lib dir.
  • This standardisation allows to easily identify the root of the gem (required for checking backtraces) and leads to less ambiguity.

@segiddins
Copy link
Member

ping

#
def self.load_plugins(plugin_prefix)
return if plugin_paths
Copy link
Member

Choose a reason for hiding this comment

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

@plugin_paths ||= begin...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend not to use this syntax because I have observed issues with the automatic alignment of vim.

@segiddins
Copy link
Member

@irrationalfab any update on this PR?

@fabiopelosin
Copy link
Member Author

I have rebased the branch. I think that standardizing that plugins should include the plugin loader (cocoapods-plugin.rb) in ./lib (and not in any of its subfolders) is good and makes logic simpler in general.

I don't think that it should create any issue with existing plugins because all our plugins and the templates already adhere to this convention. Given the lack of feedback I'm letting the to @CocoaPods/core 24h hours to oppose to this patch and after I will merge it.

@alloy
Copy link
Member

alloy commented Aug 12, 2014

👍

fabiopelosin added a commit that referenced this pull request Aug 12, 2014
@fabiopelosin fabiopelosin merged commit 303b960 into master Aug 12, 2014
@fabiopelosin fabiopelosin deleted the irrationalfab/plugins-exception branch August 12, 2014 14:13
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.

Report differently crashes caused by plug-ins
3 participants