Please review pull request #648: master - Windows and Ruby opened by (daniel-pittman)
Description:
The 1.9.3 changes on master broke some of our Windows support, and showed some other shortfalls in our platform abstractions. This set of commits brings that up to speed on Windows Ruby 1.8.7, our supported stable Windows platform.
This has partial 1.9.3 compatibility - there are some known test failures where it looks like Win32API related changes might have broken our code. We only officially support 1.8.7 on Windows at the moment, and at least this moves us forward to actually testing the code effectively, so I would rather get it in now than delay longer.
- Opened: Mon Apr 09 22:58:06 UTC 2012
- Based on: puppetlabs:master (a199c23baffcd5cdcff27d74cf043317baef0ab0)
- Requested merge: daniel-pittman:maint/master/ruby-1.9.3-support (e662de55298ceeb02d92cf7d81a3f851a05483c0)
Diff follows:
diff --git a/lib/puppet/feature/base.rb b/lib/puppet/feature/base.rb
index a73fdb5..cb05675 100644
--- a/lib/puppet/feature/base.rb
+++ b/lib/puppet/feature/base.rb
@@ -16,6 +16,8 @@
# We can use Microsoft Windows functions
Puppet.features.add(:microsoft_windows) do
begin
+ require 'Win32API' # case matters in this require!
+ require 'win32ole'
require 'sys/admin'
require 'win32/process'
require 'win32/dir'
diff --git a/lib/puppet/parser/functions/generate.rb b/lib/puppet/parser/functions/generate.rb
index cf9166f..0463350 100644
--- a/lib/puppet/parser/functions/generate.rb
+++ b/lib/puppet/parser/functions/generate.rb
@@ -14,7 +14,7 @@
raise Puppet::ParseError, "Generators must be fully qualified" unless Puppet::Util.absolute_path?(args[0])
if Puppet.features.microsoft_windows?
- valid = args[0] =~ /^[a-z]:(?:[\/\\][\w.-]+)+$/i
+ valid = args[0] =~ /^[a-z]:(?:[\/\\][-.~\w]+)+$/i
else
valid = args[0] =~ /^[-\/\w.]+$/
end
diff --git a/lib/puppet/rails/database/schema.rb b/lib/puppet/rails/database/schema.rb
index 43e320a..931a1b6 100644
--- a/lib/puppet/rails/database/schema.rb
+++ b/lib/puppet/rails/database/schema.rb
@@ -1,10 +1,13 @@
+require 'stringio'
+
class Puppet::Rails::Schema
def self.init
oldout = nil
+ text = ''
Puppet::Util.benchmark(Puppet, :notice, "Initialized database") do
# We want to rewrite stdout, so we don't get migration messages.
oldout = $stdout
- $stdout = File.open("/dev/null", "w")
+ $stdout = StringIO.new(text, 'w')
ActiveRecord::Schema.define do
create_table :resources do |t|
t.column :title, :text, :null => false
@@ -122,10 +125,12 @@ def self.init
add_index :inventory_facts, [:node_id, :name], :unique => true
end
end
+ rescue Exception => e
+ $stderr.puts e
+ $stderr.puts "The output from running the code was:", text
+ raise e
ensure
- $stdout.close
$stdout = oldout if oldout
- oldout = nil
end
end
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 76bfbe6..c2e0d7f 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -9,6 +9,7 @@
require 'monitor'
require 'tempfile'
require 'pathname'
+require 'ostruct'
module Puppet
module Util
@@ -402,40 +403,36 @@ def replace_file(file, default_mode, &block)
file_exists = file.exist?
- # If the file exists, use its current mode/owner/group. If it doesn't, use
- # the supplied mode, and default to current user/group.
- if file_exists
- if Puppet.features.microsoft_windows?
- mode = Puppet::Util::Windows::Security.get_mode(file.to_s)
- uid = Puppet::Util::Windows::Security.get_owner(file.to_s)
- gid = Puppet::Util::Windows::Security.get_owner(file.to_s)
- else
- stat = file.lstat
-
- mode = stat.mode
- uid = stat.uid
- gid = stat.gid
- end
-
- # We only care about the four lowest-order octets. Higher octets are
- # filesystem-specific.
- mode &= 07777
- else
- mode = default_mode
- uid = Process.euid
- gid = Process.egid
- end
-
# Set properties of the temporary file before we write the content, because
# Tempfile doesn't promise to be safe from reading by other people, just
# that it avoids races around creating the file.
if Puppet.features.microsoft_windows?
- Puppet::Util::Windows::Security.set_mode(mode, tempfile.path)
- Puppet::Util::Windows::Security.set_owner(uid, tempfile.path)
- Puppet::Util::Windows::Security.set_group(gid, tempfile.path)
+ # Our Windows emulation is pretty limited, and so we have to carefully
+ # and specifically handle the platform. --daniel 2012-03-30
+ if file_exists
+ uid = Puppet::Util::Windows::Security.get_owner(file.to_s)
+ Puppet::Util::Windows::Security.set_owner(uid, tempfile.path)
+
+ gid = Puppet::Util::Windows::Security.get_owner(file.to_s)
+ Puppet::Util::Windows::Security.set_group(gid, tempfile.path)
+
+ mode = Puppet::Util::Windows::Security.get_mode(file.to_s)
+ Puppet::Util::Windows::Security.set_mode(mode, tempfile.path)
+ else
+ Puppet::Util::Windows::Security.set_mode(default_mode, tempfile.path)
+ # ...and we can't set the owner or group, not that we really care,
+ # since they should default to the current process user and group.
+ end
else
- tempfile.chmod(mode)
- tempfile.chown(uid, gid)
+ # Grab the current file mode, and fall back to the defaults.
+ stat = file.lstat rescue OpenStruct.new(:mode => default_mode,
+ :uid => Process.euid,
+ :gid => Process.egid)
+
+ # We only care about the bottom four slots, which make the real mode,
+ # and not the rest of the platform stat call fluff and stuff.
+ tempfile.chmod(stat.mode & 07777)
+ tempfile.chown(stat.uid, stat.gid)
end
# OK, now allow the caller to write the content of the file.
@@ -457,7 +454,25 @@ def replace_file(file, default_mode, &block)
tempfile.close
- File.rename(tempfile.path, file)
+ if Puppet.features.microsoft_windows?
+ # This will appropriately clone the file, but only if the file we are
+ # replacing exists. Which is kind of annoying; thanks Microsoft.
+ begin
+ # Yes, the arguments are reversed compared to the rest of the world.
+ Puppet::Util::Windows::File.replace_file(file, tempfile.path)
+ rescue Puppet::Util::Windows::Error => e
+ # This might race, but there are enough possible cases that there
+ # isn't a good, solid "better" way to do this, and the next call
+ # should fail in the same way anyhow.
+ raise if File.exist? file
+ # The magic constant is "replace existing files", and "ensure content
+ # is flushed to disk", as per the MSDN reference guide:
+ # http://msdn.microsoft.com/en-us/library/aa365512(v=vs.85).aspx
+ Puppet::Util::Windows::File.move_file_ex(tempfile.path, file, 0x1|0x8)
+ end
+ else
+ File.rename(tempfile.path, file)
+ end
# Ideally, we would now fsync the directory as well, but Ruby doesn't
# have support for that, and it doesn't matter /that/ much...
diff --git a/lib/puppet/util/execution.rb b/lib/puppet/util/execution.rb
index 356fb4b..e843b55 100644
--- a/lib/puppet/util/execution.rb
+++ b/lib/puppet/util/execution.rb
@@ -71,7 +71,6 @@ def self.execfail(command, exception)
# :custom_environment (default {}) -- a hash of key/value pairs to set as environment variables for the duration
# of the command
def self.execute(command, arguments = {})
-
# specifying these here rather than in the method signature to allow callers to pass in a partial
# set of overrides without affecting the default values for options that they don't pass in
default_arguments = {
diff --git a/lib/puppet/util/monkey_patches.rb b/lib/puppet/util/monkey_patches.rb
index 952aa27..be8ee47 100644
--- a/lib/puppet/util/monkey_patches.rb
+++ b/lib/puppet/util/monkey_patches.rb
@@ -2,8 +2,12 @@
module Puppet::Util::MonkeyPatches
end
-unless defined? JRUBY_VERSION
+begin
Process.maxgroups = 1024
+rescue Exception
+ # Actually, I just want to ignore it, since various platforms - JRuby,
+ # Windows, and so forth - don't support it, but only because it isn't a
+ # meaningful or implementable concept there.
end
module RDoc
@@ -130,10 +134,15 @@ def self.binread(name, length = nil, offset = 0)
def self.binwrite(name, string, offset = nil)
# Determine if we should truncate or not. Since the truncate method on a
# file handle isn't implemented on all platforms, safer to do this in what
- # looks like the libc interface - which is usually pretty robust.
+ # looks like the libc / POSIX flag - which is usually pretty robust.
# --daniel 2012-03-11
mode = Fcntl::O_CREAT | Fcntl::O_WRONLY | (offset.nil? ? Fcntl::O_TRUNC : 0)
- IO.open(IO::sysopen(name, mode)) do |f|
+
+ # We have to duplicate the mode because Ruby on Windows is a bit precious,
+ # and doesn't actually carry over the mode. It won't work to just use
+ # open, either, because that doesn't like our system modes and the default
+ # open bits don't do what we need, which is awesome. --daniel 2012-03-30
+ IO.open(IO::sysopen(name, mode), mode) do |f|
# ...seek to our desired offset, then write the bytes. Don't try to
# seek past the start of the file, eh, because who knows what platform
# would legitimately blow up if we did that.
diff --git a/lib/puppet/util/windows.rb b/lib/puppet/util/windows.rb
index 086e08c..5096867 100644
--- a/lib/puppet/util/windows.rb
+++ b/lib/puppet/util/windows.rb
@@ -3,4 +3,5 @@ module Puppet::Util::Windows
require 'puppet/util/windows/security'
require 'puppet/util/windows/user'
require 'puppet/util/windows/process'
+ require 'puppet/util/windows/file'
end
diff --git a/lib/puppet/util/windows/file.rb b/lib/puppet/util/windows/file.rb
new file mode 100644
index 0000000..e4db8fb
--- /dev/null
+++ b/lib/puppet/util/windows/file.rb
@@ -0,0 +1,27 @@
+require 'puppet/util/windows'
+
+module Puppet::Util::Windows::File
+ require 'windows/api'
+ require 'windows/wide_string'
+
+ ReplaceFileWithoutBackupW = Windows::API.new('ReplaceFileW', 'PPVLVV', 'B')
+ def replace_file(target, source)
+ result = ReplaceFileWithoutBackupW.call(WideString.new(target.to_s),
+ WideString.new(source.to_s),
+ 0, 0x1, 0, 0)
+ return true unless result == 0
+ raise Puppet::Util::Windows::Error.new("ReplaceFile(#{target}, #{source})")
+ end
+ module_function :replace_file
+
+ MoveFileEx = Windows::API.new('MoveFileExW', 'PPL', 'B')
+ def move_file_ex(source, target, flags = 0)
+ result = MoveFileEx.call(WideString.new(source.to_s),
+ WideString.new(target.to_s),
+ flags)
+ return true unless result == 0
+ raise Puppet::Util::Windows::Error.
+ new("MoveFileEx(#{source}, #{target}, #{flags.to_s(8)})")
+ end
+ module_function :move_file_ex
+end
diff --git a/spec/lib/puppet_spec/database.rb b/spec/lib/puppet_spec/database.rb
index 2f7c209..86cb177 100644
--- a/spec/lib/puppet_spec/database.rb
+++ b/spec/lib/puppet_spec/database.rb
@@ -23,8 +23,8 @@ def can_use_scratch_database?
# automatically for you. Nothing to do there.
def setup_scratch_database
dir = PuppetSpec::Files.tmpdir('puppet-sqlite')
- Puppet[:dbadapter] = 'sqlite3'
- Puppet[:dblocation] = (dir + 'storeconfigs.sqlite').to_s
- Puppet[:railslog] = '/dev/null'
+ Puppet[:dbadapter] = 'sqlite3'
+ Puppet[:dblocation] = (dir + 'storeconfigs.sqlite').to_s
+ Puppet[:railslog] = (dir + 'storeconfigs.log').to_s
Puppet::Rails.init
end
diff --git a/spec/lib/puppet_spec/files.rb b/spec/lib/puppet_spec/files.rb
index 5608454..51d9615 100755
--- a/spec/lib/puppet_spec/files.rb
+++ b/spec/lib/puppet_spec/files.rb
@@ -22,7 +22,7 @@ def self.cleanup
fail "Not deleting tmpfile #{path} outside regular tmpdir" unless in_tmp(path)
begin
- FileUtils.rm_r path, :secure => true
+ FileUtils.rm_rf path, :secure => true
rescue Errno::ENOENT
# nothing to do
end
diff --git a/spec/unit/parser/functions/file_spec.rb b/spec/unit/parser/functions/file_spec.rb
index 901b750..d2ecc2f 100755
--- a/spec/unit/parser/functions/file_spec.rb
+++ b/spec/unit/parser/functions/file_spec.rb
@@ -40,7 +40,7 @@ def with_file_content(content)
it "should not fail when some files are absent" do
expect {
with_file_content('one') do |one|
- scope.function_file(["/this-should-not-exist", one]).should == 'one'
+ scope.function_file([make_absolute("/should-not-exist"), one]).should == 'one'
end
}.should_not raise_error
end
diff --git a/spec/unit/parser/functions/generate_spec.rb b/spec/unit/parser/functions/generate_spec.rb
index 42cf988..1fc9e85 100755
--- a/spec/unit/parser/functions/generate_spec.rb
+++ b/spec/unit/parser/functions/generate_spec.rb
@@ -48,6 +48,12 @@
Puppet.features.stubs(:microsoft_windows?).returns(true)
end
+ it "should accept the tilde in the path" do
+ command = "C:/DOCUME~1/ADMINI~1/foo.bat"
+ Dir.expects(:chdir).with(File.dirname(command)).returns("yay")
+ scope.function_generate([command]).should == 'yay'
+ end
+
it "should accept lower-case drive letters" do
command = 'd:/command/foo'
Dir.expects(:chdir).with(File.dirname(command)).returns("yay")
diff --git a/spec/unit/provider/service/base_spec.rb b/spec/unit/provider/service/base_spec.rb
index 9522fd7..03a33e2 100755
--- a/spec/unit/provider/service/base_spec.rb
+++ b/spec/unit/provider/service/base_spec.rb
@@ -19,17 +19,17 @@
RbConfig::CONFIG["RUBY_INSTALL_NAME"] +
RbConfig::CONFIG["EXEEXT"])
- Start = "#{Ruby} -rfileutils -e 'FileUtils.touch(ARGV[0])'"
- Status = "#{Ruby} -e 'exit File.file?(ARGV[0])'"
- Stop = "#{Ruby} -e 'File.exist?(ARGV[0]) and File.unlink(ARGV[0])'"
+ Start = [Ruby, '-rfileutils', '-e', 'FileUtils.touch(ARGV[0])']
+ Status = [Ruby, '-e' 'exit File.file?(ARGV[0])']
+ Stop = [Ruby, '-e', 'File.exist?(ARGV[0]) and File.unlink(ARGV[0])']
let :flag do tmpfile('base-service-test') end
subject do
type.new(:name => "test", :provider => :base,
- :start => "#{Start} #{flag}",
- :status => "#{Status} #{flag}",
- :stop => "#{Stop} #{flag}"
+ :start => Start + [flag],
+ :status => Status + [flag],
+ :stop => Stop + [flag]
).provider
end
diff --git a/spec/unit/puppet_spec.rb b/spec/unit/puppet_spec.rb
index 33fb2be..afe7a9d 100755
--- a/spec/unit/puppet_spec.rb
+++ b/spec/unit/puppet_spec.rb
@@ -24,7 +24,7 @@
end
it "should be able to change the path" do
- newpath = ENV["PATH"] + ":/something/else"
+ newpath = ENV["PATH"] + File::PATH_SEPARATOR + "/something/else"
Puppet[:path] = newpath
ENV["PATH"].should == newpath
end
diff --git a/spec/unit/util/log/destinations_spec.rb b/spec/unit/util/log/destinations_spec.rb
index 07052b3..69b627c 100755
--- a/spec/unit/util/log/destinations_spec.rb
+++ b/spec/unit/util/log/destinations_spec.rb
@@ -24,13 +24,15 @@
describe Puppet::Util::Log.desttypes[:file] do
+ include PuppetSpec::Files
+
before do
File.stubs(:open) # prevent actually creating the file
@class = Puppet::Util::Log.desttypes[:file]
end
it "should default to autoflush false" do
- @class.new('/tmp/log').autoflush.should == false
+ @class.new(tmpfile('log')).autoflush.should == false
end
describe "when matching" do
diff --git a/spec/unit/util/storage_spec.rb b/spec/unit/util/storage_spec.rb
index cbdaa43..c8a477d 100755
--- a/spec/unit/util/storage_spec.rb
+++ b/spec/unit/util/storage_spec.rb
@@ -174,13 +174,12 @@
describe "when storing to the state file" do
before(:each) do
- @state_file = Tempfile.new('storage_test')
+ @state_file = tmpfile('storage_test')
@saved_statefile = Puppet[:statefile]
- Puppet[:statefile] = @state_file.path
+ Puppet[:statefile] = @state_file
end
it "should create the state file if it does not exist" do
- @state_file.close!()
FileTest.exists?(Puppet[:statefile]).should be_false
Puppet::Util::Storage.cache(:yayness)
@@ -189,7 +188,6 @@
end
it "should raise an exception if the state file is not a regular file" do
- @state_file.close!()
Dir.mkdir(Puppet[:statefile])
Puppet::Util::Storage.cache(:yayness)
@@ -208,10 +206,5 @@
proc { Puppet::Util::Storage.load }.should_not raise_error
Puppet::Util::Storage.state.should == {:yayness=>{}}
end
-
- after(:each) do
- @state_file.close!()
- Puppet[:statefile] = @saved_statefile
- end
end
end
diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb
index 704e285..ed2da80 100755
--- a/spec/unit/util_spec.rb
+++ b/spec/unit/util_spec.rb
@@ -365,7 +365,12 @@ def get_mode(file)
File.read(target.path).should == "I am the passenger...\n"
end
- [0555, 0600, 0660, 0700, 0770].each do |mode|
+ # When running with the same user and group sid, which is the default,
+ # Windows collapses the owner and group modes into a single ACE, resulting
+ # in set(0600) => get(0660) and so forth. --daniel 2012-03-30
+ modes = [0555, 0660, 0770]
+ modes += [0600, 0700] unless Puppet.features.microsoft_windows?
+ modes.each do |mode|
it "should copy 0#{mode.to_s(8)} permissions from the target file by default" do
set_mode(mode, target.path)
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
