[PATCH/puppet 2/5] Fix bug detecting empty uri path

[Russell Jackson]

Signed-off-by: Russell Jackson <r...@csub.edu>
---
lib/puppet/provider/package/freebsd.rb | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/puppet/provider/package/freebsd.rb b/lib/puppet/provider/package/freebsd.rb
index e0045ac..8632b50 100755
--- a/lib/puppet/provider/package/freebsd.rb
+++ b/lib/puppet/provider/package/freebsd.rb
@@ -96,7 +96,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa
if !defined? @source
if @resource[:source]
@source = URI.parse(@resource[:source])
- if !@source.hierarchical?
+ if @source.path.empty?
@source.merge! uri_path
end
else # source parameter not set; build default source URI
--
1.7.0.4

[Russell Jackson]

* Use port origin as resource name
* Use port index from package site to determine package to use for a
given origin
* Promote as default package provider. Demote portupgrade provider.

Signed-off-by: Russell Jackson <r...@csub.edu>
---

lib/puppet/provider/package/freebsd.rb | 161 +++++++++++++++++++++++++++-----
lib/puppet/provider/package/ports.rb | 2 -
2 files changed, 137 insertions(+), 26 deletions(-)

diff --git a/lib/puppet/provider/package/freebsd.rb b/lib/puppet/provider/package/freebsd.rb
index e10a20b..e0045ac 100755
--- a/lib/puppet/provider/package/freebsd.rb
+++ b/lib/puppet/provider/package/freebsd.rb
@@ -1,37 +1,150 @@
-Puppet::Type.type(:package).provide :freebsd, :parent => :openbsd do
- desc "The specific form of package management on FreeBSD. This is an
- extremely quirky packaging system, in that it freely mixes between
- ports and packages. Apparently all of the tools are written in Ruby,
- so there are plans to rewrite this support to directly use those
- libraries."
+require 'open-uri'

- commands :pkginfo => "/usr/sbin/pkg_info",
- :pkgadd => "/usr/sbin/pkg_add",
- :pkgdelete => "/usr/sbin/pkg_delete"
+Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Package do
+ include Puppet::Util::Execution
+
+ desc "The specific form of package management on FreeBSD. Resource names must be
+ specified as the port origin: <port_category>/<port_name>."
+
+ commands :pkginfo => "/usr/sbin/pkg_info",
+ :pkgadd => "/usr/sbin/pkg_add",
+ :pkgdelete => "/usr/sbin/pkg_delete"

confine :operatingsystem => :freebsd
+ defaultfor :operatingsystem => :freebsd

- def self.listcmd
- command(:pkginfo)
+ @@lock = Mutex.new
+ @@ports_index = nil
+
+ def self.parse_pkg_string(pkg_string)
+ {
+ :pkg_name => pkg_string.split("-").slice(0..-2).join("-"),
+ :pkg_version => pkg_string.split("-")[-1],
+ }
end

- def install
- should = @resource.should(:ensure)
+ def self.unparse_pkg_info(pkg_info)
+ [:pkg_name, :pkg_version].map { |key| pkg_info[key] }.join("-")
+ end
+
+ def self.parse_origin(origin_path)
+ begin
+ origin = {
+ :port_category => origin_path.split("/").fetch(-2),
+ :port_name => origin_path.split("/").fetch(-1),
+ }
+ rescue IndexError
+ raise Puppet::Error.new "#{origin_path}: not in required origin format: .*/<port_category>/<port_name>"
+ end
+ origin
+ end

- if @resource[:source] =~ /\/$/
- if @resource[:source] =~ /^(ftp|https?):/
- Puppet::Util::Execution::withenv :PACKAGESITE => @resource[:source] do
- pkgadd "-r", @resource[:name]
+ def self.instances
+ packages = []
+ output = pkginfo "-aoQ"
+ output.split("\n").each do |data|
+ pkg_string, pkg_origin = data.split(":")
+ pkg_info = self.parse_pkg_string(pkg_string)
+
+ packages << new({
+ :provider => self.name,
+ :name => pkg_origin,
+ :ensure => pkg_info[:pkg_version],
+ })
+ end
+ packages
+ end
+
+ def ports_index
+ @@lock.synchronize do
+ if @@ports_index.nil?
+ @@ports_index = {}
+ uri = source.merge File.join(source.path, "/INDEX")
+ Puppet.debug "Fetching INDEX: #{uri.inspect}"
+ begin
+ open(uri, "r") do |f|
+ while (line = f.gets)
+ fields = line.split("|")
+ pkg_info = self.class.parse_pkg_string(fields[0])
+ origin = self.class.parse_origin(fields[1])
+ @@ports_index[origin] = pkg_info
+ end
+ end
+ rescue
+ @@ports_index = nil
+ raise Puppet::Error.new "Could not fetch ports INDEX"
end
- else
- Puppet::Util::Execution::withenv :PKG_PATH => @resource[:source] do
- pkgadd @resource[:name]
+ end
+ end
+ @@ports_index
+ end
+
+ def uri_path
+ Facter.loadfacts
+ File.join(
+ "/", "pub", "FreeBSD", "ports",
+ Facter.value(:hardwareisa),
+ [
+ "packages",
+ Facter.value(:kernelmajversion).split(".")[0],
+ "stable",
+ ].join("-")
+ )
+ end
+
+ def source
+ if !defined? @source
+ if @resource[:source]
+ @source = URI.parse(@resource[:source])
+ if !@source.hierarchical?
+ @source.merge! uri_path
end
+ else # source parameter not set; build default source URI
+ @source = URI::Generic.build({
+ :scheme => "ftp",
+ :host => "ftp.freebsd.org",
+ :path => uri_path,
+ })
end
+ Puppet.debug "Package: #{@resource[:name]}: source => #{@source.inspect}"
+ end
+ @source
+ end
+
+ def origin
+ if !defined? @origin
+ @origin = self.class.parse_origin(@resource[:name])
+ Puppet.debug "Package: #{@resource[:name]}: origin => #{@origin.inspect}"
+ end
+ @origin
+ end
+
+ def package_uri
+ begin
+ pkg_name = self.class.unparse_pkg_info(ports_index.fetch(origin))
+ rescue IndexError
+ raise Puppet::Error.new "package not found in INDEX"
+ end
+ uri = source.merge File.join(source.path, "All", pkg_name + ".tbz")
+ Puppet.debug "Package: #{@resource[:name]}: package_uri => #{uri.inspect}"
+ uri
+ end
+
+ def install
+ should = @resource.should(:ensure)
+ origin # call origin so we check the package name for correctness early
+
+ # Source URI is for local file path.
+ if !source.absolute? or source.scheme == "file"
+ pkgadd source.path
+ # Source URI is to specific package file
+ elsif source.absolute? && source.path.end_with?(".tbz")
+ pkgadd source.to_s
+ # Source URI is to a package repository
else
- Puppet.warning "source is defined but does not have trailing slash, ignoring #{@resource[:source]}" if @resource[:source]
- pkgadd "-r", @resource[:name]
+ pkgadd "-f", package_uri.to_s
end
+ nil
end

def query
@@ -44,7 +157,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => :openbsd do
end

def uninstall
- pkgdelete "#{@resource[:name]}-#{@resource.should(:ensure)}"
+ output = pkginfo "-qO", @resource[:name]
+ output.split("\n").each { |pkg_name| pkgdelete([pkg_name]) }
end
end
-
diff --git a/lib/puppet/provider/package/ports.rb b/lib/puppet/provider/package/ports.rb
index c802092..1f2ed43 100755
--- a/lib/puppet/provider/package/ports.rb
+++ b/lib/puppet/provider/package/ports.rb
@@ -6,8 +6,6 @@ Puppet::Type.type(:package).provide :ports, :parent => :freebsd, :source => :fre
:portuninstall => "/usr/local/sbin/pkg_deinstall",
:portinfo => "/usr/sbin/pkg_info"

- defaultfor :operatingsystem => :freebsd
-
# I hate ports
%w{INTERACTIVE UNAME}.each do |var|
ENV.delete(var) if ENV.include?(var)
--
1.7.0.4

[Russell Jackson]

Signed-off-by: Russell Jackson <r...@csub.edu>
---

lib/puppet/provider/package/freebsd.rb | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/provider/package/freebsd.rb b/lib/puppet/provider/package/freebsd.rb
index 8632b50..b17a84e 100755
--- a/lib/puppet/provider/package/freebsd.rb
+++ b/lib/puppet/provider/package/freebsd.rb
@@ -100,8 +100,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa
@source.merge! uri_path
end

else # source parameter not set; build default source URI

- @source = URI::Generic.build({
- :scheme => "ftp",
+ @source = URI::FTP.build({
:host => "ftp.freebsd.org",
:path => uri_path,
})
--
1.7.0.4

[Russell Jackson]

Signed-off-by: Russell Jackson <r...@csub.edu>
---

lib/puppet/provider/package/freebsd.rb | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/puppet/provider/package/freebsd.rb b/lib/puppet/provider/package/freebsd.rb
index fcba793..f36e29e 100755
--- a/lib/puppet/provider/package/freebsd.rb
+++ b/lib/puppet/provider/package/freebsd.rb
@@ -1,4 +1,6 @@
require 'open-uri'
+require 'net/ftp'
+require 'bz2'

Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Package do
include Puppet::Util::Execution
@@ -71,20 +73,22 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa
@@lock.synchronize do
if @@ports_index.nil?
@@ports_index = {}
- uri = source.merge "INDEX"
+ uri = source.merge "INDEX.bz2"

Puppet.debug "Fetching INDEX: #{uri.inspect}"

begin

open(uri, "r") do |f|

- while (line = f.gets)
- fields = line.split("|")
- pkg_info = self.class.parse_pkg_string(fields[0])
- origin = self.class.parse_origin(fields[1])
- @@ports_index[origin] = pkg_info
+ BZ2::Reader.open(f.path) do |f|

+ while (line = f.gets)
+ fields = line.split("|")
+ pkg_info = self.class.parse_pkg_string(fields[0])
+ origin = self.class.parse_origin(fields[1])
+ @@ports_index[origin] = pkg_info
+ end

end
end
- rescue
+ rescue IOError, OpenURI::HTTPError, Net::FTPError
@@ports_index = nil
- raise Puppet::Error.new "Could not fetch ports INDEX"
+ raise Puppet::Error.new "Could not fetch ports INDEX: #{$!}"
end
end
end
--
1.7.0.4

[Matt Robinson]

Thanks for mailing that Russell, it's easier for us to keep track of what we still need to review if it gets sent to the list since we've been keeping track of patches in patchwork:  https://patchwork.puppetlabs.com/project/puppet/list/

That said, it's also really helpful if your commit messages include the ticket number so when we review the patch we can refer to the ticket in redmine (or on the mailing list) for appropriate context.

http://projects.puppetlabs.com/projects/puppet/wiki/Development_Lifecycle#Commit+Messages

Matt

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppe...@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.

[Russell Jackson]

* URI::FTP.merge tries to set typecode on argument to merge
even if it isn't another URI::FTP object.

* End default path in slash so the last element in the path isn't
truncated by URI.merge

Signed-off-by: Russell Jackson <r...@csub.edu>
---

lib/puppet/provider/package/freebsd.rb | 18 +++++++++++++++---
1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/puppet/provider/package/freebsd.rb b/lib/puppet/provider/package/freebsd.rb
index b17a84e..fcba793 100755
--- a/lib/puppet/provider/package/freebsd.rb
+++ b/lib/puppet/provider/package/freebsd.rb
@@ -16,6 +16,18 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa
@@lock = Mutex.new
@@ports_index = nil

+ # fix bug in URI::FTP merge method that tries to set typecode
+ # even when other is a string.
+ class URI::FTP
+ def merge(other)
+ tmp = super(other)
+ if self != tmp
+ tmp.set_typecode(other.typecode) rescue NoMethodError
+ end
+ return tmp

+ end
+ end
+

def self.parse_pkg_string(pkg_string)
{

:pkg_name => pkg_string.split("-").slice(0..-2).join("-"),

@@ -59,7 +71,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa

@@lock.synchronize do
if @@ports_index.nil?
@@ports_index = {}

- uri = source.merge File.join(source.path, "/INDEX")
+ uri = source.merge "INDEX"

Puppet.debug "Fetching INDEX: #{uri.inspect}"

begin

open(uri, "r") do |f|

@@ -89,7 +101,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa

Facter.value(:kernelmajversion).split(".")[0],

"stable",
].join("-")
- )
+ ) << "/"
end

def source
@@ -124,7 +136,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => Puppet::Provider::Packa
rescue IndexError

raise Puppet::Error.new "package not found in INDEX"

end
- uri = source.merge File.join(source.path, "All", pkg_name + ".tbz")
+ uri = source.merge File.join("All", pkg_name + ".tbz")

Puppet.debug "Package: #{@resource[:name]}: package_uri => #{uri.inspect}"

uri
end
--
1.7.0.4

[Matt Robinson]

I realized I forgot to include the ticket number in my last email about including ticket numbers.

I believe this patch series was for #4996.

[Jacob Helwig]

So, I've got a couple of comments on this patch series.

It really looks like all of this should be one commit, except for moving
"defaultfor :operatingsystem => :freebsd" from ports.rb to freebsd.rb
(which should be in its own commit). Not a killer, but would certainly
make it easier to review since it looks like later patches fix up things
that were written earlier in the series.

This is more nit-picking around the commit message, but it seems like
the commit message should be something more of the form:

(#4996) Use port origin when querying installed packages on FreeBSD

Something explaining why & how "This fixes the problem where the port
name specified in the package resource name (used by pkg_add -r)
doesn’t match what is recorded as the package name by pkg_info. Puppet
therefore thinks the package isn’t installed and tries to install it
on every run. By consistently using the port origin everywhere when
querying for installed packages, we avoid this problem."

Something explaining why "Use port index from package site to
determine package to use for a given origin" is necessary.

I trust that you know a lot more about FreeBSD and why these changes are
necessary (frankly, it wouldn't take much to know more than me about
FreeBSD ;), however I don't expect you to be "on call" for the rest of
this code's lifetime to be able to explain any reasoning behind it.
It'd be great if some of that domain knowledge and expertise were
transfered in the commit message for other people to reference while
working on the code in the future. ;)

I've got one other "major" comment, but I'll put that in the individual
commit email (4/5 in the series).

--
Jacob Helwig

[Jacob Helwig]

This definitely looks like a bug in URI::FTP, but rather than
monkey-patching URI::FTP (which really should go under
./lib/puppet/util/monkey_patches.rb, so all of the monkey-patching is in
the same place) it looks like we could just use the + method instead of
merge. It looks like it just Does The Right Thing(TM) from my testing,
and it works with both URI::Generic, and URI::FTP.

--
Jacob Helwig