Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix log copy #252

Merged
merged 9 commits into from
Jan 7, 2015
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions package/yast2-installation.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Wed Jan 7 14:27:28 UTC 2015 - jreidinger@suse.com

- do not stuck during copy of logs files (bnc#897091)
- 3.1.126

-------------------------------------------------------------------
Thu Dec 18 20:12:47 UTC 2014 - lslezak@suse.cz

Expand Down
6 changes: 3 additions & 3 deletions package/yast2-installation.spec
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@


Name: yast2-installation
Version: 3.1.125
Version: 3.1.126
Release: 0

BuildRoot: %{_tmppath}/%{name}-%{version}-build
Expand All @@ -37,8 +37,8 @@ BuildRequires: docbook-xsl-stylesheets libxslt update-desktop-files yast2-core-
BuildRequires: yast2-devtools >= 3.1.10
BuildRequires: rubygem(rspec)

# Linuxrc.keys
BuildRequires: yast2 >= 3.1.41
# Base clients for inst clients
BuildRequires: yast2 >= 3.1.112

# Yast::Remote
BuildRequires: yast2-network
Expand Down
1 change: 1 addition & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ ylibdir = "${yast2dir}/lib/installation"
ylib_DATA = \
lib/installation/cio_ignore.rb \
lib/installation/clone_settings.rb \
lib/installation/copy_logs_finish.rb \
lib/installation/minimal_installation.rb \
lib/installation/prep_shrink.rb \
lib/installation/remote_finish_client.rb
Expand Down
143 changes: 2 additions & 141 deletions src/clients/copy_logs_finish.rb
Original file line number Diff line number Diff line change
@@ -1,142 +1,3 @@
# encoding: utf-8
require "installation/copy_logs_finish"

# ------------------------------------------------------------------------------
# Copyright (c) 2006-2012 Novell, Inc. All Rights Reserved.
#
#
# This program is free software; you can redistribute it and/or modify it under
# the terms of version 2 of the GNU General Public License as published by the
# Free Software Foundation.
#
# This program is distributed in the hope that it will be useful, but WITHOUT
# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License along with
# this program; if not, contact Novell, Inc.
#
# To contact Novell about this file by physical or electronic mail, you may find
# current contact information at www.novell.com.
# ------------------------------------------------------------------------------

# File:
# copy_logs_finish.ycp
#
# Module:
# Step of base installation finish
#
# Authors:
# Jiri Srain <jsrain@suse.cz>
#
# $Id$
#
module Yast
class CopyLogsFinishClient < Client
include Yast::Logger

def main
Yast.import "UI"

textdomain "installation"

Yast.import "Directory"
Yast.include self, "installation/misc.rb"

@ret = nil
@func = ""
@param = {}

# Check arguments
if Ops.greater_than(Builtins.size(WFM.Args), 0) &&
Ops.is_string?(WFM.Args(0))
@func = Convert.to_string(WFM.Args(0))
if Ops.greater_than(Builtins.size(WFM.Args), 1) &&
Ops.is_map?(WFM.Args(1))
@param = Convert.to_map(WFM.Args(1))
end
end

log.info "starting copy_logs_finish"
log.debug "func=#{@func}"
log.debug "param=#{@param}"

if @func == "Info"
return {
"steps" => 1,
# progress step title
"title" => _(
"Copying log files to installed system..."
),
"when" => [:installation, :live_installation, :update, :autoinst]
}
elsif @func == "Write"
@log_files = Convert.convert(
WFM.Read(path(".local.dir"), Directory.logdir),
:from => "any",
:to => "list <string>"
)

Builtins.foreach(@log_files) do |file|
log.debug "Processing file #{file}"

if file == "y2log" || Builtins.regexpmatch(file, "^y2log-[0-9]+$")
# Prepare y2log, y2log-* for log rotation

target_no = 1

if Ops.greater_than(Builtins.size(file), Builtins.size("y2log-"))
target_no = Ops.add(
1,
Builtins.tointeger(
Builtins.substring(file, Builtins.size("y2log-"), 5)
)
)
end

target_basename = Builtins.sformat("y2log-%1", target_no)
InjectRenamedFile(Directory.logdir, file, target_basename)

compress_cmd = Builtins.sformat(
"gzip %1/%2/%3",
Installation.destdir,
Directory.logdir,
target_basename
)
log.debug "Compress command: #{compress_cmd}"
WFM.Execute(path(".local.bash"), compress_cmd)
elsif Builtins.regexpmatch(file, "^y2log-[0-9]+\\.gz$")
target_no = Ops.add(
1,
Builtins.tointeger(
Builtins.regexpsub(file, "y2log-([0-9]+)\\.gz", "\\1")
)
)
InjectRenamedFile(
Directory.logdir,
file,
Builtins.sformat("y2log-%1.gz", target_no)
)
elsif file == "zypp.log"
# Save zypp.log from the inst-sys
InjectRenamedFile(Directory.logdir, file, "zypp.log-1") # not y2log, y2log-*
else
InjectFile(Ops.add(Ops.add(Directory.logdir, "/"), file))
end
end

copy_cmd = "/bin/cp /var/log/pbl.log '#{Installation.destdir}/#{Directory.logdir}/pbl-instsys.log'"
log.debug "Copy command: #{copy_cmd}"
WFM.Execute(path(".local.bash"), copy_cmd)
else
log.error "unknown function: #{@func}"
@ret = nil
end

log.debug "ret=#{@ret}"
log.info "copy_logs_finish finished"
deep_copy(@ret)
end
end
end

Yast::CopyLogsFinishClient.new.main
::Installation::CopyLogsFinish.run
13 changes: 0 additions & 13 deletions src/include/installation/misc.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,6 @@ def InjectFile(filename)
#return SCR::Write (.target.byte, filename, copy_buffer);
end


def InjectRenamedFile(dir, src_name, target_name)
src = "#{dir}/#{src_name}"
target = "#{Installation.destdir}/#{dir}/#{target_name}"
command = "/bin/cp #{src} #{target}"

Builtins.y2milestone("InjectRenamedFile: <%1> -> <%2>", src, target)
Builtins.y2debug("Inject command: #{command}")

WFM.Execute(path(".local.bash"), command)
nil
end

def UpdateWizardSteps
wizard_mode = Mode.mode
Builtins.y2milestone("Switching Steps to %1 ", wizard_mode)
Expand Down
107 changes: 107 additions & 0 deletions src/lib/installation/copy_logs_finish.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# ------------------------------------------------------------------------------
# Copyright (c) 2006-2015 Novell, Inc. All Rights Reserved.
#
#
# This program is free software; you can redistribute it and/or modify it under
# the terms of version 2 of the GNU General Public License as published by the
# Free Software Foundation.
#
# This program is distributed in the hope that it will be useful, but WITHOUT
# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License along with
# this program; if not, contact Novell, Inc.
#
# To contact Novell about this file by physical or electronic mail, you may find
# current contact information at www.novell.com.
# ------------------------------------------------------------------------------

require "yast"

require "installation/finish_client"

module Installation
class CopyLogsFinish < ::Installation::FinishClient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking. Do we have any convention on naming all these clients living under lib/installation? Like always using WhateverFinish for clients inheriting from FinishClient?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually no convention...in bootloader it is simple Bootloader/FinishClient but here it can create collisions so I name it this way. CopyLogs without finish can confuse reader as it can be seen as common class to copy logs from one place to another.

include Yast::I18n

LOCAL_BASH = Yast::Path.new(".local.bash")

def initialize
textdomain "installation"

Yast.import "Directory"
Yast.import "Installation"
end

def steps
1
end

def title
_("Copying log files to installed system...")
end

def modes
[:installation, :live_installation, :update, :autoinst]
end

def write
log_files = Yast::WFM.Read(Yast::Path.new(".local.dir"), Yast::Directory.logdir)

log_files.each do |file|
log.debug "Processing file #{file}"

case file
when "y2log", /\Ay2log-\d+\z/
# Prepare y2log, y2log-* for log rotation
target_no = 1

if file != "y2log"
prefix_size = "y2log-".size
target_no = file[prefix_size..-1].to_i + 1
end

target_basename = "y2log-#{target_no}"
copy_log_to_target(file, target_basename)

target_path = ::File.join(
Yast::Installation.destdir,
Yast::Directory.logdir,
target_basename
)
compress_cmd = "gzip -f #{target_path}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a comment here to justify the -f (with a reference to the bug report)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea 👍

log.debug "Compress command: #{compress_cmd}"
Yast::WFM.Execute(LOCAL_BASH, compress_cmd)
when /\Ay2log-\d+\.gz\z/
target_no = file[/y2log-(\d+)/, 1].to_i + 1
copy_log_to_target(file, "y2log-#{target_no}.gz")
when "zypp.log"
# Save zypp.log from the inst-sys
copy_log_to_target(file, "zypp.log-1") # not y2log, y2log-*
else
copy_log_to_target(file)
end
end

copy_cmd = "/bin/cp /var/log/pbl.log '#{Yast::Installation.destdir}/#{Yast::Directory.logdir}/pbl-instsys.log'"
log.debug "Copy command: #{copy_cmd}"
Yast::WFM.Execute(LOCAL_BASH, copy_cmd)

nil
end

private

def copy_log_to_target(src_file, dst_file = src_file)
dir = Yast::Directory.logdir
src_path = "#{dir}/#{src_file}"
dst_path = "#{Yast::Installation.destdir}/#{dir}/#{dst_file}"
command = "/bin/cp #{src_path} #{dst_path}"

log.info "copy log with '#{command}'"

Yast::WFM.Execute(LOCAL_BASH, command)
end
end
end
1 change: 1 addition & 0 deletions test/Makefile.am
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
TESTS = \
inst_functions_test.rb \
cio_ignore_test.rb \
copy_logs_finish_test.rb \
prep_shrink_test.rb \
remote_finish_test.rb

Expand Down
53 changes: 53 additions & 0 deletions test/copy_logs_finish_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#! /usr/bin/env rspec

require_relative "./test_helper"

require "installation/copy_logs_finish"

describe ::Installation::CopyLogsFinish do
describe "#write" do
before do
allow(Yast::WFM).to receive(:Execute)
end

it "copies logs from instalation to target system" do
allow(Yast::WFM).to receive(:Read).and_return(["y2start.log"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without reading the implementation code, the purpose of this allow (returning a list of log files) is not obvious (no with part). Moreover, it's repeated in every it.

What about using let or some specific helper method to label it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also considering it and if you get same idea I will do it.


expect(Yast::WFM).to receive(:Execute).with(anything(), /cp/).at_least(:once)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The y2start.log returned by the previous call is not used in the expectation. Feels strange.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair comment. I will add it.


subject.write
end

it "rotate y2log" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rotates :-)

allow(Yast::WFM).to receive(:Read).and_return(["y2log-1.gz"])

expect(Yast::WFM).to receive(:Execute).with(anything(), /cp .*y2log-1.gz .*y2log-2.gz/)

subject.write
end

it "compress y2log if not already done" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"compresses"... and "does" (in the next one) and "rotates" (last one).

allow(Yast::WFM).to receive(:Read).and_return(["y2log-1"])

expect(Yast::WFM).to receive(:Execute).with(anything(), /gzip .*y2log-2/) #-2 due to rotation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(Yast::WFM).to receive(:Execute).with(anything(), ...) is also quite long and repeated several times, so it maybe deserves a helper method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, maybe time to finally find time to have generic rspec helper for SCR in yast2 or ruby bindings


subject.write
end

it "do not stuck during compress if file already exists (bnc#897091)" do
allow(Yast::WFM).to receive(:Read).and_return(["y2log-1"])

expect(Yast::WFM).to receive(:Execute).with(anything(), /gzip -f/)

subject.write
end

it "rotate zypp.log" do
allow(Yast::WFM).to receive(:Read).and_return(["zypp.log"])

expect(Yast::WFM).to receive(:Execute).with(anything(), /cp .*zypp.log .*zypp.log-1/)

subject.write
end
end
end