From 7f7ebce8ad4fa3b9fe5f6645c82dfd44cab536c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= <lslezak@suse.cz>
Date: Wed, 10 Jul 2019 17:20:40 +0200
Subject: [PATCH] Comments

---
 devel/product_scanner.rb                      |  9 ++-
 src/lib/y2packager/dialogs/addon_selector.rb  | 16 +++--
 .../y2packager/dialogs/product_summary.erb    |  6 +-
 src/lib/y2packager/product_finder.rb          | 65 +++++++++++++++----
 src/lib/y2packager/product_location.rb        | 15 ++++-
 src/modules/Packages.rb                       |  3 +
 test/product_location_test.rb                 |  4 +-
 7 files changed, 93 insertions(+), 25 deletions(-)

diff --git a/devel/product_scanner.rb b/devel/product_scanner.rb
index 97b9560ac..e1081c8f7 100755
--- a/devel/product_scanner.rb
+++ b/devel/product_scanner.rb
@@ -1,9 +1,13 @@
 #! /usr/bin/env ruby
 
-# This script tests the product scanner which evaluates the
-# SLES Packages DVD or the Offline installation medium.
+# This is a testing script which runs the product scanner
+# for the SLES Packages DVD or the Offline installation medium.
 
+require "pp"
 require "yast"
+
+# YaST modifies the load path, we need to update it
+# *after* calling require "yast"
 $LOAD_PATH.unshift(File.join(__dir__, "../src/lib"))
 
 require "y2packager/product_location"
@@ -25,5 +29,4 @@
 
 puts "Scanning #{url}..."
 
-require "pp"
 pp Y2Packager::ProductLocation.scan(url, base_product)
diff --git a/src/lib/y2packager/dialogs/addon_selector.rb b/src/lib/y2packager/dialogs/addon_selector.rb
index 06c55b1b3..8ac0c3150 100644
--- a/src/lib/y2packager/dialogs/addon_selector.rb
+++ b/src/lib/y2packager/dialogs/addon_selector.rb
@@ -32,7 +32,8 @@ class AddonSelector < ::UI::InstallationDialog
       attr_reader :selected_products
 
       # TODO: handle a theoretical case when a product subdirectory contains several
-      # libzypp products
+      # libzypp products (only for 3rd party or manually created media, the official
+      # SUSE media always contain one product per repository)
 
       # Constructor
       #
@@ -41,8 +42,10 @@ def initialize(products)
         super()
         textdomain "packager"
 
-        # do not offer base products, they would conflict with the already selected base product
-        @products = products.select { |p| !p.base }
+        @products = products
+        # do not offer base products, they would conflict with the already selected base product,
+        # allow a hidden way to force displaying them in some special cases
+        @products.reject!(&:base) unless ENV["Y2_DISPLAY_BASE_PRODUCTS"] == "1"
         @selected_products = []
       end
 
@@ -85,8 +88,8 @@ def addon_repos_handler
         select_dependant_products
       end
 
+      # refresh the details of the currently selected add-on
       def refresh_details(current_product)
-        # refresh the details
         details = product_description(current_product)
         Yast::UI.ChangeWidget(Id(:details), :Value, details)
         Yast::UI.ChangeWidget(Id(:details), :Enabled, true)
@@ -96,11 +99,15 @@ def select_dependant_products
         # select the dependent products
         new_selection = current_selection
 
+        # the selection has not changed, nothing to do
         return if new_selection == selected_products
 
+        # add the dependant items to the selected list
         selected_items = Yast::UI.QueryWidget(Id(:addon_repos), :SelectedItems)
         new_items = new_selection - selected_products
         new_items.each do |p|
+          # the dependencies contain also the transitive (indirect) dependencies,
+          # we do not need to recursively evaluate the list
           selected_items.concat(p.depends_on)
         end
 
@@ -209,6 +216,7 @@ def product_description(product)
         dependencies = []
         if product.depends_on && !product.depends_on.empty?
           product.depends_on.each do |p|
+            # display the human readable product name instead of the product directory
             prod = @products.find { |pr| pr.dir == p }
             dependencies << (prod.summary || prod.name) if prod
           end
diff --git a/src/lib/y2packager/dialogs/product_summary.erb b/src/lib/y2packager/dialogs/product_summary.erb
index 822802e2f..32322274c 100644
--- a/src/lib/y2packager/dialogs/product_summary.erb
+++ b/src/lib/y2packager/dialogs/product_summary.erb
@@ -2,14 +2,18 @@
 textdomain "packager"
 %>
 
+<%# TRANSLATORS: the RichText header, followed by the name of the directory %>
 <b><%= _("Directory on the Media:") %></b> <%= h(product.dir) %><br>
+<%# TRANSLATORS: the RichText header, followed by the name of the medium %>
 <b><%= _("Media Name:") %></b> <%= h(product.name) %><br>
 
 <% if product.product_name %>
+  <%# TRANSLATORS: the RichText header, followed by the product identifier, e.g. "SLES" %>
   <b><%= _("Product ID:") %></b> <%= h(product.product_name) %>
 <% end %>
 
 <% if !dependencies.empty? %>
+  <%# TRANSLATORS: the RichText section header, followed by the names of the dependant products %>
   <h3><%= _("Dependencies") %></h3>
   <ul>
     <% dependencies.each do |dep| %>
@@ -19,7 +23,7 @@ textdomain "packager"
 <% end %>
 
 <% if product.description %>
-  <%# TRANSLATORS: the header followed by an untranslated (English) product description %>
+  <%# TRANSLATORS: the RichText header, it is followed by an untranslated (English) product description %>
   <h3><%= _("Product Description (English Only)") %></h3>
   <%# no escaping, the description already *is* a rich text %>
   <p><%= product.description %></p>
diff --git a/src/lib/y2packager/product_finder.rb b/src/lib/y2packager/product_finder.rb
index 760cc735f..be84e5dca 100644
--- a/src/lib/y2packager/product_finder.rb
+++ b/src/lib/y2packager/product_finder.rb
@@ -15,30 +15,46 @@
 module Y2Packager
   # This class finds products in a Solv pool
   class ProductFinder
+    #
+    # Constructor
+    #
+    # @param [Solv::Pool] pool <description>
+    #
     def initialize(pool)
       @pool = pool.pool
     end
 
-    # TODO: refactor this long method
+    #
+    # Evaluate all products in the pool and return details about them
+    # including the dependencies.
+    #
+    # @param base [String] The name of the base product used for evaluating the
+    #  dependencies.
+    #
+    # @return [Array<Hash>] The list of found products
+    #
     def products(base = nil)
       marked_base_products = base_product_tags
       # evaluate all products
       pool.whatprovides(pool.str2id("product()")).each_with_object([]) do |s, list|
+        # the dependant repositories, includes also the transient dependencies
         required = []
         solver = pool.Solver
-        # install the solvable (the product *-release package)
+        # select this product solvable (the product *-release package)
         jobs = [pool.Job(Solv::Job::SOLVER_SOLVABLE | Solv::Job::SOLVER_INSTALL, s.id)]
 
         if base
-          # add the base product
+          # select the base product
           base_product = pool.select("product(#{base})", Solv::Selection::SELECTION_PROVIDES)
           jobs += base_product.jobs(Solv::Job::SOLVER_INSTALL)
         end
 
+        # run the solver to evaluate all dependencies
         problems = solver.solve(jobs)
 
         # in case of problems, ignore the dependencies
         if problems.empty?
+          # find all repositories which have a product selected to install
           solver.transaction.newsolvables.each do |n|
             next if n == s
             n.lookup_deparray(Solv::SOLVABLE_PROVIDES).each do |dep|
@@ -51,25 +67,18 @@ def products(base = nil)
           required.sort!
         end
 
-        provides = s.lookup_deparray(Solv::SOLVABLE_PROVIDES)
-        order = nil
-
-        provides.each do |p|
-          next unless p.str =~ /\Adisplayorder\(\s*([0-9]+)\s*\)\z/
-          order = Regexp.last_match[1].to_i if Regexp.last_match
-        end
-
         ret = {
           prod_dir:        s.repo.name,
           product_package: s.name,
           summary:         s.lookup_str(Solv::SOLVABLE_SUMMARY),
           description:     s.lookup_str(Solv::SOLVABLE_DESCRIPTION),
           depends_on:      problems.empty? ? required : nil,
-          order:           order
+          order:           display_order(s)
         }
 
-        # in theory a release package might provide several products
-        provides.each do |p|
+        # in theory a release package might provide several products,
+        # create an item for each of them
+        s.lookup_deparray(Solv::SOLVABLE_PROVIDES).each do |p|
           next unless p.str =~ /\Aproduct\(\)\s*=\s*(\S+)/
           product_name = Regexp.last_match[1]
           product_data = {
@@ -84,8 +93,15 @@ def products(base = nil)
 
   private
 
+    #
+    # Return the list of marked base products. A base product is defined
+    # by the "system-installation() = <product>" provides.
+    #
+    # @return [Array<String>] The base products
+    #
     def base_product_tags
       install_provides = pool.whatprovides(pool.str2id("system-installation()"))
+
       tags = install_provides.each_with_object([]) do |s, list|
         provides = s.lookup_deparray(Solv::SOLVABLE_PROVIDES)
 
@@ -94,9 +110,30 @@ def base_product_tags
           list << Regexp.last_match[1]
         end
       end
+
       tags.uniq
     end
 
+    #
+    # Find the "displayorder()"" provides value for the specific solvable object
+    #
+    # @param solvable [Solv] The solvable object from the pool
+    #
+    # @return [Integer,nil] The display order value or nil if not defined
+    #
+    def display_order(solvable)
+      # all solvable provides
+      provides = solvable.lookup_deparray(Solv::SOLVABLE_PROVIDES)
+
+      order = nil
+      provides.each do |p|
+        next unless p.str =~ /\Adisplayorder\(\s*([0-9]+)\s*\)\z/
+        order = Regexp.last_match[1].to_i if Regexp.last_match
+      end
+
+      order
+    end
+
     attr_reader :pool
   end
 end
diff --git a/src/lib/y2packager/product_location.rb b/src/lib/y2packager/product_location.rb
index 841f3ca50..ce4281526 100644
--- a/src/lib/y2packager/product_location.rb
+++ b/src/lib/y2packager/product_location.rb
@@ -41,9 +41,21 @@ class ProductLocation
     attr_reader :order
     # @return [Boolean] Base product flag
     attr_reader :base
-    # @return [Array<String>] The product dependencies
+    # @return [Array<String>] The product dependencies, includes also the transitive
+    #  (indirect) dependencies
     attr_reader :depends_on
 
+    #
+    # Scan the URL for the available product subdirectories
+    # and their products.
+    #
+    # @param url [String] The base repository URL
+    # @param base_product [String,nil]  The base product used for evaluating the
+    #   product dependencies, if nil the solver can select any product to satisfy
+    #   the dependencies.
+    #
+    # @return [Array<Y2Packager::ProductLocation>] The found products
+    #
     def self.scan(url, base_product = nil)
       log.info "Scanning #{Yast::URL.HidePassword(url)} for products..."
 
@@ -60,6 +72,7 @@ def self.scan(url, base_product = nil)
       finder = Y2Packager::ProductFinder.new(pool)
 
       # TODO: handle also subdirectories which do not contain any product
+      # (custom or 3rd party repositories)
       finder.products(base_product).map do |p|
         media_name_pair = downloader.product_repos.find { |r| r[1] == p[:prod_dir] }
         media_name = media_name_pair ? media_name_pair.first : p[:prod_dir]
diff --git a/src/modules/Packages.rb b/src/modules/Packages.rb
index 25e314fef..4b8f266ba 100644
--- a/src/modules/Packages.rb
+++ b/src/modules/Packages.rb
@@ -1716,6 +1716,9 @@ def FindAndRememberAddOnProductsFiles(initial_repository)
     end
 
     def Initialize_StageInitial(show_popup, base_url, log_url, product_dir = "/")
+      # if there are multiple repositories on the medium we cannot initialize it now,
+      # we do not know which base product (repository) will be used, it will be selected
+      # by the user later in the workflow
       if !meta_data_present(base_url, product_dir)
         log.info "Metadata not found at #{log_url}, postponing the repository initialization"
         return
diff --git a/test/product_location_test.rb b/test/product_location_test.rb
index 834596adb..30a357460 100644
--- a/test/product_location_test.rb
+++ b/test/product_location_test.rb
@@ -5,13 +5,13 @@
 
 require "y2packager/product_location"
 
-# a helper method
+# a helper method, find a specified product in the list
 def find_product(arr, product)
   arr.find { |p| p.product_name == product }
 end
 
 # loading all repositories and evaluating the product dependencies
-# using  the solver takes some time, run it only once and cache
+# using the solver takes some time, run it only once and cache
 # the result for all tests
 dir = File.join(__dir__, "data/zypp/test_offline_repo")
 repo_url = "dir://#{URI.escape(dir)}"