rodjek / librarian-puppet

http://librarian-puppet.com
MIT License
691 stars 209 forks source link

fix bug when using passed in project_dir when using the API directly #302

Closed logicminds closed 9 years ago

logicminds commented 9 years ago

When using the API directly for example:

    e = Librarian::Puppet::Environment.new(:project_path => project_path)

The path is ignored in the DSL because of a bug. (https://github.com/logicminds/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L45)

This is easily fixed by traversing the path method.

specfile.path.kind_of?(Pathname)

Additionally, I attempted to create some tests for this but they are incomplete as I am unsure how to create the test with regards to setting a target. Hoping you can help with this so I can finish the tests and get this PR merge and tested.

  rec = Librarian::Puppet::Receiver.new(target)

Since most people never use the API like this this bug has been masked for a while.

carlossg commented 9 years ago

I have created a test in master da341d5 based on your PR that is succeeding, do you see anything wrong in there?

carlossg commented 9 years ago

IIUC by the time you call Librarian::Dsl::run you need to pass the specfile location, environment.project_dir is no longer applicable

If you share your code I may understand better how you are trying to call it

logicminds commented 9 years ago

Ok here is what I am doing. After reviewing the code some more and your tests the piece I refactored doesn't work in this case and also because Librarian::Specfile.new.kind_of?(Pathname) seems to magically be true and the originally code works as intended. I have no idea why this works since it doesn't extend, include or subclass Pathname.

e = Librarian::Puppet::Environment.new(:project_path => project_path)
Librarian::Puppet::Action::Resolve.new(e, :force => true).run
e.lock.dependencies.map {|d| { d.name => d.source.uri.to_s,
                                   :type => d.source.class,
                                   :version => d.requirement.to_s} }

With regards to Librarian::Puppet::Action::Resolve.new(e, :force => true).run this seems to ignore the project directory. I would prefer not to use this but I don't know the code base that well to understand how to get a list of resolved dependencies without using this method.

logicminds commented 9 years ago

I think I found the real issue but I am not sure. It lies a little bit deeper in the code but its reproducible

This code replaces the specfile that contains the passed in project_dir with the default_specfile. Because this default_specfile is a instance of Proc the following code fails to set the proper working directory which now becomes Dir.pwd. https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L44-L48

So because this is set incorrectly the metadata file cannot be found. https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L61

Your tests didn't cover some of the super class code which is why they were passing. What really needs to be done is environment.dsl(environment.specfile.path, []) which is equilivent to environment.spec

Its seems like the dependent library librarianp would need to set a propertyin this part(https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L44-L48) then the following section https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L61 would need to use this property instead of Dir.pwd as the default.

@carlossg thoughts? I have added a new test case in my tests along with rebasing against master to include your new tests.

You can test this via: bundle exec rspec spec/receiver_spec.rb:28

carlossg commented 9 years ago

Found the issue, and fixed librarianp to make it work, check out the issue-308 branch, you can run the test with

DEBUG=true rspec spec/action/resolve_spec.rb