The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Feature #4258

Add pkgutil provider

Added by James Turnbull almost 6 years ago. Updated about 4 years ago.

Status:ClosedStart date:07/16/2010
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Solaris
Target version:2.7.0
Affected Puppet version:0.25.5 Branch:https://github.com/domcleal/puppet/tree/tickets/master/4258-dev
Keywords:communitypatch

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


pkgutil.rb.patch Magnifier (684 Bytes) Rudy Gevaert, 11/01/2010 05:15 pm

pkgutil_4258_gpg.patch Magnifier - Fixes install status (from Rudy) and fixes package status if use_gpg enabled (924 Bytes) Dominic Cleal, 11/11/2010 05:07 pm

History

#1 Updated by James Turnbull almost 6 years ago

  • Branch set to http://github.com/jamtur01/puppet/tree/tickets/master/4258

#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[14858]: [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.

#3 Updated by James Turnbull over 5 years ago

  • Target version changed from 2.6.1 to 2.6.2

#4 Updated by Jacob Helwig over 5 years ago

  • Target version changed from 2.6.2 to 69

#5 Updated by James Turnbull over 5 years ago

I think this is a better set of patches:

http://github.com/automatthias/puppet/commits/master/

#6 Updated by James Turnbull over 5 years ago

  • Assignee deleted (James Turnbull)

#7 Updated by Rudy Gevaert over 5 years ago

I want to test this. However I don’t know anything about git. How can I easily add this to my running puppet client? And do I need to add it on the puppet master too?

#8 Updated by Rudy Gevaert over 5 years ago

Hi James,

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.

I’m running

bash-3.00# pkgutil --version
2.1

#9 Updated by Rudy Gevaert over 5 years ago

Maciej, I don’t understand on what you based your regular expression.

I ran every released version of ‘pkgutil -c’ and every released version produces the same output.

#10 Updated by Rudy Gevaert over 5 years ago

An other issue that should be handled is when doing pkgutil -c --single somepackage pkgutil exits with exit code 1 when the package is not in the catalogue. I don’t understand all what is going on but it seems puppet barfs on the exit code 1.

#11 Updated by Dominic Cleal over 5 years ago

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.

#12 Updated by Rudy Gevaert over 5 years ago

We are nog using gpg to check the signatures, only md5. Dominic, I’ll test your patch ASAP. I saw that the pkgutil author responded to my post on the the puppet google group.

#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

#14 Updated by James Turnbull over 5 years ago

  • Target version changed from 69 to 2.7.x

#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

#16 Updated by Matt Robinson over 5 years ago

  • Keywords set to communitypatch

#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

Hi Juerg,

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.

#19 Updated by James Turnbull about 5 years ago

Jeurg

Can you add your branch to the ticket please. Thanks.

#20 Updated by Juerg Walz about 5 years ago

Hi James

My branch is: https://github.com/gw42/puppet/tree/tickets/master/4258-dev

I’ve cloned Dominic Cleal’s repo.

Cheers, Juerg

#21 Updated by James Turnbull about 5 years ago

  • Branch changed from http://github.com/jamtur01/puppet/tree/tickets/master/4258 to https://github.com/gw42/puppet/tree/tickets/master/4258-dev

#22 Updated by Dominic Cleal about 5 years ago

Thanks Juerg. Which version of pkgutil have you tested against? I’m aware that 2.3 is out now and starts using non-zero exit codes in some cases so it’d be good to have checked this still works throughout.

#23 Updated by Juerg Walz about 5 years ago

Dominic, I’m using version 2.3. So far my test were successful (with the proposed changes in the provider). The new -parse option might be useful, but I think it’s a bit too early to rely on it (“backwards” compatibility).

Juerg

#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.

#26 Updated by Juerg Walz about 5 years ago

I’ve made another minor update to handle packages where the shortname is not equal to the package file name (eg. “screen” where the package name is “CSWscrn”).

#27 Updated by James Turnbull about 5 years ago

  • Branch changed from https://github.com/gw42/puppet/tree/tickets/master/4258-dev to https://github.com/domcleal/puppet/tree/tickets/master/4258-dev

#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?

#32 Updated by Rudy Gevaert about 5 years ago

@dominic, that would be nice way, imho. However we should make sure we use the correct pkgutil.conf. /opt/etc/… /etc/opt/ …

#33 Updated by Mark Phillips about 5 years ago

That would be /etc/opt/csw/pkgutil.conf, and always should be.

#34 Updated by Juerg Walz about 5 years ago

I’ve added the suggested check for the wgetopts (https://github.com/gw42/puppet/tree/tickets/master/4258-dev).

#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

https://github.com/gw42/puppet/tree/tickets/master/4258-dev

#36 Updated by Juerg Walz about 5 years ago

My repository should have all the latest updates.

I haven’t got any feedback on my latest patch yet, though.

#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.

commit:d88b3763cea9e116c8abf45ca2aa4ec80fa20349

#45 Updated by James Turnbull almost 5 years ago

  • Status changed from Merged - Pending Release to Closed
  • Target version changed from 2.7.x to 2.7.0

#46 Updated by Anonymous about 4 years ago

  • Assignee deleted (Matt Robinson)

Also available in: Atom PDF