Factored out required vs. optional used features

src/lib/y2storage/clients/inst_disk_proposal.rb

@@ -162,23 +162,7 @@ def actions_after_partitioner(devicegraph, dialog_result)
       # Add storage-related software packages (filesystem tools etc.) to the
       # set of packages to be installed.
       def add_storage_packages
-        features = storage_manager.staging.used_features
-        required_features = storage_manager.staging.used_features(required_only: true)
-
-        required_packages = required_features.pkg_list
-        optional_packages = features.pkg_list - required_packages
-
-        set_proposal_packages(required_packages, false)
-        set_proposal_packages(optional_packages, true)
-      end
-
-      # @see #add_storage_packages
-      #
-      # @param pkgs [Array<String>] list of packages
-      # @param optional [Boolean] whether the packages in the list are optional
-      def set_proposal_packages(pkgs, optional)
-        pkg_handler = Y2Storage::PackageHandler.new(pkgs, optional: optional)
-        pkg_handler.set_proposal_packages
+        Y2Storage::PackageHandler.set_proposal_packages_for(storage_manager.staging)
       end
 
       # Save the list of filesystem to /etc/sysconfig/storage.

src/lib/y2storage/clients/partitions_proposal.rb

@@ -120,8 +120,9 @@ def update_state
           staging = storage_manager.staging
           actiongraph = staging ? staging.actiongraph : nil
           self.actions_presenter = ActionsPresenter.new(actiongraph)
-          Y2Storage::DumpManager.dump(staging)
-          Y2Storage::DumpManager.dump(actions_presenter)
+          DumpManager.dump(staging)
+          DumpManager.dump(actions_presenter)
+          PackageHandler.set_proposal_packages_for(staging)
         end
       end
 

src/lib/y2storage/devicegraph.rb

@@ -615,6 +615,23 @@ def used_features(required_only: false)
       StorageFeaturesList.from_bitfield(storage_used_features(type))
     end
 
+    # List of required (mandatory) storage features used by the devicegraph
+    #
+    # @return [StorageFeaturesList]
+    def required_used_features
+      used_features(required_only: true)
+    end
+
+    # List of optional storage features used by the devicegraph
+    #
+    # @return [StorageFeaturesList]
+    def optional_used_features
+      all = storage_used_features(Storage::UsedFeaturesDependencyType_SUGGESTED)
+      required = storage_used_features(Storage::UsedFeaturesDependencyType_REQUIRED)
+      # Using binary XOR in those bit fields to calculate the difference
+      StorageFeaturesList.from_bitfield(all ^ required)
+    end
+
     private
 
     # Copy of a device tree where hashes have been substituted by sorted

src/lib/y2storage/package_handler.rb

@@ -116,6 +116,21 @@ def set_proposal_packages
       success
     end
 
+    # Add the proposal packages for storage that are needed for the specified
+    # devicegraph's used features. This marks the packages for installation;
+    # it does not install them yet.
+    #
+    # @param devicegraph [Devicegraph] usually StorageManager.instance.staging
+    # @param optional
+    def self.set_proposal_packages_for(devicegraph, optional: true)
+      required_packages = devicegraph.required_used_features.pkg_list
+      PackageHandler.new(required_packages, optional: false).set_proposal_packages
+      return unless optional
+
+      optional_packages = devicegraph.optional_used_features.pkg_list
+      PackageHandler.new(optional_packages, optional: true).set_proposal_packages
+    end
+
     private
 
     # Whether the packages should be considered as optional when adding them to the

test/y2storage/devicegraph_test.rb

@@ -1331,4 +1331,54 @@ def with_sda2_deleted(initial_graph)
       end
     end
   end
+
+  describe "#required_used_features" do
+    before { fake_scenario(scenario) }
+
+    context "with local devices combining several filesystem types" do
+      let(:scenario) { "mixed_disks" }
+
+      it "returns only the features for mounted filesystems" do
+        features = fake_devicegraph.required_used_features
+        expect(features).to be_a Y2Storage::StorageFeaturesList
+        expect(features.map(&:id))
+          .to contain_exactly(:UF_BTRFS, :UF_XFS, :UF_SWAP)
+      end
+    end
+
+    context "with an empty disk" do
+      let(:scenario) { "empty_disks" }
+
+      it "Survives not having any storage features" do
+        features = fake_devicegraph.required_used_features
+        expect(features).to be_a Y2Storage::StorageFeaturesList
+        expect(features.map(&:id)).to be == []
+      end
+    end
+  end
+
+  describe "#optional_used_features" do
+    before { fake_scenario(scenario) }
+
+    context "with local devices combining several filesystem types" do
+      let(:scenario) { "mixed_disks" }
+
+      it "returns only the features for filesystems that are not mounted" do
+        features = fake_devicegraph.optional_used_features
+        expect(features).to be_a Y2Storage::StorageFeaturesList
+        expect(features.map(&:id))
+          .to contain_exactly(:UF_EXT4, :UF_NTFS)
+      end
+    end
+
+    context "with an empty disk" do
+      let(:scenario) { "empty_disks" }
+
+      it "Survives not having any storage features" do
+        features = fake_devicegraph.optional_used_features
+        expect(features).to be_a Y2Storage::StorageFeaturesList
+        expect(features.map(&:id)).to be == []
+      end
+    end
+  end
 end

test/y2storage/package_handler_test.rb

@@ -135,4 +135,46 @@
       end
     end
   end
+
+  describe "#set_proposal_packages_for" do
+    before do
+      fake_scenario(scenario)
+      allow(Yast::Mode).to receive(:installation).and_return(installation)
+      allow(Yast::Package).to receive(:Installed).and_return(false)
+      allow(Yast::Package).to receive(:Available).and_return(true)
+    end
+
+    context "with local devices combining several filesystem types" do
+      PROPOSAL_ID = "storage_proposal"
+      let(:scenario) { "mixed_disks" }
+      let(:installation) { true }
+
+      it "Adds the correct required and optional storage packages to the proposal" do
+        expect(Yast::PackagesProposal).to receive(:SetResolvables)
+          .with(PROPOSAL_ID, :package, ["btrfsprogs", "e2fsprogs", "xfsprogs"], optional: false)
+          .and_return true
+        expect(Yast::PackagesProposal).to receive(:SetResolvables)
+          .with(PROPOSAL_ID, :package, ["e2fsprogs", "ntfs-3g", "ntfsprogs"], optional: true)
+          .and_return true
+        described_class.set_proposal_packages_for(fake_devicegraph)
+      end
+
+      it "Adds only required storage packages to the proposal if 'optional' is 'false" do
+        expect(Yast::PackagesProposal).to receive(:SetResolvables)
+          .with(PROPOSAL_ID, :package, ["btrfsprogs", "e2fsprogs", "xfsprogs"], optional: false)
+          .and_return true
+        described_class.set_proposal_packages_for(fake_devicegraph, optional: false)
+      end
+    end
+
+    context "with empty disks" do
+      let(:scenario) { "empty_disks" }
+      let(:installation) { true }
+
+      it "Does not add any storage packages to the proposal" do
+        expect(Yast::PackagesProposal).not_to receive(:SetResolvables)
+        described_class.set_proposal_packages_for(fake_devicegraph, optional: true)
+      end
+    end
+  end
 end