Set $LOAD_PATH instead of puppet's libdir

[joshcooper]

Commit 7f7e74f set puppet's libdir to include the lib directory for
each module in the modulepath. Doing so enabled puppet's autoloader to
load modules that hadn't been pluginsynced. However, this only worked for
code that the autoloader directly loaded. It didn't work when the
autoloaded type, provider, etc required helper code in the same module,
or different module, because the module's lib directory was not in ruby's
$LOAD_PATH.

The commit also relied on inconsistent behavior in puppet whereby sometimes
puppet assumed libdir was a (semi)colon separated list of directories, and
sometimes it assumed a single directory. In Puppet 4.0, puppet will
only interpret the libdir setting as a single directory[1].

As a result, this commit stops setting libdir and instead updates the
ruby $LOAD_PATH. In doing so, rspec-puppet is the "application" the uses
puppet as a "library". In that pattern, rspec-puppet is the thing that
sets the ruby $LOAD_PATH, and puppet just uses the specified $LOAD_PATH.
This is similar to how we fixed loading faces from modules[2] in commit[3].

[1] https://tickets.puppetlabs.com/browse/PUP-3336
[2] https://projects.puppetlabs.com/issues/7316
[3] puppetlabs/puppet@4f99f25cd

[joshcooper]

/cc @richardc, @fatmcgav, @jhoblitt, @domcleal

[jhoblitt]

In and of itself, this doesn't fix the rspec-puppet-augeas breakage but it's a step in the right direction. Example of testing this branch in a puppet module that uses rspec-puppet-augeas: https://travis-ci.org/jhoblitt/puppet-autofsck/builds/42829734

[joshcooper]

Thanks @jhoblitt! It looks like the test failures are due to puppet trying to "use" some of its settings while executing the tests -- so puppet creates the settings catalog, tries to apply it, and manage the file/directory permissions:

Got 1 failure(s) while initializing: File[/etc/puppet]: change from absent to directory failed: Could not set 'directory' on ensure: Permission denied - /etc/puppet

In puppet's specs we explicitly set confdir and vardir to a temp directory https://github.com/puppetlabs/puppet/blob/master/spec/spec_helper.rb#L128-L132. Partly so that state from one test doesn't pollute another test.

But I'm wondering if something changed in puppet to make it so that modules must override confdir & vardir?

[domcleal]

I think using the future parser/evaluator was the change that triggers it, as it causes a run through the Puppet settings code that I don't think happens in a current parser configuration. I didn't dig much further.

https://github.com/rodjek/rspec-puppet/blob/v1.0.1/lib/rspec-puppet/support.rb#L104-L105 is similar to what you suggest @joshcooper, as it configures a temporary directory for vardir. Perhaps we should extend this to match Puppet's tests?

Regarding this PR, it looks correct 👍

[joshcooper]

@domcleal doing the same for confdir makes sense to me.

@hlindberg wondering if something in the future parser/evaluator might be causing puppet to manage settings in a way that the old didn't?

[hlindberg]

One difference when using the future parser is that it needs the environment's module path much earlier, and does a scan of bindings to load from modules. It does not directly do anything with settings, but may cause things to happen in a slightly different order. The bindings system has some code that looks at confdir expecting it to be there (it is searching for a file IIRC), the file does not have to exist, but it may not like if there there is no confdir).

[domcleal]

Apologies, seems I was muddled and it wasn't the future parser. It seems that something between rspec-puppet 0.1.6 and 1.0.0 was causing the change, but I haven't tried to determine exactly what. Since rspec-puppet-augeas was pinned to < 1.0.0 I guess I hadn't seen it until now. I'll handle this in r-p-a as it's fairly unique to that gem (which applies a real catalog), not rspec-puppet.

[fatmcgav]

Ok, I've tried this fix on my rspec-deptest module that I wrote to prove PUP-3336, and I'm still seeing the same issue...

Latest Travis build is: https://travis-ci.org/fatmcgav/rspec-deptest/builds/42972251 where I've removed the spec_helper.rb work-around and switched to use commit 311ac19, and it's still failing with the same error.

I've added a puts $LOAD_PATH to the top of my spec file, and the module paths aren't being added to the $LOAD_PATH unfortunately :(

Let me know if can help further, or if I'm doing something wrong with my tests...

Cheers
Gav

[joshcooper]

@fatmcgav thanks for testing this out, the deptest module is pretty handy! And it showed I had a bug, I wasn't actually enumerating over the Dir entries. I updated the PR with the proper fix and tests.

That said, it still fails with your module, because the RSpec::Puppet::Support#setup_puppet method is never being invoked when I execute your module's tests. For example, the travis-ci job shows /dev/null/lib in the $LOAD_PATH, which is because puppet defaults vardir to /dev/null (so setup_puppet never changed vardir to the tempdir).

If I change the test to explicitly call setup_puppet:

  it {
    puts "LOAD_PATH: " + $LOAD_PATH.select {|d| d =~ /rspec-deptest\/spec\/fixtures/}.join(':')
    setup_puppet
    puts "LOAD_PATH: " + $LOAD_PATH.select {|d| d =~ /rspec-deptest\/spec\/fixtures/}.join(':')
    should compile
  }

Then the modules in the fixtures directory are correctly added to the $LOAD_PATH and tests pass:

$ bundle exec rake test
---> syntax:manifests
---> syntax:templates
---> syntax:hiera:yaml
librarian-puppet install --path=spec/fixtures/modules/
/Users/josh/.rbenv/versions/1.9.3-p545/bin/ruby -S rspec spec/classes/init_spec.rb --color
LOAD_PATH:
LOAD_PATH: /Users/josh/work/rspec-deptest/spec/fixtures/modules/glassfish/lib:/Users/josh/work/rspec-deptest/spec/fixtures/modules/stdlib/lib
.
Finished in 0.90834 seconds
1 example, 0 failures

I'm not sure why setup_puppet isn't getting called though...

[fatmcgav]

@joshcooper Just came back to this one...

Any more thoughts?

[joshcooper]

@fatmcgav thanks for the ping. I think we need to figure out why RSpec::Puppet::Support#setup_puppet is never getting called. I would expect rspec-puppet to call it automatically as part of test framework setup so that modules don't need to, but I'm not sure about that.

Since setup_puppet is never called, many things are not configured correctly, e.g. vardir is set to /dev/null, in addition to the specific issue of $LOAD_PATH not being updated. Once we call setup_puppet things should just work.

[DavidS]

Yeah, I've rebased and tested this locally and it works fine for me.

[joshcooper]

@hunner @DavidS I've updated the PR to resolve the conflict. I updated the setup_puppet method so that it allows modulepath to be specified, which is probably not the right thing for Puppet 4. Alternatively, I could something like the following, though I'm not sure what basedir should be in rspec-puppet?

basedir = Puppet.version.to_f >= 4.0 ? Puppet[:environmentpath] : Puppet[:modulepath]
Dir["#{basedir}/*/lib"].entries.each do |lib|
  $LOAD_PATH << lib
end

[DavidS]

Most important for rspec-puppet, as I'm currently using it, is to have the containing module's lib/ in the load path.

[coveralls]

Changes Unknown when pulling d427883 on joshcooper:dont-set-libdir into * on rodjek:master*.