The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com
https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:
Add pkgutil provider
|Affected Puppet version:||0.25.5||Branch:||https://github.com/domcleal/puppet/tree/tickets/master/4258-dev|
Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com
#2 Updated by Maciej Bliziński Maciej almost 6 years ago
There are existing resources:
- Patches in the OpenCSW codebase: https://gar.svn.sourceforge.net/svnroot/gar/csw/mgar/pkg/puppet/trunk/files/
- My puppet branch on github: http://github.com/automatthias/puppet/
- A page on pkgutil’s wiki page: http://pkgutil.wikidot.com/puppetprovider
There are 2 versions of the patch in existence: Gary Law’s and mine. The main difference between the two is that mine uses the —single option of pkgutil, which speeds up the operation a great deal.
The current pkgutil provider has a problem in which it always thinks that the state of every package has changed:
puppetd: [ID 702911 local7.notice] (//package::solaris_pkg/Package[cups]/ensure) ensure changed ‘catalog’ to ‘integrity’
Since it always thinks that the cups package has changed state, it restarts the daemon at every puppet run, while it really doesn’t have to; I would also apply to, let’s say, Apache, and we don’t want the web server being restarted every time puppet daemon is run.
I’ve been meaning to correct that, but the current provider code is hard to work with; I’ve poured a good few hours into it and gotten essentially nowhere. The factory class methods are confusing, and there’s no clarity about the interface. I’ve tried to work it out by reading other providers. The problem with this approach is that there’s no clear picture emerging as to which methods are just private utility methods of a given provider, which are actually required by the interface, and which are optional (it looks like different providers have different capabilities). I know pkgutil pretty well and I’m willing to contribute, but I need guidance as to what are the required methods of the provider interface, what arguments they expect and what are the required return data structures.
#8 Updated by Rudy Gevaert over 5 years ago
- File pkgutil.rb.patch added
I gave the testing a try. Luckily I could just download the provider pkgutil.rb file.
The patch on http://github.com/automatthias/puppet/commits/master/ doesn’t work on my system.
The output of pkgutil on my system is different than the output that is expected by the provider.
I adjusted the provider the be able to parse the output pkgutil is giving me:
info: Applying configuration version '1288631027' debug: Puppet::Type::Package::ProviderPkgutil: Executing '/opt/csw/bin/pkgutil -c --single subversion' warning: CSWsvn 1.4.5,REV=2007.11.18 SAME warning: 1:CSWsvn warning: 2:1.4.5,REV=2007.11.18 warning: 3:SAME debug: Puppet::Type::Package::ProviderPkgutil: Executing '/opt/csw/bin/pkgutil -c --single emacs' warning: CSWemacs notinst 22.1 warning: 1:CSWemacs warning: 2:notinst warning: 3:22.1 debug: Puppet::Type::Package::ProviderPkgutil: Executing '/opt/csw/bin/pkgutil -y -i emacs' notice: /Stage[main]//Node[zonnebloem]/Package[emacs]/ensure: created debug: Finishing transaction 75266800
To be clear, the warnings are just output added by me. The first line is the output produced by calling the pkgutil command. Line 1,2,3 are the matched regular expressions.
You can see that my patched provider installed emacs without a problem.
Attached you can find the changed I made.
bash-3.00# pkgutil --version 2.1
#11 Updated by Dominic Cleal over 5 years ago
- File pkgutil_4258_gpg.patch added
I agree with Rudy – the versions of pkgutil from sourceforge do output “notinst” for packages that aren’t installed and so the patch is required.
In addition, the “justme” mode fails when use_gpg is enabled with signed catalogs as both pkgutil and gpg output lines before the package information line. pkgutil.rb parses these and returns the first line, rather than the package info.
The attached patch ignores these lines while parsing and supercedes Rudy’s patch.
#13 Updated by Dominic Cleal over 5 years ago
Thanks Rudy. I’ve since put it on a fork of Maciej’s git branch, applied your patch, tidied up my use_gpg fix and added a fix for catalog updates. The latter you’ll notice if pkgutil’s catalogs are due to be refreshed, which you can force with catalog_update = 0 in pkgutil.conf. This generates some logging from pkgutil itself, plus wget output (run with —no-verbose or -q in wgetopts).
My repo: https://github.com/domcleal/puppet/tree/master
pkgutil.rb itself: https://github.com/domcleal/puppet/blob/143fc744a839affd328234fca26246d49d15d3d8/lib/puppet/provider/package/pkgutil.rb
#15 Updated by Dominic Cleal over 5 years ago
I’ve made a few further tidyups (naming, indentation, comments), pulled in fixes from Rudy’s github branch and tried to make the single package query mode a little more robust in the face of pkgutil noise.
To get the ball rolling, I’ve mailed the patch to puppet-dev so any further work should be discussed in this thread: http://groups.google.com/group/puppet-dev/browse_thread/thread/f5da6ae1f0cef88d
#17 Updated by Juerg Walz about 5 years ago
I’ve got a few updates (probably due to a newer version of pkgutil): proper handling of “Not in catalog”, detection of “SAME” version and to use the “-u” argument for updates.
My patch would be for the version of the provider found under: https://github.com/domcleal/puppet/raw/143fc744a839affd328234fca26246d49d15d3d8/lib/puppet/provider/package/pkgutil.rb … which was the most up-to-date version I could find.
Where do I need to send my patch to? Who’s maintaining the “official” provider, which hopefully ends up in the official Puppet distribution?
#18 Updated by Dominic Cleal about 5 years ago
I’d suggest cloning my git repo from that URL, committing your changes and then using “rake mail_patches” to send the changes to the puppet-dev list. There’s an overview of the whole development process here you should review: http://projects.puppetlabs.com/projects/puppet/wiki/Development_Development_Lifecycle
If that sounds a bit much, feel free to either post your own clone on github (or somewhere) with your extra commit(s) and I’ll be happy to review and test it against the pkgutil 2.2b1 version used here. I notice that 2.3 has been released with —parse, is this what you’re planning to use?
Having just got the hang of the spec tests, I might try and write one for this new provider.
#24 Updated by Juerg Walz about 5 years ago
I’ve changed the handling of short package names (w/o CSW, or any prefix). Checkout the latest commit in my branch.
The idea is to be able to use the short package names in the Puppet manifests (ex. “ruby” instead of “CSWruby”) to avoid OS-specific package names. This really helps if you maintain multiple platforms.
Noob question: should I run “rake mail_patches” for small changes like this?
#25 Updated by Jacob Helwig about 5 years ago
Yeah, you should send the updated version to the mailing list. Please also point out in the cover-letter that it’s the v2 (or whatever version you’re up to) of the patch-series. Big bonus points if you also include the diff of what’s changed since the last version of the patch series. Thanks for putting in the time to work on this.
#28 Updated by Dominic Cleal about 5 years ago
Updated method to handle package aliases (i.e. “subversion” is “CSWsvn”) by calling pkgutil -a to translate between the two. This means we no longer need to call out to -c —single for each package resource that uses an alias instead of the full name. The spec’s been updated to test various aspects of this.
There’s been a discussion on -users about making this versionable too, but we’ll need to gather some catalog and pkgutil output samples if we want to try and do this.
#29 Updated by Juerg Walz about 5 years ago
I’ve tested your changes, Dominic. There was an issue with the hashes created for “aliases”. I’ve send through a small change in the way the original hash gets “cloned”.
Otherwise, your new version works great. I’ve ran a few tests (with aliases + Solaris pkg name).
#30 Updated by Juerg Walz about 5 years ago
Where would be the best place to put a note that the pkgutil.conf should have “wgetopts=-q” set?
It works without it, but I got “Couldn’t match” warnings when a pkgutil catalogue update was due. There are empty lines in the output (which, strangely enough, I couldn’t match in the “noise” function with something like /\s*$/ …). Anyway, using the ‘quiet’ option of wget is the better solution.
#31 Updated by Dominic Cleal about 5 years ago
I’d suggest using the -nv option, rather than -q as the quiet mode will also hide error messages. Still, I don’t know where we can record the information – perhaps we could do something a bit nasty and check the user’s pkgutil.conf file to warn if it isn’t set. Any better ideas?
#35 Updated by Matt Robinson about 5 years ago
- Assignee set to Matt Robinson
I’m going to be reviewing this for inclusion into the main codebase today. I know there’s been a lot of work, and a lot of discussion which is great. However, I’m definitely not a very advanced Solaris user and all the back and forth on this may take a while to review. I’m inclined to do a fairly light review and get this merged in and then issues that may still be present can be addressed in other tickets. Does that sound reasonable to those involved?
From what I understand the most recent code that I should be looking at to merge is at
#37 Updated by Dominic Cleal about 5 years ago
Great, thanks Matt. I’ll review Rudy’s latest change later if I can too.
If we can get it into the codebase now that’d be good as I think we all consider it stable, then perhaps we can look at versionable support (as requested on -users) as an incremental improvement.
#38 Updated by Matt Robinson about 5 years ago
I haven’t been able to merge this because the tests don’t pass. It looks like the last commit in the series introduces an assumption that a file will exist in /etc/opt/csw/pkgutil.conf. The code is in the provider in a way that it’s not possible to stub since this file existence assumption is in place outside of any method calls in the provider.
─ % rspec spec/unit/provider/package/pkgutil_spec.rb /Users/matthewrobinson/work/puppet/spec/../lib/puppet/util/autoload.rb:87:in `load': Could not autoload package: Could not autoload /Users/matthewrobinson/work/puppet/spec/../lib/puppet/provider/package/pkgutil.rb: No such file or directory - /etc/opt/csw/pkgutil.conf (Puppet::Error) from /Users/matthewrobinson/work/puppet/spec/../lib/puppet/util/autoload.rb:75:in `each' from /Users/matthewrobinson/work/puppet/spec/../lib/puppet/util/autoload.rb:75:in `load' from /Users/matthewrobinson/work/puppet/spec/../lib/puppet/metatype/manager.rb:124:in `type' from /Users/matthewrobinson/work/puppet/spec/unit/provider/package/pkgutil_spec.rb:5 from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/configuration.rb:386:in `load' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/configuration.rb:386:in `load_spec_files' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/configuration.rb:386:in `map' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/configuration.rb:386:in `load_spec_files' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/command_line.rb:18:in `run' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/runner.rb:55:in `run_in_process' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/runner.rb:46:in `run' from /Library/Ruby/Gems/1.8/gems/rspec-core-2.5.1/lib/rspec/core/runner.rb:10:in `autorun' from /usr/bin/rspec:19
Also very odd is that when I tried to merge or rebase this branch, I got merge conflicts even though all the changes should be to the same file. It looks like there may have been some cherry picking of patches out of order? For example this patch series doesn’t make sense:
- apkg = pkg + apkg = Hash.new(pkg) - apkg = Hash.new(pkg) + apkg = pkg.dup - apkg = Hash.new(pkg) + apkg = pkg
I think I can resolve it all correctly with Hash.new being the desired state at the end, but it’s weird that this branch has merge conflicts present within itself. I’ve not seen that before.
I’ll see if I can figure out a good way to get the test passing on this tomorrow to get it into Statler before we do an RC, but it may not happen.
#39 Updated by Dominic Cleal about 5 years ago
Thanks for the feedback Matt.
I’ve moved the new pkgutil.conf test inside a “healthcheck” method, which now contains the tests that were in the extended method. From what I can tell, they were never being run to begin with (having been copied from blastwave.rb)! This method is then called from the prefetch and is stubbed out in the spec.
The merge conflict looks like a problem on Juerg’s branch as he’s fixed the same issue as me, then merged in the same commit from my branch.
The above commits, along with Juerg’s to add the pkgutil.conf check have been pushed to my own branch (https://github.com/domcleal/puppet/tree/tickets/master/4258-dev) and I’ve verified it merges into next.
#40 Updated by Matt Robinson about 5 years ago
Thanks Dominic. I still get a test failure:
Failures: 1) Puppet::Type::Package::ProviderPkgutil when installing should use a command without versioned package Failure/Error: @resource[:ensure] = :latest Puppet::Error: Parameter ensure failed: Provider must have features 'upgradeable' to set 'ensure' to 'latest' # ./lib/puppet/parameter.rb:171:in `fail' # ./lib/puppet/parameter.rb:257:in `validate' # ./lib/puppet/property.rb:300:in `should=' # ./lib/puppet/property.rb:300:in `each' # ./lib/puppet/property.rb:300:in `should=' # ./lib/puppet/property.rb:337:in `value=' # ./lib/puppet/type.rb:416:in `=' # ./spec/unit/provider/package/pkgutil_spec.rb:35
#41 Updated by Dominic Cleal about 5 years ago
I can’t reproduce this Matt – I’ve done a pull of the ‘next’ branch from the puppetlabs repo on github and merged in 4258-dev and also tried with puppetlabs' master branch.
I don’t fully understand how the features are determined, but looking at the package type it looks like it introspects the provider for certain methods (latest and update). Does your branch have different requirements for the upgradeable feature? The pkgutil provider has both latest and update methods implemented.
The new pip provider is the only one to explicitly list upgradeable as a feature (along with installable etc.) so I assume this is automatically determined. If I remove the explicit has_feature list from pip, it still passes the tests (which include setting ensure to latest).
Edit: I’m also quite concerned that we’ve missed the Statler RC – can we still get it included in a later RC once this issue is resolved?
#42 Updated by Matt Robinson about 5 years ago
I’m not sure why that failure is happening either, and also noticed that most providers don’t explicitly declare upgradeable. I’ll look into it a bit today, I just didn’t have time yesterday with the RC stuff. And I’m pretty sure we can still get this in for an RC2 since it’s all new code and doesn’t affect the core system.
#43 Updated by Matt Robinson about 5 years ago
This appears to just be an issue with running the test on Macs. I’ve verified that the test doesn’t fail on multiple machines on Ubuntu and Redhat based systems, but does fail on multiple machines for Macs. For some reason on Mac’s the feautures for the provider don’t include upgradeable. Maybe something needs to be stubbed, but that really shouldn’t be in my mind. The features for a provider shouldn’t depend on the OS – or should they? I’ll be investigating.
#44 Updated by Matt Robinson about 5 years ago
- Status changed from In Topic Branch Pending Review to Merged - Pending Release
The resource being used for testing didn’t explicitly set the provider, so it ended up using whatever the default provider was on the system on which it was run. This was problematic when running the specs on a Mac since the default provider is pkgdmg and that provider doesn’t seem to be upgradeable. So when you tried to set the ensure to latest you got the error I mentioned above.
With that test fix I’ve gone ahead and merged this branch. Bugs against the provider should be filed as new tickets. Thanks to all those involved in getting this provider written.