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

Use cp -c when copying files #17373

Merged
merged 19 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions Library/Homebrew/cask/artifact/moved.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

require "cask/artifact/relocated"
require "cask/quarantine"
require "utils/copy"

module Cask
module Artifact
Expand Down Expand Up @@ -108,8 +109,8 @@ def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall
if target.writable?
source.children.each { |child| FileUtils.move(child, target/child.basename) }
else
command.run!("/bin/cp", args: ["-pR", *source.children, target],
sudo: true)
::Utils::Copy.recursive_with_attributes(source.children, target,
force_command: true, sudo: true, command:)
end
Quarantine.copy_xattrs(source, target, command:)
source.rmtree
Expand All @@ -118,7 +119,7 @@ def move(adopt: false, force: false, verbose: false, predecessor: nil, reinstall
else
# default sudo user isn't necessarily able to write to Homebrew's locations
# e.g. with runas_default set in the sudoers (5) file.
command.run!("/bin/cp", args: ["-pR", source, target], sudo: true)
::Utils::Copy.recursive_with_attributes(source, target, force_command: true, sudo: true, command:)
source.rmtree
end

Expand Down Expand Up @@ -161,8 +162,9 @@ def move_back(skip: false, force: false, adopt: false, command: nil, **options)
ohai "Backing #{self.class.english_name} '#{target.basename}' up to '#{source}'"
source.dirname.mkpath

# We need to preserve extended attributes between copies.
command.run!("/bin/cp", args: ["-pR", target, source], sudo: !source.parent.writable?)
::Utils::Copy.recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:,
# This is required to preserve extended attributes between copies.
force_command: true)

delete(target, force:, command:, **options)
end
Expand Down
4 changes: 4 additions & 0 deletions Library/Homebrew/extend/os/copy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# typed: strict
# frozen_string_literal: true

require "extend/os/mac/utils/copy" if OS.mac?
28 changes: 28 additions & 0 deletions Library/Homebrew/extend/os/mac/utils/copy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# typed: strict
# frozen_string_literal: true

module Utils
module Copy
class << self
module MacOSOverride
private

# Use the lightweight `clonefile(2)` syscall if applicable.
SONOMA_FLAGS = T.let(["-c"].freeze, T::Array[String])

sig { returns(T.nilable(T::Array[String])) }
def extra_flags
# The `cp` command on older macOS versions also had the `-c` option, but before Sonoma,
# the command would fail if the `clonefile` syscall isn't applicable (the underlying
# filesystem doesn't support the feature or the source and the target are on different
# filesystems).
return if MacOS.version < :sonoma

SONOMA_FLAGS
end
end

prepend MacOSOverride
end
end
end
3 changes: 2 additions & 1 deletion Library/Homebrew/extend/pathname.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require "metafiles"
require "extend/file/atomic"
require "system_command"
require "utils/copy"

module DiskUsageExtension
sig { returns(Integer) }
Expand Down Expand Up @@ -226,7 +227,7 @@ def cp_path_sub(pattern, replacement)
else
dst.dirname.mkpath
dst = yield(self, dst) if block_given?
FileUtils.cp(self, dst)
Utils::Copy.with_attributes(self, dst)
end
end

Expand Down
20 changes: 20 additions & 0 deletions Library/Homebrew/sorbet/rbi/upstream.rbi
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,23 @@

# This file contains temporary definitions for fixes that have
# been submitted upstream to https://github.com/sorbet/sorbet.

# https://github.com/sorbet/sorbet/pull/7959
module FileUtils
sig {
params(
src: T.any(File, String, Pathname, T::Array[T.any(File, String, Pathname)]),
dest: T.any(String, Pathname),
preserve: T.nilable(T::Boolean),
noop: T.nilable(T::Boolean),
verbose: T.nilable(T::Boolean),
dereference_root: T::Boolean,
remove_destination: T.nilable(T::Boolean),
).returns(T.nilable(T::Array[String]))
}
def self.cp_r(src, dest, preserve: nil, noop: nil, verbose: nil, dereference_root: true, remove_destination: nil)
# XXX: This comment is a placeholder to suppress `Style/EmptyMethod` lint.
# Simply compacting the method definition in a single line would in turn trigger
# `Layout/LineLength`, driving `brew style --fix` to an infinite loop.
end
end
5 changes: 3 additions & 2 deletions Library/Homebrew/test/cask/artifact/app_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,9 @@
allow(command).to receive(:run!).with(any_args).and_call_original

expect(command).to receive(:run!)
.with("/bin/cp", args: ["-pR", source_contents_path, target_path],
sudo: true)
.with(a_string_ending_with("cp"),
hash_including(args: include(source_contents_path, target_path),
sudo: true))
.and_call_original
expect(FileUtils).not_to receive(:move).with(source_contents_path, an_instance_of(Pathname))

Expand Down
1 change: 1 addition & 0 deletions Library/Homebrew/test/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@

RSpec::Matchers.define_negated_matcher :not_to_output, :output
RSpec::Matchers.alias_matcher :have_failed, :be_failed
RSpec::Matchers.define_negated_matcher :exclude, :include

# Match consecutive elements in an array.
RSpec::Matchers.define :array_including_cons do |*cons|
Expand Down
146 changes: 146 additions & 0 deletions Library/Homebrew/test/utils/copy_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
# frozen_string_literal: true

require "system_command"
require "utils/copy"

RSpec.describe Utils::Copy do
let(:path) { Pathname(Dir.mktmpdir) }
let(:source) { path/"source" }
let(:target) { path/"target" }

RSpec.shared_examples "copies files" do |method_name|
context "when the source is a regular file" do
before do
source.write "foo"
FileUtils.touch source, mtime: 42
end

it "copies the file and preserves its attributes" do
expect(target.exist?).to be(false)

described_class.public_send(method_name, source, target)

expect(target.file?).to be(true)
expect(target.read).to eq(source.read)
expect(target.mtime).to eq(source.mtime)
end
end

context "when the source is a list of files and the target is a directory" do
let(:source) { [path/"file1", path/"file2"] }
let(:target_children) { [target/"file1", target/"file2"] }

before do
source.each do |source|
source.write("foo")
FileUtils.touch source, mtime: 42
end
target.mkpath
end

it "copies the files and preserves their attributes" do
expect(target_children.map(&:exist?)).to all be(false)

described_class.public_send(method_name, source, target)

expect(target_children.map(&:file?)).to all be(true)
target_children.zip(source) do |target, source|
expect(target.read).to eq(source.read)
expect(target.mtime).to eq(source.mtime)
end
end
end
end

RSpec.shared_context "with macOS version" do |version|
before do
allow(MacOS).to receive(:version).and_return(MacOSVersion.new(version))
end
end

RSpec.shared_examples ".*with_attributes" do |method_name, fileutils_method_name|
context "when running on macOS Sonoma or later", :needs_macos do
include_context "with macOS version", "14"

include_examples "copies files", method_name

it "executes `cp` command with `-c` flag" do
expect(SystemCommand).to receive(:run!).with(
a_string_ending_with("cp"),
hash_including(args: include("-c").and(end_with(source, target))),
)

described_class.public_send(method_name, source, target)
end
end

context "when running on Linux or macOS Ventura or earlier" do
include_context "with macOS version", "13" if OS.mac?

include_examples "copies files", method_name

it "uses `FileUtils.#{fileutils_method_name}`" do
expect(SystemCommand).not_to receive(:run!)
expect(FileUtils).to receive(fileutils_method_name).with(source, target, hash_including(preserve: true))

described_class.public_send(method_name, source, target)
end

context "when `force_command` is set" do
it "executes `cp` command without `-c` flag" do
expect(SystemCommand).to receive(:run!).with(
a_string_ending_with("cp"),
hash_including(args: exclude("-c").and(end_with(source, target))),
)

described_class.public_send(method_name, source, target, force_command: true)
end
end
end
end

describe ".with_attributes" do
include_examples ".*with_attributes", :with_attributes, :cp
end

describe ".recursive_with_attributes" do
RSpec.shared_examples "copies directory" do
context "when the source is a directory" do
before do
FileUtils.mkpath source, mode: 0742
(source/"child").tap do |child|
child.write "foo"
FileUtils.touch child, mtime: 42
end
end

it "copies the directory recursively and preserves its attributes" do
expect(target.exist?).to be(false)

described_class.recursive_with_attributes(source, target)

expect(target.directory?).to be(true)
expect(target.stat.mode).to be(source.stat.mode)

[source/"child", target/"child"].tap do |source, target|
expect(target.file?).to be(true)
expect(target.read).to eq(source.read)
expect(target.mtime).to eq(source.mtime)
end
end
end
end

include_examples ".*with_attributes", :recursive_with_attributes, :cp_r

context "when running on macOS Sonoma or later", :needs_macos do
include_context "with macOS version", "14"
include_examples "copies directory"
end

context "when running on Linux or macOS Ventura or earlier" do
include_context "with macOS version", "13" if OS.mac?
include_examples "copies directory"
end
end
end
4 changes: 3 additions & 1 deletion Library/Homebrew/unpack_strategy/bzip2.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: true
# frozen_string_literal: true

require "utils/copy"

module UnpackStrategy
# Strategy for unpacking bzip2 archives.
class Bzip2
Expand All @@ -19,7 +21,7 @@ def self.can_extract?(path)

sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose:)
FileUtils.cp path, unpack_dir/basename, preserve: true
Utils::Copy.with_attributes path, unpack_dir/basename
quiet_flags = verbose ? [] : ["-q"]
system_command! "bunzip2",
args: [*quiet_flags, unpack_dir/basename],
Expand Down
10 changes: 6 additions & 4 deletions Library/Homebrew/unpack_strategy/directory.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: true
# frozen_string_literal: true

require "utils/copy"

module UnpackStrategy
# Strategy for unpacking directories.
class Directory
Expand All @@ -20,10 +22,10 @@ def self.can_extract?(path)
sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose:)
path.children.each do |child|
system_command! "cp",
args: ["-pR", (child.directory? && !child.symlink?) ? "#{child}/." : child,
unpack_dir/child.basename],
verbose:
Utils::Copy.recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child,
unpack_dir/child.basename,
force_command: true,
verbose:
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion Library/Homebrew/unpack_strategy/gzip.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: true
# frozen_string_literal: true

require "utils/copy"

module UnpackStrategy
# Strategy for unpacking gzip archives.
class Gzip
Expand All @@ -19,7 +21,7 @@ def self.can_extract?(path)

sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose:)
FileUtils.cp path, unpack_dir/basename, preserve: true
Utils::Copy.with_attributes path, unpack_dir/basename
quiet_flags = verbose ? [] : ["-q"]
system_command! "gunzip",
args: [*quiet_flags, "-N", "--", unpack_dir/basename],
Expand Down
4 changes: 3 additions & 1 deletion Library/Homebrew/unpack_strategy/lzip.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: true
# frozen_string_literal: true

require "utils/copy"

module UnpackStrategy
# Strategy for unpacking lzip archives.
class Lzip
Expand All @@ -23,7 +25,7 @@

sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose:)
FileUtils.cp path, unpack_dir/basename, preserve: true
Utils::Copy.with_attributes path, unpack_dir/basename

Check warning on line 28 in Library/Homebrew/unpack_strategy/lzip.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/unpack_strategy/lzip.rb#L28

Added line #L28 was not covered by tests
quiet_flags = verbose ? [] : ["-q"]
system_command! "lzip",
args: ["-d", *quiet_flags, unpack_dir/basename],
Expand Down
4 changes: 3 additions & 1 deletion Library/Homebrew/unpack_strategy/lzma.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: true
# frozen_string_literal: true

require "utils/copy"

module UnpackStrategy
# Strategy for unpacking LZMA archives.
class Lzma
Expand All @@ -23,7 +25,7 @@

sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose:)
FileUtils.cp path, unpack_dir/basename, preserve: true
Utils::Copy.with_attributes path, unpack_dir/basename

Check warning on line 28 in Library/Homebrew/unpack_strategy/lzma.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/unpack_strategy/lzma.rb#L28

Added line #L28 was not covered by tests
quiet_flags = verbose ? [] : ["-q"]
system_command! "unlzma",
args: [*quiet_flags, "--", unpack_dir/basename],
Expand Down
4 changes: 3 additions & 1 deletion Library/Homebrew/unpack_strategy/uncompressed.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: true
# frozen_string_literal: true

require "utils/copy"

module UnpackStrategy
# Strategy for unpacking uncompressed files.
class Uncompressed
Expand All @@ -22,7 +24,7 @@ def extract_nestedly(to: nil, basename: nil, verbose: false, prioritize_extensio

sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose: false)
FileUtils.cp path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), preserve: true, verbose:
Utils::Copy.with_attributes path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose:
end
end
end
Loading
Loading