From: Giovanni Bechis <giovanni@paclan.it>
Subject: Re: Overhaul package handling in puppet 8
To: Sebastian Reitenbach <sebastia@l00-bugdead-prods.de>
Cc: ports@openbsd.org, kn@openbsd.org, atalaran@gmail.com
Date: Wed, 10 Apr 2024 08:26:54 +0200

On 4/10/24 07:10, Sebastian Reitenbach wrote:
> Hi,
> On Tuesday, April 09, 2024 22:43 CEST, Giovanni Bechis <giovanni@paclan.it> wrote:
> 
>> On Tue, Apr 09, 2024 at 10:17:30PM +0200, Sebastian Reitenbach wrote:
>>> Since we now have a recent Puppet in ports, I started looking at how packages are handled with Puppet.
>>> My current trouble is that it wasn't really possible to install banches properly: i.e. can't properly install gimp,
>>> or auto* based on branch. Or esp, when want to install multiple of them, it was just not possible.
>>> For ports where branches conflict, i.e. postfix, this was working, but had to specify exact version, and on every
>>> upgrade bump the version .... _very_ annoying.
>>>
>>> Currently Puppet allows to install packages of a given version (ensure => "X.Y.Z"), or to follow updates (ensure => "latest").
>>>
>> [...]
>>> This is just for Puppet 8. Anyone still on Puppet 7? It should be easily ported to Puppet 7 as well.
>>>
>> I am on Puppet 7 and I do not have time to upgrade to Puppet 8 soon; I
> 
> My upgrade from Puppet 7 to 8 took quite a long time. They removed a lot of backward compat
> shim, and if you have many old and partially hand made or patched modules like I do, it took
> quite a while to update all of them....
> 
upgrading is on my queue and I have lot of hand made modules, other then that, can a Puppet 7 client connect to a Puppet 8 server ?

> Attached an untested version of the same changes for the Puppet 7 port.
> 
>> am also using "ensure => latest" syntax but I can change my code to get
>> rid of it OpenBSD will no more support it.
> 
> What's your use-case to using "ensure => latest"? If there's really a "good one",
> I can look into re-adding the feature.
> My use-case up to now for sparingly using "latest" was to force using "snapshot"
> branch of some packages.
> 
my OpenBSD Puppet server manages Linux and Windows servers as well, on those OS I want to update automatically only some trusted packages, not all the packages the distro wants to update.
Anyway I can change that part of my code without big issues.
  Cheers
    Giovanni

>>
>>   Cheers
>>    Giovanni
>>
>>
>>> cheers,
>>> Sebastian
>>>
>>>
>>> Index: Makefile
>>> ===================================================================
>>> RCS file: /cvs/ports/sysutils/ruby-puppet/8/Makefile,v
>>> diff -u -r1.2 Makefile
>>> --- Makefile	20 Mar 2024 21:21:14 -0000	1.2
>>> +++ Makefile	9 Apr 2024 19:51:40 -0000
>>> @@ -1,6 +1,7 @@
>>>   PORTROACH=		limit:^7
>>>   
>>>   VERSION=		8.5.1
>>> +REVISION=		0
>>>   
>>>   RUN_DEPENDS+=		converters/ruby-multi_json,${MODRUBY_FLAVOR}>=1.13,<2 \
>>>   			devel/ruby-concurrent-ruby,${MODRUBY_FLAVOR}>=1,<2 \
>>> Index: patches/patch-lib_puppet_provider_package_openbsd_rb
>>> ===================================================================
>>> RCS file: /cvs/ports/sysutils/ruby-puppet/8/patches/patch-lib_puppet_provider_package_openbsd_rb,v
>>> diff -u -r1.2 patch-lib_puppet_provider_package_openbsd_rb
>>> --- patches/patch-lib_puppet_provider_package_openbsd_rb	20 Mar 2024 21:21:14 -0000	1.2
>>> +++ patches/patch-lib_puppet_provider_package_openbsd_rb	9 Apr 2024 19:51:40 -0000
>>> @@ -1,42 +1,89 @@
>>> -- Handle errors from pkg_add
>>> -- Handle uninstall_options being 'nil' by default
>>> -- If no flavor speficied, force the empty flavor with '--'
>>> -  but skipping the % un-ambiguity pkg names
>>> -- Bail out on shortform PKG_PATH (i.e. 'ftp.openbsd.org')
>>> -- pkg.conf is gone
>>> -- properly handle packages with multiple versions and flavors,
>>> -  i.e. postfix-XXX-flavor
>>> -
>>> +- get rid of versionable (no ensure => "version X.X.X")
>>> +- get rid of upgradeable (ensure => latest)
>>> +- properly support branches
>>>   
>>>   Index: lib/puppet/provider/package/openbsd.rb
>>>   --- lib/puppet/provider/package/openbsd.rb.orig
>>>   +++ lib/puppet/provider/package/openbsd.rb
>>> -@@ -24,6 +24,8 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> -   has_feature :upgradeable
>>> +@@ -6,10 +6,14 @@ require_relative '../../../puppet/provider/package'
>>> + Puppet::Type.type(:package).provide :openbsd, :parent => Puppet::Provider::Package do
>>> +   desc "OpenBSD's form of `pkg_add` support.
>>> +
>>> ++    OpenBSD has the concept of package branches, providing multiple versions of the
>>> ++    same package, i.e. `stable` vs. `snapshot`. To select a specific branch,
>>> ++    suffix the package name with % sign follwed by the branch name, i.e. `gimp%stable`.
>>> ++
>>> +     This provider supports the `install_options` and `uninstall_options`
>>> +     attributes, which allow command-line flags to be passed to pkg_add and pkg_delete.
>>> +     These options should be specified as an array where each element is either a
>>> +-     string or a hash."
>>> ++    string or a hash."
>>> +
>>> +   commands :pkginfo => "pkg_info",
>>> +            :pkgadd => "pkg_add",
>>> +@@ -18,220 +22,94 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> +   defaultfor 'os.name' => :openbsd
>>> +   confine 'os.name' => :openbsd
>>> +
>>> +-  has_feature :versionable
>>> +   has_feature :install_options
>>> +   has_feature :uninstall_options
>>> +-  has_feature :upgradeable
>>>      has_feature :supports_flavors
>>>    
>>> -+  mk_resource_methods
>>> -+
>>>      def self.instances
>>> -     packages = []
>>> -
>>> -@@ -46,12 +48,6 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> -
>>> -             packages << new(hash)
>>> -             hash = {}
>>> +-    packages = []
>>> +-
>>> ++    final = []
>>> +     begin
>>> +-      execpipe(listcmd) do |process|
>>> +-        # our regex for matching pkg_info output
>>> +-        regex = /^(.*)-(\d[^-]*)[-]?([\w-]*)(.*)$/
>>> +-        fields = [:name, :ensure, :flavor]
>>> +-        hash = {}
>>> ++      packages = listcmd
>>> ++      packages.each { |package, value|
>>> ++        if !package.empty?()
>>> ++          value[:provider] = self.name
>>> ++          final << new(value)
>>> ++        end
>>> ++      }
>>> ++      return final
>>> +
>>> +-        # now turn each returned line into a package object
>>> +-        process.each_line { |line|
>>> +-          match = regex.match(line.split[0])
>>> +-          if match
>>> +-            fields.zip(match.captures) { |field, value|
>>> +-              hash[field] = value
>>> +-            }
>>> +-
>>> +-            hash[:provider] = self.name
>>> +-
>>> +-            packages << new(hash)
>>> +-            hash = {}
>>>   -          else
>>>   -            unless line =~ /Updating the pkgdb/
>>>   -              # Print a warning on lines we can't match, but move
>>>   -              # on, since it should be non-fatal
>>>   -              warning(_("Failed to match line %{line}") % { line: line })
>>>   -            end
>>> -           end
>>> -         }
>>> -       end
>>> -@@ -67,26 +63,17 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> +-          end
>>> +-        }
>>> +-      end
>>> +-
>>> +-      return packages
>>> +     rescue Puppet::ExecutionFailure
>>> +-      return nil
>>> ++      nil
>>> +     end
>>>      end
>>>    
>>> -   def latest
>>> +   def self.listcmd
>>> +-    [command(:pkginfo), "-a"]
>>> +-  end
>>> +-
>>> +-  def latest
>>>   -    parse_pkgconf
>>>   -
>>>   -    if @resource[:source][-1, 1] == ::File::SEPARATOR
>>> @@ -45,55 +92,51 @@
>>>   -      e_vars = {}
>>>   -    end
>>>   -
>>> -     if @resource[:flavor]
>>> -       query = "#{@resource[:name]}--#{@resource[:flavor]}"
>>> -     else
>>> +-    if @resource[:flavor]
>>> +-      query = "#{@resource[:name]}--#{@resource[:flavor]}"
>>> +-    else
>>>   -      query = @resource[:name]
>>> -+      query = @resource[:name] + "--"
>>> -     end
>>> -
>>> +-    end
>>> +-
>>>   -    output = Puppet::Util.withenv(e_vars) { pkginfo "-Q", query }
>>>   -    version = properties[:ensure]
>>> -+    output = Puppet::Util.withenv({}) {pkginfo "-Q", query}
>>> -
>>> -     if output.nil? or output.size == 0 or output =~ /Error from /
>>> -       debug "Failed to query for #{resource[:name]}"
>>> +-
>>> +-    if output.nil? or output.size == 0 or output =~ /Error from /
>>> +-      debug "Failed to query for #{resource[:name]}"
>>>   -      return version
>>> -+      return properties[:ensure]
>>> -     else
>>> -       # Remove all fuzzy matches first.
>>> -       output = output.split.select { |p| p =~ /^#{resource[:name]}-(\d[^-]*)[-]?(\w*)/ }.join
>>> -@@ -95,21 +82,22 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> -
>>> -     if output =~ /^#{resource[:name]}-(\d[^-]*)[-]?(\w*) \(installed\)$/
>>> -       debug "Package is already the latest available"
>>> +-    else
>>> +-      # Remove all fuzzy matches first.
>>> +-      output = output.split.select { |p| p =~ /^#{resource[:name]}-(\d[^-]*)[-]?(\w*)/ }.join
>>> +-      debug "pkg_info -Q for #{resource[:name]}: #{output}"
>>> +-    end
>>> +-
>>> +-    if output =~ /^#{resource[:name]}-(\d[^-]*)[-]?(\w*) \(installed\)$/
>>> +-      debug "Package is already the latest available"
>>>   -      return version
>>> -+      return properties[:ensure]
>>> -     else
>>> -       match = /^(.*)-(\d[^-]*)[-]?(\w*)$/.match(output)
>>> -       debug "Latest available for #{resource[:name]}: #{match[2]}"
>>> -
>>> +-    else
>>> +-      match = /^(.*)-(\d[^-]*)[-]?(\w*)$/.match(output)
>>> +-      debug "Latest available for #{resource[:name]}: #{match[2]}"
>>> +-
>>>   -      if version.to_sym == :absent || version.to_sym == :purged
>>> -+      if properties[:ensure].to_sym == :absent
>>> -         return match[2]
>>> -       end
>>> -
>>> -       vcmp = version.split('.').map { |s| s.to_i } <=> match[2].split('.').map { |s| s.to_i }
>>> -+      vcmp = properties[:ensure].split('.').map{|s|s.to_i} <=> match[2].split('.').map{|s|s.to_i}
>>> -       if vcmp > 0
>>> -         # The locally installed package may actually be newer than what a mirror
>>> -         # has. Log it at debug, but ignore it otherwise.
>>> +-        return match[2]
>>> +-      end
>>> +-
>>> +-      vcmp = version.split('.').map { |s| s.to_i } <=> match[2].split('.').map { |s| s.to_i }
>>> +-      if vcmp > 0
>>> +-        # The locally installed package may actually be newer than what a mirror
>>> +-        # has. Log it at debug, but ignore it otherwise.
>>>   -        debug "Package #{resource[:name]} #{version} newer then available #{match[2]}"
>>>   -        return version
>>> -+        debug "Package #{resource[:name]} #{properties[:ensure]} newer then available #{match[2]}"
>>> -+        return properties[:ensure]
>>> -       else
>>> -         return match[2]
>>> -       end
>>> -@@ -120,57 +108,25 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> -     self.install(true)
>>> -   end
>>> -
>>> +-      else
>>> +-        return match[2]
>>> +-      end
>>> +-    end
>>> +-  end
>>> +-
>>> +-  def update
>>> +-    self.install(true)
>>> +-  end
>>> +-
>>>   -  def parse_pkgconf
>>>   -    unless @resource[:source]
>>>   -      if Puppet::FileSystem.exist?("/etc/pkg.conf")
>>> @@ -111,7 +154,20 @@
>>>   -              end
>>>   -            end
>>>   -          end
>>> --        end
>>> ++    regex_fuzzy = /^(.*)--([\w-]+)?(%[^w]+)?$/
>>> ++    f = []
>>> ++    f = pkginfo("-a", "-z").split("\n")
>>> ++    packages = {}
>>> ++    f.each do |line|
>>> ++        match = regex_fuzzy.match(line.split[0])
>>> ++        name = match.captures[0]
>>> ++        flavor = match.captures[1]
>>> ++        branch = match.captures[2]
>>> ++        if branch.nil?
>>> ++               	pname = name
>>> ++        else
>>> ++               	pname = name + branch
>>> +         end
>>>   -
>>>   -        unless @resource[:source]
>>>   -          raise Puppet::Error,
>>> @@ -121,14 +177,18 @@
>>>   -        raise Puppet::Error,
>>>   -              _("You must specify a package source or configure an installpath in /etc/pkg.conf")
>>>   -      end
>>> --    end
>>> --  end
>>> --
>>> -   def install(latest = false)
>>> ++        packages[pname] = { :name => pname, :flavor => flavor, :branch => branch, :ensure => "present" }
>>> +     end
>>> ++    packages
>>> +   end
>>> +
>>> +-  def install(latest = false)
>>> ++  def install
>>>        cmd = []
>>>    
>>>   -    parse_pkgconf
>>> --
>>> ++    full_name = get_full_name(action="install")
>>> +
>>>   -    if @resource[:source][-1, 1] == ::File::SEPARATOR
>>>   -      e_vars = { 'PKG_PATH' => @resource[:source] }
>>>   -      full_name = get_full_name(latest)
>>> @@ -139,27 +199,24 @@
>>>   -
>>>   +    cmd << '-r'
>>>        cmd << install_options
>>> --    cmd << full_name
>>> -+    cmd << get_full_name(latest)
>>> +     cmd << full_name
>>>    
>>> -     if latest
>>> +-    if latest
>>>   -      cmd.unshift('-rz')
>>> -+      cmd.unshift('-z')
>>> -     end
>>> -
>>> --    Puppet::Util.withenv(e_vars) { pkgadd cmd.flatten.compact }
>>>   +    # pkg_add(1) doesn't set the return value upon failure so we have to peek
>>>   +    # at it's output to see if something went wrong.
>>>   +    output = Puppet::Util.withenv({}) { pkgadd cmd.flatten.compact }
>>> -+    require 'pp'
>>> -+    pp output
>>>   +    if output =~ /Can't find /
>>>   +      self.fail "pkg_add returned: #{output.chomp}"
>>> -+    end
>>> +     end
>>> +-
>>> +-    Puppet::Util.withenv(e_vars) { pkgadd cmd.flatten.compact }
>>>      end
>>>    
>>> -   def get_full_name(latest = false)
>>> -@@ -179,11 +135,20 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> +-  def get_full_name(latest = false)
>>> ++  def get_full_name(action="install")
>>> +     # In case of a real update (i.e., the package already exists) then
>>> +     # pkg_add(8) can handle the flavors. However, if we're actually
>>>        # installing with 'latest', we do need to handle the flavors. This is
>>>        # done so we can feed pkg_add(8) the full package name to install to
>>>        # prevent ambiguity.
>>> @@ -168,53 +225,35 @@
>>>   -    elsif latest
>>>   -      # Don't depend on get_version for updates.
>>>   -      @resource[:name]
>>> -+    if resource[:flavor]
>>> -+      # If :ensure contains a version, use that instead of looking it up.
>>> -+      # This allows for installing packages with the same stem, but multiple
>>> -+      # version such as postfix-VERSION-flavor.
>>> -+      if @resource[:ensure].to_s =~ /(\d[^-]*)$/
>>> -+        use_version = @resource[:ensure]
>>> -+      else
>>> -+        use_version = ''
>>> -+      end
>>> -+      "#{resource[:name]}-#{use_version}-#{resource[:flavor]}"
>>> -+    elsif resource[:name].to_s.match(/[a-z0-9]%[0-9a-z]/i)
>>> -+        "#{resource[:name]}"
>>> -+    elsif not latest
>>> -+      "#{resource[:name]}--"
>>> -     else
>>> -       # If :ensure contains a version, use that instead of looking it up.
>>> -       # This allows for installing packages with the same stem, but multiple
>>> -@@ -194,33 +159,41 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> -         use_version = get_version
>>> -       end
>>> +-    else
>>> +-      # If :ensure contains a version, use that instead of looking it up.
>>> +-      # This allows for installing packages with the same stem, but multiple
>>> +-      # version such as openldap-server.
>>> +-      if @resource[:ensure].to_s =~ /(\d[^-]*)$/
>>> +-        use_version = @resource[:ensure]
>>> +-      else
>>> +-        use_version = get_version
>>> +-      end
>>>    
>>>   -      [@resource[:name], use_version, @resource[:flavor]].join('-').gsub(/-+$/, '')
>>> -+      if resource[:flavor]
>>> -+        [ @resource[:name], use_version, @resource[:flavor]].join('-').gsub(/-+$/, '')
>>> -+      else
>>> -+        [ @resource[:name], use_version ]
>>> -+      end
>>> ++    name_branch_regex = /^(\S*)(%\w*)$/
>>> ++    match = name_branch_regex.match(@resource[:name])
>>> ++    if match
>>> ++      use_name = match.captures[0]
>>> ++      use_branch = match.captures[1]
>>> ++    else
>>> ++      use_name = @resource[:name]
>>> ++      use_branch = ''
>>>        end
>>> -   end
>>> +-  end
>>>    
>>> -   def get_version
>>> +-  def get_version
>>>   -    execpipe([command(:pkginfo), "-I", @resource[:name]]) do |process|
>>>   -      # our regex for matching pkg_info output
>>>   -      regex = /^(.*)-(\d[^-]*)[-]?(\w*)(.*)$/
>>>   -      master_version = 0
>>>   -      version = -1
>>> -+    pkg_search_name = @resource[:name]
>>> -+    unless pkg_search_name.match(/[a-z0-9]%[0-9a-z]/i) and not @resource[:flavor]
>>> -+      # we are only called when no flavor is specified
>>> -+      # so append '--' to the :name to avoid patch versions on flavors
>>> -+      pkg_search_name << "--"
>>> -+    end
>>> -+    # our regex for matching pkg_info output
>>> -+    regex = /^(.*)-(\d[^-]*)[-]?(\w*)(.*)$/
>>> -+    master_version = 0
>>> -+    version = -1
>>> -
>>> +-
>>>   -      process.each_line do |line|
>>>   -        match = regex.match(line.split[0])
>>>   -        if match
>>> @@ -224,29 +263,36 @@
>>>   -
>>>   -          master_version = version unless master_version > version
>>>   -        end
>>> -+    # pkg_info -I might return multiple lines, i.e. flavors
>>> -+    matching_pkgs = pkginfo("-I", "pkg_search_name")
>>> -+    matching_pkgs.each_line do |line|
>>> -+      if match = regex.match(line.split[0])
>>> -+        # now we return the first version, unless ensure is latest
>>> -+        version = match.captures[1]
>>> -+        return version unless @resource[:ensure] == "latest"
>>> -+        master_version = version unless master_version > version
>>> -       end
>>> -+    end
>>> -
>>> +-      end
>>> +-
>>>   -      return master_version unless master_version == 0
>>>   -      return '' if version == -1
>>> -+    return master_version unless master_version == 0
>>> -+    return '' if version == -1
>>> -+    raise Puppet::Error, _("%{version} is not available for this package") % { version: version }
>>> -
>>> +-
>>>   -      raise Puppet::Error, _("%{version} is not available for this package") % { version: version }
>>> --    end
>>> -   rescue Puppet::ExecutionFailure
>>> -     return nil
>>> ++    if @resource[:flavor]
>>> ++      return "#{use_name}--#{@resource[:flavor]}#{use_branch}"
>>> ++    else
>>> ++      return "#{use_name}--#{use_branch}"
>>> +     end
>>> +-  rescue Puppet::ExecutionFailure
>>> +-    return nil
>>> ++
>>>      end
>>> -@@ -239,7 +212,7 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> +
>>> +   def query
>>> +-    # Search for the version info
>>> +-    if pkginfo(@resource[:name]) =~ /Information for (inst:)?#{@resource[:name]}-(\S+)/
>>> +-      return { :ensure => $2 }
>>> +-    else
>>> +-      return nil
>>> ++    pkg = self.class.instances.find do |package|
>>> ++      @resource[:name] == package.name
>>> +     end
>>> ++    pkg ? pkg.properties : nil
>>> +   end
>>> +
>>> +   def install_options
>>> +@@ -239,15 +117,19 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>>      end
>>>    
>>>      def uninstall_options
>>> @@ -254,4 +300,27 @@
>>>   +    [join_options(resource[:uninstall_options])]
>>>      end
>>>    
>>> -   def uninstall
>>> +-  def uninstall
>>> +-    pkgdelete uninstall_options.flatten.compact, @resource[:name]
>>> ++  def uninstall(purge = false)
>>> ++    if purge
>>> ++      pkgdelete "-c", "-qq", uninstall_options.flatten.compact, get_full_name(action="uninstall")
>>> ++    else
>>> ++      pkgdelete uninstall_options.flatten.compact, get_full_name(action="uninstall")
>>> ++    end
>>> +   end
>>> +
>>> +   def purge
>>> +-    pkgdelete "-c", "-q", @resource[:name]
>>> ++    uninstall(purge=true)
>>> +   end
>>> +
>>> +   def flavor
>>> +@@ -256,7 +138,6 @@ Puppet::Type.type(:package).provide :openbsd, :parent
>>> +
>>> +   def flavor=(value)
>>> +     if flavor != @resource.should(:flavor)
>>> +-      uninstall
>>> +       install
>>> +     end
>>> +   end
>>
>>