Fix capture by listening for method definition on Kernel and undefine on Namespace #242

Merged
merged 1 commit into from Aug 9, 2012

Conversation

Projects
None yet
2 participants
Contributor

cgriego commented Jul 20, 2012

Fixes capture method issues when ActiveSupport Kernel extensions are loaded after a namespace is defined (code added before fixes when AS is loaded before the namespace is defined). Given the new use of capture in the deploy:clean task, AS would have to be loaded before the deploy recipe to work with the existing code.

It's ugly but it seems to work to fix capture even if loaded after a namespace is defined. It's based on similar strategies for recreating BasicObject in Ruby 1.8.

Fixes #175 #168 #170 #239 #169

@cgriego cgriego Listen for method definition on Kernel and undefine on Namespace
Fixes capture method issues when ActiveSupport Kernel extensions are
loaded after a namespace is defined.
4ce1550
Contributor

carsomyr commented Jul 26, 2012

What does the snippet return result if self != Kernel do?

Contributor

cgriego commented Jul 26, 2012

It skips the rest of the callback if the method was added to a class other than Kernel since nearly all classes won't have the callback defined.

Contributor

carsomyr commented Jul 28, 2012

Hi, I have a few more niggling questions.

  • Could you add a unit test to verify that a clashing method has been booted when Kernel adds it? Everything compacted into a single commit would be ideal.
  • I read up on Ruby method lookup here. How does Kernel fit into this scheme? Is there an unwritten rule to search it for methods?
  • How does the patch handle the Configuration object? FYI, the top level namespace is of type Configuration.
Contributor

cgriego commented Jul 31, 2012

  1. The case is already tested by the test that was updated in the original commit.
  2. Kernel is a module mixed into Object that provides globalesque methods like puts and raise. http://ruby-doc.org/docs/ProgrammingRuby/html/ref_m_kernel.html
  3. The point of the patch is to ensure namespaces have access to the methods on configuration. Namespace relies on method_missing to forward method calls to Configuration, but if Kernel defines a conflicting method, then to Namespace the method is not missing because Ruby walks the ancestors before declaring it missing, including Kernel.
Contributor

carsomyr commented Jul 31, 2012

Ok, that makes sense. I am aware of the method_missing lookup functionality of Namespace, and now I'm convinced that it is the only affected component. As for the unit tests, what do you mean by "original commit"? I see two unit test lines removed.

Contributor

carsomyr commented Jul 31, 2012

@cgriego From what I can see, removing those two lines of unit test code just masks other problems. The unit test should pass with them in place. That said, I tried removing the explicitly_define_clashing_kernel_methods method and everything passed. Actually, I have no clue what that particular method does and how it can be correct. Do you see any reason to justify its existence, in light of your current fix?

Contributor

cgriego commented Aug 1, 2012

This is the process I went through.

  1. I modified the existing test that was masking the issue that occurred when methods were added to Kernel after the namespace was created by deleting the two lines as you see in the commit.
  2. I ran the tests, observed the following failure:
  1) Failure:
test_kernel_method_clashing_should_not_affect_method_delegation_to_parent(ConfigurationNamespacesDSLTest)
    [./test/configuration/namespace_dsl_test.rb:325:in `test_kernel_method_clashing_should_not_affect_method_delegation_to_parent'
     /Users/cgriego/.rvm/gems/ruby-1.8.7-p358@capistrano/gems/mocha-0.9.12/lib/mocha/integration/test_unit/ruby_version_186_and_above.rb:22:in `__send__'
     /Users/cgriego/.rvm/gems/ruby-1.8.7-p358@capistrano/gems/mocha-0.9.12/lib/mocha/integration/test_unit/ruby_version_186_and_above.rb:22:in `run']:
<"config"> expected but was
<"kernel">.
  1. Then I applied the fix and the tests passed.

Removing the two lines from the existing test exposed the issue. It didn't mask other problem that I can see.

The explicitly_define_clashing_kernel_methods method is still needed in the case where conflicting methods are added to Kernel before the namespace file is required, which is a case that is very difficult to unit test.

Contributor

carsomyr commented Aug 1, 2012

@cgriego I think some topics are being conflated here. Removing those two lines changed the meaning of the unit test, and indeed your code fixed it and a lot more, being general purpose. It does not, however, address the original meaning of the unit test, which seems to fail with your code as an addition. Put those two removed lines back in, you will get the following.

NameError: undefined method `some_weird_method' for class `Capistrano::Configuration::Namespaces::Namespace'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:206:in `method'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:206:in `block in explicitly_define_clashing_kernel_methods'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:205:in `each'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:205:in `explicitly_define_clashing_kernel_methods'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:180:in `initialize'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:29:in `initialize_with_namespaces'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:75:in `new'
    /Users/royliu/Desktop/work/capistrano/lib/capistrano/configuration/namespaces.rb:75:in `namespace'
    test/configuration/namespace_dsl_test.rb:325:in `test_kernel_method_clashing_should_not_affect_method_delegation_to_parent'
    /Users/royliu/Desktop/work/capistrano/vendor/bundle/ruby/1.9.1/gems/mocha-0.9.12/lib/mocha/integration/mini_test/version_201_to_202.rb:27:in `run'

Unless I am mistaken and the original unit test no longer makes sense in light of the pull request changes, I think there's a bad interaction with the explicitly_define_clashing_kernel_methods method.

  1. some_weird_method is added to Kernel.
  2. some_weird_method is undef'd on Namespace. Thus, Namespace no longer responds to some_weird_method, despite it being owned by Kernel.
  3. The Namespace constructor for :clash2 calls explicitly_define_clashing_kernel_methods, which then raises an exception because the namespace doesn't have some_weird_method (see point 2).

So there you have it, a reasonable unit test that fails unexpectedly: Even if the explicitly_define_clashing_kernel_methods method serves a purpose, I still don't think it's correct.

@carsomyr carsomyr added a commit that referenced this pull request Aug 9, 2012

@carsomyr carsomyr Merge pull request #242 from cgriego/capture
Fix capture by listening for method definition on Kernel and undefine on Namespace
b7b204c

@carsomyr carsomyr added a commit that referenced this pull request Aug 9, 2012

@carsomyr carsomyr Reconcile changes from pull request #242 8b3a188

@carsomyr carsomyr merged commit 4ce1550 into capistrano:master Aug 9, 2012

Contributor

carsomyr commented Aug 9, 2012

@cgriego How does 8b3a188 look? Is it a reasonable replacement for the code I removed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment