From 93c42881570ad62a0816c3f3846d38e781cce7b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Alejandro=20Anderssen=20Gonz=C3=A1lez?= Date: Mon, 20 Aug 2018 10:37:50 +0100 Subject: [PATCH 1/4] AutoInstallRules: increased maxdepth default for merging profiles Cleaned the Merge method and prepared for moving the responsibility to a specific class in the future (currently AutoinstClass and AutoInstallRules shared some duplicated code) --- src/modules/AutoInstallRules.rb | 98 ++++++++++++++------------------- src/modules/AutoinstClass.rb | 5 +- test/AutoInstallRules_test.rb | 71 ++++++++++++++++++++++++ test/AutoinstClass_test.rb | 4 +- 4 files changed, 118 insertions(+), 60 deletions(-) diff --git a/src/modules/AutoInstallRules.rb b/src/modules/AutoInstallRules.rb index 6fcdb10e6..46462dad6 100644 --- a/src/modules/AutoInstallRules.rb +++ b/src/modules/AutoInstallRules.rb @@ -874,79 +874,70 @@ def GetRules end end + #TODO: Move the responsibility of merging profiles to a specific class + # removing also the duplication of code between this module and the + # AutoinstClass one. + + MERGE_CMD = "/usr/bin/xsltproc".freeze + MERGE_DEFAULTS = "--novalid --maxdepth 10000 --param replace \"'false'\"".freeze + MERGE_XSLT_PATH = "/usr/share/autoinstall/xslt/merge.xslt".freeze + + # Merges the given profiles + # + # @param base_profile [String] the base profile file path + # @param with [String] the profile to be merged file path + # @param to [String] the resulting control file path + # @return [Hash] stdout and stderr output + def merge_profiles(base_profile, with, to) + dontmerge_str = "" + AutoinstConfig.dontmerge.each_with_index do |dm, i| + dontmerge_str << " --param dontmerge#{i+1} \"'#{dm}'\"" + end + merge_command = + "#{MERGE_CMD} #{MERGE_DEFAULTS}" \ + "#{dontmerge_str} --param with \"'#{with}'\" " \ + "--output #{to} " \ + "#{MERGE_XSLT_PATH} #{base_profile}" + + out = SCR.Execute(path(".target.bash_output"), merge_command, {}) + Builtins.y2milestone("Merge command: #{merge_command}") + Builtins.y2milestone("Merge stdout: #{out["stdout"]}, stderr: #{out["stderr"]}") + out + end # Merge Rule results # @param [String] result_profile the resulting control file path # @return [Boolean] true on success def Merge(result_profile) - tmpdir = AutoinstConfig.tmpDir + base_profile = File.join(AutoinstConfig.tmpdir, "base_profile.xml") + merge_profile = File.join(AutoinstConfig.tmpdir, "result.xml") ok = true skip = false error = false - - base_profile = Ops.add(tmpdir, "/base_profile.xml") - - Builtins.foreach(@tomerge) do |file| + @tomerge.each do |file| Builtins.y2milestone("Working on file: %1", file) - current_profile = Ops.add( - Ops.add(AutoinstConfig.local_rules_location, "/"), - file - ) + current_profile = File.join(AutoinstConfig.local_rules_location, file) if !skip - if !XML_cleanup(current_profile, Ops.add(tmpdir, "/base_profile.xml")) + if !XML_cleanup(current_profile, base_profile) Builtins.y2error("Error reading XML file") message = _( "The XML parser reported an error while parsing the autoyast profile. The error message is:\n" ) - message = Ops.add(message, XML.XMLError) - Popup.Error(message) + Popup.Error(message + XML.XMLError) error = true end skip = true elsif !error - _MergeCommand = "/usr/bin/xsltproc --novalid --param replace \"'false'\" " - dontmerge_str = "" - i = 1 - Builtins.foreach(AutoinstConfig.dontmerge) do |dm| - dontmerge_str = Ops.add( - dontmerge_str, - Builtins.sformat(" --param dontmerge%1 \"'%2'\" ", i, dm) - ) - i = Ops.add(i, 1) - end - _MergeCommand = Ops.add(_MergeCommand, dontmerge_str) - - _MergeCommand = Ops.add(_MergeCommand, "--param with ") - _MergeCommand = Ops.add( - Ops.add(Ops.add(_MergeCommand, "\"'"), current_profile), - "'\" " - ) - _MergeCommand = Ops.add( - Ops.add(Ops.add(_MergeCommand, "--output "), tmpdir), - "/result.xml" - ) - _MergeCommand = Ops.add( - _MergeCommand, - " /usr/share/autoinstall/xslt/merge.xslt " - ) - _MergeCommand = Ops.add(Ops.add(_MergeCommand, base_profile), " ") + xsltret = merge_profiles(base_profile, current_profile, merge_profile) - Builtins.y2milestone("Merge command: %1", _MergeCommand) - xsltret = Convert.to_map( - SCR.Execute(path(".target.bash_output"), _MergeCommand) - ) Builtins.y2milestone("Merge result: %1", xsltret) - if Ops.get_integer(xsltret, "exit", -1) != 0 || - Ops.get_string(xsltret, "stderr", "") != "" + if xsltret["exit"] != 0 || xsltret.fetch("stderr", "") != "" Builtins.y2error("Merge Failed") - StdErrLog(Ops.get_string(xsltret, "stderr", "")) + StdErrLog(xsltret["stderr"]) ok = false end - XML_cleanup( - Ops.add(tmpdir, "/result.xml"), - Ops.add(tmpdir, "/base_profile.xml") - ) + XML_cleanup(merge_profile, base_profile) else Builtins.y2error("Error while merging control files") end @@ -954,14 +945,7 @@ def Merge(result_profile) return !error if error - SCR.Execute( - path(".target.bash"), - Ops.add( - Ops.add(Ops.add("cp ", tmpdir), "/base_profile.xml "), - result_profile - ) - ) - + SCR.Execute(path(".target.bash"), "cp #{base_profile} #{result_profile}") Builtins.y2milestone("Ok=%1", ok) @dontmergeIsDefault = true AutoinstConfig.dontmerge = deep_copy(@dontmergeBackup) diff --git a/src/modules/AutoinstClass.rb b/src/modules/AutoinstClass.rb index 47196517d..19b6c3f75 100644 --- a/src/modules/AutoinstClass.rb +++ b/src/modules/AutoinstClass.rb @@ -112,6 +112,9 @@ def AutoinstClass nil end + MERGE_CMD = "/usr/bin/xsltproc".freeze + MERGE_DEFAULTS = "--novalid --maxdepth 10000 --param replace \"'false'\"".freeze + # Merge classes # def MergeClasses(configuration, base_profile, resultFileName) @@ -120,7 +123,7 @@ def MergeClasses(configuration, base_profile, resultFileName) dontmerge_str << " --param dontmerge#{i+1} \"'#{dm}'\" " end merge_command = - "/usr/bin/xsltproc --novalid --param replace \"'false'\" #{dontmerge_str} --param with " \ + "#{MERGE_CMD} #{MERGE_DEFAULTS} #{dontmerge_str} --param with " \ "\"'#{findPath(configuration["name"], configuration["class"])}'\" " \ "--output #{File.join(AutoinstConfig.tmpDir, resultFileName)} " \ "#{MERGE_XSLT_PATH} #{base_profile} " diff --git a/test/AutoInstallRules_test.rb b/test/AutoInstallRules_test.rb index b2c95fd83..17741936a 100755 --- a/test/AutoInstallRules_test.rb +++ b/test/AutoInstallRules_test.rb @@ -208,4 +208,75 @@ end end end + + describe '#merge_profiles' do + let(:base_profile_path) { File.join(FIXTURES_PATH, 'profiles', 'partitions.xml') } + let(:tmp_dir) { File.join(root_path, 'tmp') } + let(:expected_xml) { File.read(expected_xml_path) } + let(:output_path) { File.join(tmp_dir, 'output.xml') } + let(:to_merge_path) { File.join(FIXTURES_PATH, 'classes', 'swap', 'largeswap.xml') } + let(:output_xml) { File.read(output_path) } + let(:dontmerge) { [] } + let(:merge_xslt_path) { File.join(root_path, 'xslt', 'merge.xslt') } + let(:xsltproc_command) { + "/usr/bin/xsltproc --novalid --maxdepth 10000 --param replace \"'false'\" " \ + "--param with \"'#{to_merge_path}'\" "\ + "--output #{output_path} " \ + "#{merge_xslt_path} #{base_profile_path}" + } + + before(:each) do + stub_const("Yast::AutoInstallRulesClass::MERGE_XSLT_PATH", merge_xslt_path) + end + + around(:each) do |example| + FileUtils.rm_rf(tmp_dir) if Dir.exist?(tmp_dir) + FileUtils.mkdir(tmp_dir) + example.run + FileUtils.rm_rf(tmp_dir) + end + + before(:each) do + allow(Yast::AutoinstConfig).to receive(:tmpDir).and_return(tmp_dir) + allow(Yast::AutoinstConfig).to receive(:dontmerge).and_return(dontmerge) + subject.Files + end + + it 'executes xsltproc and returns a hash with info about the result' do + expect(Yast::SCR).to receive(:Execute). + with(Yast::Path.new(".target.bash_output"), xsltproc_command, {}).and_call_original + out = subject.merge_profiles(base_profile_path, to_merge_path, output_path) + expect(out).to eq({ 'exit' => 0, 'stderr' => '', 'stdout' => '' }) + end + + context 'when all elements must be merged' do + let(:expected_xml_path) { File.join(root_path, 'test', 'fixtures', 'output', 'partitions-merged.xml') } + + it 'merges elements from the base profile and the rule profile' do + expect(Yast::SCR).to receive(:Execute). + with(Yast::Path.new(".target.bash_output"), xsltproc_command, {}).and_call_original + out = subject.merge_profiles(base_profile_path, to_merge_path, output_path) + expect(output_xml).to eq(expected_xml) + end + end + + context 'when some elements are not intended to be merged' do + let(:expected_xml_path) { File.join(root_path, 'test', 'fixtures', 'output', 'partitions-dontmerge.xml') } + let(:dontmerge) { ['partition'] } + let(:xsltproc_command) { + "/usr/bin/xsltproc --novalid --maxdepth 10000 --param replace \"'false'\" " \ + "--param dontmerge1 \"'partition'\" " \ + "--param with \"'#{to_merge_path}'\" "\ + "--output #{output_path} " \ + "#{merge_xslt_path} #{base_profile_path}" + } + + it 'does not merge those elements' do + expect(Yast::SCR).to receive(:Execute). + with(Yast::Path.new(".target.bash_output"), xsltproc_command, {}).and_call_original + out = subject.merge_profiles(base_profile_path, to_merge_path, output_path) + expect(output_xml).to eq(expected_xml) + end + end + end end diff --git a/test/AutoinstClass_test.rb b/test/AutoinstClass_test.rb index 6691fa8be..d282c53f0 100755 --- a/test/AutoinstClass_test.rb +++ b/test/AutoinstClass_test.rb @@ -222,7 +222,7 @@ let(:merge_xslt_path) { File.join(ROOT_PATH, 'xslt', 'merge.xslt') } let(:conf_to_merge) { { "class" => "swap", "name" => "largeswap.xml" } } let(:xsltproc_command) { - "/usr/bin/xsltproc --novalid --param replace \"'false'\" " \ + "/usr/bin/xsltproc --novalid --maxdepth 10000 --param replace \"'false'\" " \ "--param with \"'#{subject.findPath("largeswap.xml", "swap")}'\" "\ "--output #{File.join(tmp_dir, "output.xml")} " \ "#{merge_xslt_path} #{base_profile_path} " @@ -267,7 +267,7 @@ let(:expected_xml_path) { File.join(ROOT_PATH, 'test', 'fixtures', 'output', 'partitions-dontmerge.xml') } let(:dontmerge) { ['partition'] } let(:xsltproc_command) { - "/usr/bin/xsltproc --novalid --param replace \"'false'\" " \ + "/usr/bin/xsltproc --novalid --maxdepth 10000 --param replace \"'false'\" " \ "--param dontmerge1 \"'partition'\" " \ "--param with \"'#{subject.findPath("largeswap.xml", "swap")}'\" "\ "--output #{File.join(tmp_dir, "output.xml")} " \ From d719300d87defdbf2914ae5f0e45c8563d69a223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Alejandro=20Anderssen=20Gonz=C3=A1lez?= Date: Mon, 20 Aug 2018 10:46:05 +0100 Subject: [PATCH 2/4] Bump version & changelog. --- package/autoyast2.changes | 7 +++++++ package/autoyast2.spec | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/package/autoyast2.changes b/package/autoyast2.changes index a234ffae1..24383b6d9 100644 --- a/package/autoyast2.changes +++ b/package/autoyast2.changes @@ -1,3 +1,10 @@ +------------------------------------------------------------------- +Fri Aug 17 18:11:33 UTC 2018 - knut.anderssen@suse.com + +- AutoInstallRules: increased default maxdepth for not crashing + with a big software package list (bsc#1104655) +- 4.0.60 + ------------------------------------------------------------------- Fri Aug 17 09:46:05 CEST 2018 - schubi@suse.de diff --git a/package/autoyast2.spec b/package/autoyast2.spec index 38d2013a1..4cc3e71aa 100644 --- a/package/autoyast2.spec +++ b/package/autoyast2.spec @@ -22,7 +22,7 @@ %endif Name: autoyast2 -Version: 4.0.59 +Version: 4.0.60 Release: 0 BuildRoot: %{_tmppath}/%{name}-%{version}-build From a8fe3887dbf571f61b04f033ebfc3826e8ea6132 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Alejandro=20Anderssen=20Gonz=C3=A1lez?= Date: Mon, 20 Aug 2018 11:22:09 +0100 Subject: [PATCH 3/4] Changes based on CR. --- src/modules/AutoInstallRules.rb | 18 +++++++++--------- test/AutoInstallRules_test.rb | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/modules/AutoInstallRules.rb b/src/modules/AutoInstallRules.rb index 46462dad6..cd3b64db3 100644 --- a/src/modules/AutoInstallRules.rb +++ b/src/modules/AutoInstallRules.rb @@ -896,12 +896,12 @@ def merge_profiles(base_profile, with, to) merge_command = "#{MERGE_CMD} #{MERGE_DEFAULTS}" \ "#{dontmerge_str} --param with \"'#{with}'\" " \ - "--output #{to} " \ + "--output \"#{to}\" " \ "#{MERGE_XSLT_PATH} #{base_profile}" out = SCR.Execute(path(".target.bash_output"), merge_command, {}) - Builtins.y2milestone("Merge command: #{merge_command}") - Builtins.y2milestone("Merge stdout: #{out["stdout"]}, stderr: #{out["stderr"]}") + log.info("Merge command: #{merge_command}") + log.info("Merge stdout: #{out["stdout"]}, stderr: #{out["stderr"]}") out end @@ -915,11 +915,11 @@ def Merge(result_profile) skip = false error = false @tomerge.each do |file| - Builtins.y2milestone("Working on file: %1", file) + log.info("Working on file: %1", file) current_profile = File.join(AutoinstConfig.local_rules_location, file) if !skip if !XML_cleanup(current_profile, base_profile) - Builtins.y2error("Error reading XML file") + log.error("Error reading XML file") message = _( "The XML parser reported an error while parsing the autoyast profile. The error message is:\n" ) @@ -930,16 +930,16 @@ def Merge(result_profile) elsif !error xsltret = merge_profiles(base_profile, current_profile, merge_profile) - Builtins.y2milestone("Merge result: %1", xsltret) + log.info("Merge result: %1", xsltret) if xsltret["exit"] != 0 || xsltret.fetch("stderr", "") != "" - Builtins.y2error("Merge Failed") - StdErrLog(xsltret["stderr"]) + log.error("Merge Failed") + StdErrLog(xsltret.fetch("stderr", "")) ok = false end XML_cleanup(merge_profile, base_profile) else - Builtins.y2error("Error while merging control files") + log.error("Error while merging control files") end end diff --git a/test/AutoInstallRules_test.rb b/test/AutoInstallRules_test.rb index 17741936a..4c6e88d22 100755 --- a/test/AutoInstallRules_test.rb +++ b/test/AutoInstallRules_test.rb @@ -221,7 +221,7 @@ let(:xsltproc_command) { "/usr/bin/xsltproc --novalid --maxdepth 10000 --param replace \"'false'\" " \ "--param with \"'#{to_merge_path}'\" "\ - "--output #{output_path} " \ + "--output \"#{output_path}\" " \ "#{merge_xslt_path} #{base_profile_path}" } @@ -267,7 +267,7 @@ "/usr/bin/xsltproc --novalid --maxdepth 10000 --param replace \"'false'\" " \ "--param dontmerge1 \"'partition'\" " \ "--param with \"'#{to_merge_path}'\" "\ - "--output #{output_path} " \ + "--output \"#{output_path}\" " \ "#{merge_xslt_path} #{base_profile_path}" } From 146d6c566bf05d3fca9eeb35db7a42da0a82f78c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Alejandro=20Anderssen=20Gonz=C3=A1lez?= Date: Mon, 20 Aug 2018 11:33:38 +0100 Subject: [PATCH 4/4] Use the new Popup Api for showing the XML errors --- src/modules/AutoInstallRules.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/modules/AutoInstallRules.rb b/src/modules/AutoInstallRules.rb index cd3b64db3..188689825 100644 --- a/src/modules/AutoInstallRules.rb +++ b/src/modules/AutoInstallRules.rb @@ -7,6 +7,7 @@ # # $Id$ require "yast" +require "yast2/popup" require "y2storage" module Yast @@ -923,7 +924,8 @@ def Merge(result_profile) message = _( "The XML parser reported an error while parsing the autoyast profile. The error message is:\n" ) - Popup.Error(message + XML.XMLError) + message += XML.XMLError + Yast2::Popup.show(message, headline: :error) error = true end skip = true