From b905959a9988d398591787269a3ba29035955a49 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Tue, 21 May 2024 20:18:58 +0900 Subject: [PATCH 01/19] Add `Utils::Cp` for interacting with `cp` command This module determines the `cp` command to use based on availability of the `coreutils` formula and optimizes the command invocation to prefer a lightweight copy-on-write clone, which is significantly faster than a full file copy and helps to reduce the risk of exhausting the storage during the operation. --- Library/Homebrew/utils/cp.rb | 96 ++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 Library/Homebrew/utils/cp.rb diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb new file mode 100644 index 0000000000000..76b41541c2ac1 --- /dev/null +++ b/Library/Homebrew/utils/cp.rb @@ -0,0 +1,96 @@ +# typed: true +# frozen_string_literal: true + +require "system_command" + +module Utils + # Helper functions for interacting with the `cp` command. + module Cp + class << self + sig { + params( + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).returns(SystemCommand::Result) + } + def copy(source, target, sudo: false, verbose: false, command: SystemCommand) + command.run! executable, args: ["-p", *extra_flags, *source, target], sudo:, verbose: + end + + sig { + params( + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).returns(SystemCommand::Result) + } + def copy_recursive(source, target, sudo: false, verbose: false, command: SystemCommand) + command.run! executable, args: ["-pR", *extra_flags, *source, target], sudo:, verbose: + end + + private + + GCP = (HOMEBREW_PREFIX/"opt/coreutils/libexec/gnubin/cp").freeze + UCP = (HOMEBREW_PREFIX/"opt/uutils-coreutils/libexec/uubin/cp").freeze + + sig { returns(T.any(String, Pathname)) } + def executable + case type + when :coreutils + GCP + when :uutils + UCP + else + "cp" + end + end + + GNU_FLAGS = [ + # Unlike BSD cp, `gcp -p` doesn't guarantee to preserve extended attributes, including + # quarantine information on macOS. + "--preserve=all", + "--no-preserve=links", + # Perform a lightweight copy-on-write clone if applicable. + "--reflink=auto", + ].freeze + # On macOS, the `cp` utility has the `-c` option, which is equivalent to + # `gcp --reflink=always`, but we are not using that because it 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). + GENERIC_FLAGS = [].freeze + + sig { returns(T::Array[String]) } + def extra_flags + if type + GNU_FLAGS + else + GENERIC_FLAGS + end + end + + sig { returns(T.nilable(Symbol)) } + def type + return @type if defined?(@type) + + { + coreutils: "coreutils", + uutils: "uutils-coreutils", + }.each do |type, formula| + begin + formula = Formula[formula] + rescue FormulaUnavailableError + next + end + return @type = type if formula.optlinked? + end + + @type = nil + end + end + end +end From deaac7ce472b4a2e504328ec6bdb88bcb92f7124 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Sun, 26 May 2024 08:46:38 +0900 Subject: [PATCH 02/19] Use `Utils::Cp` to copy files This replaces `FileUtils.cp` and `system_command! "cp"` with the new `Utils::Cp` utility where it is expected that the performance improvement outweighs the cost of the system command invocation. --- Library/Homebrew/cask/artifact/moved.rb | 10 +++++----- Library/Homebrew/dev-cmd/bottle.rb | 3 ++- Library/Homebrew/extend/pathname.rb | 3 ++- Library/Homebrew/test/cask/artifact/app_spec.rb | 5 +++-- Library/Homebrew/unpack_strategy/bzip2.rb | 4 +++- Library/Homebrew/unpack_strategy/directory.rb | 9 +++++---- Library/Homebrew/unpack_strategy/gzip.rb | 4 +++- Library/Homebrew/unpack_strategy/lzip.rb | 4 +++- Library/Homebrew/unpack_strategy/lzma.rb | 4 +++- Library/Homebrew/unpack_strategy/uncompressed.rb | 4 +++- Library/Homebrew/unpack_strategy/xz.rb | 4 +++- Library/Homebrew/unpack_strategy/zstd.rb | 4 +++- 12 files changed, 38 insertions(+), 20 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index b6fe5c893f975..252e54ad52ca6 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -3,6 +3,7 @@ require "cask/artifact/relocated" require "cask/quarantine" +require "utils/cp" module Cask module Artifact @@ -108,8 +109,7 @@ 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::Cp.copy_recursive(source.children, target, sudo: true, command:) end Quarantine.copy_xattrs(source, target, command:) source.rmtree @@ -118,7 +118,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::Cp.copy_recursive(source, target, sudo: true, command:) source.rmtree end @@ -161,8 +161,8 @@ 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::Cp` preserves extended attributes between copies. + ::Utils::Cp.copy_recursive(target, source, sudo: !source.parent.writable?, command:) delete(target, force:, command:, **options) end diff --git a/Library/Homebrew/dev-cmd/bottle.rb b/Library/Homebrew/dev-cmd/bottle.rb index 2495494c112d8..761d2f71b4ec7 100644 --- a/Library/Homebrew/dev-cmd/bottle.rb +++ b/Library/Homebrew/dev-cmd/bottle.rb @@ -11,6 +11,7 @@ require "formula_versions" require "utils/inreplace" require "erb" +require "utils/cp" require "utils/gzip" require "api" require "extend/hash/deep_merge" @@ -767,7 +768,7 @@ def merge all_bottle_hash = { formula_name => all_bottle_formula_hash } puts "Copying #{filename} to #{all_filename}" if args.verbose? - FileUtils.cp filename.to_s, all_filename.to_s + Utils::Cp.copy filename.to_s, all_filename.to_s puts "Writing #{all_filename.json}" if args.verbose? all_local_json_path = Pathname(all_filename.json) diff --git a/Library/Homebrew/extend/pathname.rb b/Library/Homebrew/extend/pathname.rb index ec649fe0e42cf..ee44019703541 100644 --- a/Library/Homebrew/extend/pathname.rb +++ b/Library/Homebrew/extend/pathname.rb @@ -6,6 +6,7 @@ require "metafiles" require "extend/file/atomic" require "system_command" +require "utils/cp" module DiskUsageExtension sig { returns(Integer) } @@ -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::Cp.copy(self, dst) end end diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb index 95cc8d9a8b446..e33d6732221d4 100644 --- a/Library/Homebrew/test/cask/artifact/app_spec.rb +++ b/Library/Homebrew/test/cask/artifact/app_spec.rb @@ -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)) diff --git a/Library/Homebrew/unpack_strategy/bzip2.rb b/Library/Homebrew/unpack_strategy/bzip2.rb index af1248c466f53..aac5347f7ab87 100644 --- a/Library/Homebrew/unpack_strategy/bzip2.rb +++ b/Library/Homebrew/unpack_strategy/bzip2.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking bzip2 archives. class Bzip2 @@ -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::Cp.copy path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "bunzip2", args: [*quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/directory.rb b/Library/Homebrew/unpack_strategy/directory.rb index 90b5ade18e772..5f049d38221f9 100644 --- a/Library/Homebrew/unpack_strategy/directory.rb +++ b/Library/Homebrew/unpack_strategy/directory.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking directories. class Directory @@ -20,10 +22,9 @@ 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::Cp.copy_recursive (child.directory? && !child.symlink?) ? "#{child}/." : child, + unpack_dir/child.basename, + verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/gzip.rb b/Library/Homebrew/unpack_strategy/gzip.rb index 2ece0befdc456..41798e4b97150 100644 --- a/Library/Homebrew/unpack_strategy/gzip.rb +++ b/Library/Homebrew/unpack_strategy/gzip.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking gzip archives. class Gzip @@ -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::Cp.copy path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "gunzip", args: [*quiet_flags, "-N", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzip.rb b/Library/Homebrew/unpack_strategy/lzip.rb index 668aa4fdcfc18..132ec7243906c 100644 --- a/Library/Homebrew/unpack_strategy/lzip.rb +++ b/Library/Homebrew/unpack_strategy/lzip.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking lzip archives. class Lzip @@ -23,7 +25,7 @@ def dependencies 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::Cp.copy path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "lzip", args: ["-d", *quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzma.rb b/Library/Homebrew/unpack_strategy/lzma.rb index d529e2de4c4f3..2e9d711376453 100644 --- a/Library/Homebrew/unpack_strategy/lzma.rb +++ b/Library/Homebrew/unpack_strategy/lzma.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking LZMA archives. class Lzma @@ -23,7 +25,7 @@ def dependencies 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::Cp.copy path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unlzma", args: [*quiet_flags, "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/uncompressed.rb b/Library/Homebrew/unpack_strategy/uncompressed.rb index fdde716dcbf4c..2fccd6ddc2e42 100644 --- a/Library/Homebrew/unpack_strategy/uncompressed.rb +++ b/Library/Homebrew/unpack_strategy/uncompressed.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking uncompressed files. class Uncompressed @@ -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::Cp.copy path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/xz.rb b/Library/Homebrew/unpack_strategy/xz.rb index 7ac0ceb4e6c38..726438231e602 100644 --- a/Library/Homebrew/unpack_strategy/xz.rb +++ b/Library/Homebrew/unpack_strategy/xz.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking xz archives. class Xz @@ -23,7 +25,7 @@ def dependencies 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::Cp.copy path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unxz", args: [*quiet_flags, "-T0", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/zstd.rb b/Library/Homebrew/unpack_strategy/zstd.rb index f8cdacb0abec3..b49615e5f4e98 100644 --- a/Library/Homebrew/unpack_strategy/zstd.rb +++ b/Library/Homebrew/unpack_strategy/zstd.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "utils/cp" + module UnpackStrategy # Strategy for unpacking zstd archives. class Zstd @@ -23,7 +25,7 @@ def dependencies 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::Cp.copy path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unzstd", args: [*quiet_flags, "-T0", "--rm", "--", unpack_dir/basename], From b8af1c546ebd9151aa507916c35fbf42031618cf Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Mon, 27 May 2024 17:48:32 +0900 Subject: [PATCH 03/19] Utils::Cp: Fix tests --- Library/Homebrew/test/formula_installer_bottle_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Library/Homebrew/test/formula_installer_bottle_spec.rb b/Library/Homebrew/test/formula_installer_bottle_spec.rb index 0f4e4eee6afb7..10a6adb49dfaa 100644 --- a/Library/Homebrew/test/formula_installer_bottle_spec.rb +++ b/Library/Homebrew/test/formula_installer_bottle_spec.rb @@ -21,6 +21,8 @@ def temporarily_install_bottle(formula) expect(formula).to be_bottled expect(formula).to pour_bottle + stub_formula_loader formula("coreutils") { url "coreutils-1.0" } + stub_formula_loader formula("uutils-coreutils") { url "uutils-coreutils-1.0" } stub_formula_loader formula("gcc") { url "gcc-1.0" } stub_formula_loader formula("glibc") { url "glibc-1.0" } stub_formula_loader formula From 942906b74a4b29ab0bc0951fe4974c5c93adc5a2 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Mon, 27 May 2024 18:00:12 +0900 Subject: [PATCH 04/19] Utils::Cp: Use `cp` from macOS --- Library/Homebrew/utils/cp.rb | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index 76b41541c2ac1..61b805a60877b 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -41,6 +41,8 @@ def copy_recursive(source, target, sudo: false, verbose: false, command: SystemC sig { returns(T.any(String, Pathname)) } def executable case type + when :macos + Pathname("/bin/cp") when :coreutils GCP when :uutils @@ -50,23 +52,26 @@ def executable end end + MACOS_FLAGS = [ + # Perform a lightweight copy-on-write clone if applicable. + "-c", + ].freeze GNU_FLAGS = [ # Unlike BSD cp, `gcp -p` doesn't guarantee to preserve extended attributes, including # quarantine information on macOS. "--preserve=all", "--no-preserve=links", - # Perform a lightweight copy-on-write clone if applicable. + # Equivalent to `-c` on macOS. "--reflink=auto", ].freeze - # On macOS, the `cp` utility has the `-c` option, which is equivalent to - # `gcp --reflink=always`, but we are not using that because it 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). GENERIC_FLAGS = [].freeze sig { returns(T::Array[String]) } def extra_flags - if type + case type + when :macos + MACOS_FLAGS + when :coreutils, :uutils GNU_FLAGS else GENERIC_FLAGS @@ -77,6 +82,12 @@ def extra_flags def type return @type if defined?(@type) + # The `cp` command on some 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 @type = :macos if MacOS.version >= :sonoma + { coreutils: "coreutils", uutils: "uutils-coreutils", From b4dcb94ad6707ba348568f87512b108e5f4e4278 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Mon, 27 May 2024 18:01:57 +0900 Subject: [PATCH 05/19] Utils::Cp: Drop special case for coreutils `cp` As per review feedback: https://github.com/Homebrew/brew/pull/17373#issuecomment-2132673915 --- .../test/formula_installer_bottle_spec.rb | 2 - Library/Homebrew/utils/cp.rb | 72 ++++--------------- 2 files changed, 12 insertions(+), 62 deletions(-) diff --git a/Library/Homebrew/test/formula_installer_bottle_spec.rb b/Library/Homebrew/test/formula_installer_bottle_spec.rb index 10a6adb49dfaa..0f4e4eee6afb7 100644 --- a/Library/Homebrew/test/formula_installer_bottle_spec.rb +++ b/Library/Homebrew/test/formula_installer_bottle_spec.rb @@ -21,8 +21,6 @@ def temporarily_install_bottle(formula) expect(formula).to be_bottled expect(formula).to pour_bottle - stub_formula_loader formula("coreutils") { url "coreutils-1.0" } - stub_formula_loader formula("uutils-coreutils") { url "uutils-coreutils-1.0" } stub_formula_loader formula("gcc") { url "gcc-1.0" } stub_formula_loader formula("glibc") { url "glibc-1.0" } stub_formula_loader formula diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index 61b805a60877b..d64804b71cb64 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -17,7 +17,11 @@ class << self ).returns(SystemCommand::Result) } def copy(source, target, sudo: false, verbose: false, command: SystemCommand) - command.run! executable, args: ["-p", *extra_flags, *source, target], sudo:, verbose: + # On macOS, `cp -p` guarantees to preserve extended attributes (including quarantine + # information) in addition to file mode. Other implementations like coreutils does not + # necessarily guarantee the same behavior, but that is fine because we don't really need to + # preserve extended attributes except when copying Cask artifacts. + command.run! "cp", args: ["-p", *extra_flags, *source, target], sudo:, verbose: end sig { @@ -30,78 +34,26 @@ def copy(source, target, sudo: false, verbose: false, command: SystemCommand) ).returns(SystemCommand::Result) } def copy_recursive(source, target, sudo: false, verbose: false, command: SystemCommand) - command.run! executable, args: ["-pR", *extra_flags, *source, target], sudo:, verbose: + command.run! "cp", args: ["-pR", *extra_flags, *source, target], sudo:, verbose: end private - GCP = (HOMEBREW_PREFIX/"opt/coreutils/libexec/gnubin/cp").freeze - UCP = (HOMEBREW_PREFIX/"opt/uutils-coreutils/libexec/uubin/cp").freeze - - sig { returns(T.any(String, Pathname)) } - def executable - case type - when :macos - Pathname("/bin/cp") - when :coreutils - GCP - when :uutils - UCP - else - "cp" - end - end - - MACOS_FLAGS = [ - # Perform a lightweight copy-on-write clone if applicable. - "-c", - ].freeze - GNU_FLAGS = [ - # Unlike BSD cp, `gcp -p` doesn't guarantee to preserve extended attributes, including - # quarantine information on macOS. - "--preserve=all", - "--no-preserve=links", - # Equivalent to `-c` on macOS. - "--reflink=auto", - ].freeze + # Use the lightweight `clonefile(2)` syscall if applicable. + MACOS_FLAGS = ["-c"].freeze GENERIC_FLAGS = [].freeze sig { returns(T::Array[String]) } def extra_flags - case type - when :macos + # 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). + if MacOS.version >= :sonoma MACOS_FLAGS - when :coreutils, :uutils - GNU_FLAGS else GENERIC_FLAGS end end - - sig { returns(T.nilable(Symbol)) } - def type - return @type if defined?(@type) - - # The `cp` command on some 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 @type = :macos if MacOS.version >= :sonoma - - { - coreutils: "coreutils", - uutils: "uutils-coreutils", - }.each do |type, formula| - begin - formula = Formula[formula] - rescue FormulaUnavailableError - next - end - return @type = type if formula.optlinked? - end - - @type = nil - end end end end From a5500aa7f210076544e600c5fca16b4340f9ca7a Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Thu, 6 Jun 2024 21:45:03 +0900 Subject: [PATCH 06/19] Utils::Cp: Fix Linux tests --- Library/Homebrew/utils/cp.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index d64804b71cb64..12d6482dfa811 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -48,7 +48,7 @@ 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). - if MacOS.version >= :sonoma + if OS.mac? && MacOS.version >= :sonoma MACOS_FLAGS else GENERIC_FLAGS From 58852106c1ebb8670b70b198dfbaa6708633fdfb Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Fri, 7 Jun 2024 18:02:52 +0900 Subject: [PATCH 07/19] Utils::Cp: Rename `copy*` methods to `copy*_with_attributes` As per review feedback: https://github.com/Homebrew/brew/pull/17373#pullrequestreview-2103870774 --- Library/Homebrew/cask/artifact/moved.rb | 6 +++--- Library/Homebrew/dev-cmd/bottle.rb | 2 +- Library/Homebrew/extend/pathname.rb | 2 +- Library/Homebrew/unpack_strategy/bzip2.rb | 2 +- Library/Homebrew/unpack_strategy/directory.rb | 6 +++--- Library/Homebrew/unpack_strategy/gzip.rb | 2 +- Library/Homebrew/unpack_strategy/lzip.rb | 2 +- Library/Homebrew/unpack_strategy/lzma.rb | 2 +- Library/Homebrew/unpack_strategy/uncompressed.rb | 2 +- Library/Homebrew/unpack_strategy/xz.rb | 2 +- Library/Homebrew/unpack_strategy/zstd.rb | 2 +- Library/Homebrew/utils/cp.rb | 4 ++-- 12 files changed, 17 insertions(+), 17 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 252e54ad52ca6..26128fba75a7b 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -109,7 +109,7 @@ 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 - ::Utils::Cp.copy_recursive(source.children, target, sudo: true, command:) + ::Utils::Cp.copy_recursive_with_attributes(source.children, target, sudo: true, command:) end Quarantine.copy_xattrs(source, target, command:) source.rmtree @@ -118,7 +118,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. - ::Utils::Cp.copy_recursive(source, target, sudo: true, command:) + ::Utils::Cp.copy_recursive_with_attributes(source, target, sudo: true, command:) source.rmtree end @@ -162,7 +162,7 @@ def move_back(skip: false, force: false, adopt: false, command: nil, **options) source.dirname.mkpath # `Utils::Cp` preserves extended attributes between copies. - ::Utils::Cp.copy_recursive(target, source, sudo: !source.parent.writable?, command:) + ::Utils::Cp.copy_recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:) delete(target, force:, command:, **options) end diff --git a/Library/Homebrew/dev-cmd/bottle.rb b/Library/Homebrew/dev-cmd/bottle.rb index 761d2f71b4ec7..d0bf47a6b00e7 100644 --- a/Library/Homebrew/dev-cmd/bottle.rb +++ b/Library/Homebrew/dev-cmd/bottle.rb @@ -768,7 +768,7 @@ def merge all_bottle_hash = { formula_name => all_bottle_formula_hash } puts "Copying #{filename} to #{all_filename}" if args.verbose? - Utils::Cp.copy filename.to_s, all_filename.to_s + Utils::Cp.copy_with_attributes filename.to_s, all_filename.to_s puts "Writing #{all_filename.json}" if args.verbose? all_local_json_path = Pathname(all_filename.json) diff --git a/Library/Homebrew/extend/pathname.rb b/Library/Homebrew/extend/pathname.rb index ee44019703541..6fd2e1ae6a382 100644 --- a/Library/Homebrew/extend/pathname.rb +++ b/Library/Homebrew/extend/pathname.rb @@ -227,7 +227,7 @@ def cp_path_sub(pattern, replacement) else dst.dirname.mkpath dst = yield(self, dst) if block_given? - Utils::Cp.copy(self, dst) + Utils::Cp.copy_with_attributes(self, dst) end end diff --git a/Library/Homebrew/unpack_strategy/bzip2.rb b/Library/Homebrew/unpack_strategy/bzip2.rb index aac5347f7ab87..c37214a837f23 100644 --- a/Library/Homebrew/unpack_strategy/bzip2.rb +++ b/Library/Homebrew/unpack_strategy/bzip2.rb @@ -21,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:) - Utils::Cp.copy path, unpack_dir/basename + Utils::Cp.copy_with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "bunzip2", args: [*quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/directory.rb b/Library/Homebrew/unpack_strategy/directory.rb index 5f049d38221f9..c95b31d379bf5 100644 --- a/Library/Homebrew/unpack_strategy/directory.rb +++ b/Library/Homebrew/unpack_strategy/directory.rb @@ -22,9 +22,9 @@ 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| - Utils::Cp.copy_recursive (child.directory? && !child.symlink?) ? "#{child}/." : child, - unpack_dir/child.basename, - verbose: + Utils::Cp.copy_recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child, + unpack_dir/child.basename, + verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/gzip.rb b/Library/Homebrew/unpack_strategy/gzip.rb index 41798e4b97150..8652123560640 100644 --- a/Library/Homebrew/unpack_strategy/gzip.rb +++ b/Library/Homebrew/unpack_strategy/gzip.rb @@ -21,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:) - Utils::Cp.copy path, unpack_dir/basename + Utils::Cp.copy_with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "gunzip", args: [*quiet_flags, "-N", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzip.rb b/Library/Homebrew/unpack_strategy/lzip.rb index 132ec7243906c..5b920687504e2 100644 --- a/Library/Homebrew/unpack_strategy/lzip.rb +++ b/Library/Homebrew/unpack_strategy/lzip.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy path, unpack_dir/basename + Utils::Cp.copy_with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "lzip", args: ["-d", *quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzma.rb b/Library/Homebrew/unpack_strategy/lzma.rb index 2e9d711376453..6ebdec1e692f6 100644 --- a/Library/Homebrew/unpack_strategy/lzma.rb +++ b/Library/Homebrew/unpack_strategy/lzma.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy path, unpack_dir/basename + Utils::Cp.copy_with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unlzma", args: [*quiet_flags, "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/uncompressed.rb b/Library/Homebrew/unpack_strategy/uncompressed.rb index 2fccd6ddc2e42..69fa900ceccab 100644 --- a/Library/Homebrew/unpack_strategy/uncompressed.rb +++ b/Library/Homebrew/unpack_strategy/uncompressed.rb @@ -24,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) - Utils::Cp.copy path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: + Utils::Cp.copy_with_attributes path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/xz.rb b/Library/Homebrew/unpack_strategy/xz.rb index 726438231e602..06c9e18828e34 100644 --- a/Library/Homebrew/unpack_strategy/xz.rb +++ b/Library/Homebrew/unpack_strategy/xz.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy path, unpack_dir/basename + Utils::Cp.copy_with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unxz", args: [*quiet_flags, "-T0", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/zstd.rb b/Library/Homebrew/unpack_strategy/zstd.rb index b49615e5f4e98..853e448a7e5d6 100644 --- a/Library/Homebrew/unpack_strategy/zstd.rb +++ b/Library/Homebrew/unpack_strategy/zstd.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy path, unpack_dir/basename + Utils::Cp.copy_with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unzstd", args: [*quiet_flags, "-T0", "--rm", "--", unpack_dir/basename], diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index 12d6482dfa811..f49036c002b71 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -16,7 +16,7 @@ class << self command: T.class_of(SystemCommand), ).returns(SystemCommand::Result) } - def copy(source, target, sudo: false, verbose: false, command: SystemCommand) + def copy_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) # On macOS, `cp -p` guarantees to preserve extended attributes (including quarantine # information) in addition to file mode. Other implementations like coreutils does not # necessarily guarantee the same behavior, but that is fine because we don't really need to @@ -33,7 +33,7 @@ def copy(source, target, sudo: false, verbose: false, command: SystemCommand) command: T.class_of(SystemCommand), ).returns(SystemCommand::Result) } - def copy_recursive(source, target, sudo: false, verbose: false, command: SystemCommand) + def copy_recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) command.run! "cp", args: ["-pR", *extra_flags, *source, target], sudo:, verbose: end From 7cfcc596b90f2cbee9ba96d40be7eb1a731db188 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Fri, 7 Jun 2024 18:16:15 +0900 Subject: [PATCH 08/19] Utils::Cp: Move macOS-specific code to `extend/os/mac` --- Library/Homebrew/extend/os/cp.rb | 4 +++ Library/Homebrew/extend/os/mac/utils/cp.rb | 30 ++++++++++++++++++++++ Library/Homebrew/utils/cp.rb | 13 ++-------- 3 files changed, 36 insertions(+), 11 deletions(-) create mode 100644 Library/Homebrew/extend/os/cp.rb create mode 100644 Library/Homebrew/extend/os/mac/utils/cp.rb diff --git a/Library/Homebrew/extend/os/cp.rb b/Library/Homebrew/extend/os/cp.rb new file mode 100644 index 0000000000000..e35d5b123a6df --- /dev/null +++ b/Library/Homebrew/extend/os/cp.rb @@ -0,0 +1,4 @@ +# typed: strict +# frozen_string_literal: true + +require "extend/os/mac/utils/cp" if OS.mac? diff --git a/Library/Homebrew/extend/os/mac/utils/cp.rb b/Library/Homebrew/extend/os/mac/utils/cp.rb new file mode 100644 index 0000000000000..ca82d67e689e4 --- /dev/null +++ b/Library/Homebrew/extend/os/mac/utils/cp.rb @@ -0,0 +1,30 @@ +# typed: true +# frozen_string_literal: true + +module Utils + module Cp + class << self + module MacOSOverride + private + + # Use the lightweight `clonefile(2)` syscall if applicable. + SONOMA_FLAGS = ["-c"].freeze + + sig { returns(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). + if MacOS.version >= :sonoma + SONOMA_FLAGS + else + super + end + end + end + + prepend MacOSOverride + end + end +end diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index f49036c002b71..a08aadcc13e0e 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -1,6 +1,7 @@ # typed: true # frozen_string_literal: true +require "extend/os/cp" require "system_command" module Utils @@ -39,20 +40,10 @@ def copy_recursive_with_attributes(source, target, sudo: false, verbose: false, private - # Use the lightweight `clonefile(2)` syscall if applicable. - MACOS_FLAGS = ["-c"].freeze GENERIC_FLAGS = [].freeze - sig { returns(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). - if OS.mac? && MacOS.version >= :sonoma - MACOS_FLAGS - else - GENERIC_FLAGS - end + GENERIC_FLAGS end end end From d9239faad32cb03273e36d3fb2ec1873992c27bf Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Fri, 7 Jun 2024 18:18:53 +0900 Subject: [PATCH 09/19] Utils::Cp: Revert the use of `Utils::Cp` in dev-cmd As per review feedback: https://github.com/Homebrew/brew/pull/17373#pullrequestreview-2103870774 --- Library/Homebrew/dev-cmd/bottle.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bottle.rb b/Library/Homebrew/dev-cmd/bottle.rb index d0bf47a6b00e7..2495494c112d8 100644 --- a/Library/Homebrew/dev-cmd/bottle.rb +++ b/Library/Homebrew/dev-cmd/bottle.rb @@ -11,7 +11,6 @@ require "formula_versions" require "utils/inreplace" require "erb" -require "utils/cp" require "utils/gzip" require "api" require "extend/hash/deep_merge" @@ -768,7 +767,7 @@ def merge all_bottle_hash = { formula_name => all_bottle_formula_hash } puts "Copying #{filename} to #{all_filename}" if args.verbose? - Utils::Cp.copy_with_attributes filename.to_s, all_filename.to_s + FileUtils.cp filename.to_s, all_filename.to_s puts "Writing #{all_filename.json}" if args.verbose? all_local_json_path = Pathname(all_filename.json) From b2ddeecdd922c097427e0aea2d5c234817ef2efc Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Sat, 8 Jun 2024 06:21:13 +0900 Subject: [PATCH 10/19] Utils::Cp: Remove `copy` prefix from method name As per review feedback: https://github.com/Homebrew/brew/pull/17373#pullrequestreview-2104523770 Co-authored-by: Mike McQuaid --- Library/Homebrew/cask/artifact/moved.rb | 6 +++--- Library/Homebrew/extend/pathname.rb | 2 +- Library/Homebrew/unpack_strategy/bzip2.rb | 2 +- Library/Homebrew/unpack_strategy/directory.rb | 6 +++--- Library/Homebrew/unpack_strategy/gzip.rb | 2 +- Library/Homebrew/unpack_strategy/lzip.rb | 2 +- Library/Homebrew/unpack_strategy/lzma.rb | 2 +- Library/Homebrew/unpack_strategy/uncompressed.rb | 2 +- Library/Homebrew/unpack_strategy/xz.rb | 2 +- Library/Homebrew/unpack_strategy/zstd.rb | 2 +- Library/Homebrew/utils/cp.rb | 4 ++-- 11 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 26128fba75a7b..77043497b034d 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -109,7 +109,7 @@ 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 - ::Utils::Cp.copy_recursive_with_attributes(source.children, target, sudo: true, command:) + ::Utils::Cp.recursive_with_attributes(source.children, target, sudo: true, command:) end Quarantine.copy_xattrs(source, target, command:) source.rmtree @@ -118,7 +118,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. - ::Utils::Cp.copy_recursive_with_attributes(source, target, sudo: true, command:) + ::Utils::Cp.recursive_with_attributes(source, target, sudo: true, command:) source.rmtree end @@ -162,7 +162,7 @@ def move_back(skip: false, force: false, adopt: false, command: nil, **options) source.dirname.mkpath # `Utils::Cp` preserves extended attributes between copies. - ::Utils::Cp.copy_recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:) + ::Utils::Cp.recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:) delete(target, force:, command:, **options) end diff --git a/Library/Homebrew/extend/pathname.rb b/Library/Homebrew/extend/pathname.rb index 6fd2e1ae6a382..529665123eddb 100644 --- a/Library/Homebrew/extend/pathname.rb +++ b/Library/Homebrew/extend/pathname.rb @@ -227,7 +227,7 @@ def cp_path_sub(pattern, replacement) else dst.dirname.mkpath dst = yield(self, dst) if block_given? - Utils::Cp.copy_with_attributes(self, dst) + Utils::Cp.with_attributes(self, dst) end end diff --git a/Library/Homebrew/unpack_strategy/bzip2.rb b/Library/Homebrew/unpack_strategy/bzip2.rb index c37214a837f23..2ca92686b9abe 100644 --- a/Library/Homebrew/unpack_strategy/bzip2.rb +++ b/Library/Homebrew/unpack_strategy/bzip2.rb @@ -21,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:) - Utils::Cp.copy_with_attributes path, unpack_dir/basename + Utils::Cp.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "bunzip2", args: [*quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/directory.rb b/Library/Homebrew/unpack_strategy/directory.rb index c95b31d379bf5..717e7782b75cf 100644 --- a/Library/Homebrew/unpack_strategy/directory.rb +++ b/Library/Homebrew/unpack_strategy/directory.rb @@ -22,9 +22,9 @@ 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| - Utils::Cp.copy_recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child, - unpack_dir/child.basename, - verbose: + Utils::Cp.recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child, + unpack_dir/child.basename, + verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/gzip.rb b/Library/Homebrew/unpack_strategy/gzip.rb index 8652123560640..8e2b869eeb088 100644 --- a/Library/Homebrew/unpack_strategy/gzip.rb +++ b/Library/Homebrew/unpack_strategy/gzip.rb @@ -21,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:) - Utils::Cp.copy_with_attributes path, unpack_dir/basename + Utils::Cp.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "gunzip", args: [*quiet_flags, "-N", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzip.rb b/Library/Homebrew/unpack_strategy/lzip.rb index 5b920687504e2..8da86400881a3 100644 --- a/Library/Homebrew/unpack_strategy/lzip.rb +++ b/Library/Homebrew/unpack_strategy/lzip.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy_with_attributes path, unpack_dir/basename + Utils::Cp.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "lzip", args: ["-d", *quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzma.rb b/Library/Homebrew/unpack_strategy/lzma.rb index 6ebdec1e692f6..b9a8f4e2d444f 100644 --- a/Library/Homebrew/unpack_strategy/lzma.rb +++ b/Library/Homebrew/unpack_strategy/lzma.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy_with_attributes path, unpack_dir/basename + Utils::Cp.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unlzma", args: [*quiet_flags, "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/uncompressed.rb b/Library/Homebrew/unpack_strategy/uncompressed.rb index 69fa900ceccab..ae36bd17f54d2 100644 --- a/Library/Homebrew/unpack_strategy/uncompressed.rb +++ b/Library/Homebrew/unpack_strategy/uncompressed.rb @@ -24,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) - Utils::Cp.copy_with_attributes path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: + Utils::Cp.with_attributes path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/xz.rb b/Library/Homebrew/unpack_strategy/xz.rb index 06c9e18828e34..94177c804fdd5 100644 --- a/Library/Homebrew/unpack_strategy/xz.rb +++ b/Library/Homebrew/unpack_strategy/xz.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy_with_attributes path, unpack_dir/basename + Utils::Cp.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unxz", args: [*quiet_flags, "-T0", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/zstd.rb b/Library/Homebrew/unpack_strategy/zstd.rb index 853e448a7e5d6..f1a89a4519581 100644 --- a/Library/Homebrew/unpack_strategy/zstd.rb +++ b/Library/Homebrew/unpack_strategy/zstd.rb @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.copy_with_attributes path, unpack_dir/basename + Utils::Cp.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unzstd", args: [*quiet_flags, "-T0", "--rm", "--", unpack_dir/basename], diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index a08aadcc13e0e..e091103f51e34 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -17,7 +17,7 @@ class << self command: T.class_of(SystemCommand), ).returns(SystemCommand::Result) } - def copy_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) + def with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) # On macOS, `cp -p` guarantees to preserve extended attributes (including quarantine # information) in addition to file mode. Other implementations like coreutils does not # necessarily guarantee the same behavior, but that is fine because we don't really need to @@ -34,7 +34,7 @@ def copy_with_attributes(source, target, sudo: false, verbose: false, command: S command: T.class_of(SystemCommand), ).returns(SystemCommand::Result) } - def copy_recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) + def recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) command.run! "cp", args: ["-pR", *extra_flags, *source, target], sudo:, verbose: end From a4271fdad1ce78b398290db76320c0a8b608323d Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Sat, 8 Jun 2024 07:58:57 +0900 Subject: [PATCH 11/19] Apply suggestions from code review https://github.com/Homebrew/brew/pull/17373#pullrequestreview-2104523770 Co-authored-by: Mike McQuaid --- Library/Homebrew/utils/cp.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index e091103f51e34..a6f5b1cfd6fa5 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -40,10 +40,8 @@ def recursive_with_attributes(source, target, sudo: false, verbose: false, comma private - GENERIC_FLAGS = [].freeze - def extra_flags - GENERIC_FLAGS + [].freeze end end end From 9156891c99c69403f88dd53e0eb8930654049136 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Sat, 8 Jun 2024 19:39:56 +0900 Subject: [PATCH 12/19] Utils::Cp: Use `FileUtils.cp` on Linux `FileUtils.cp` is implemented with the lightweight `copy_file_range(2)` syscall on Linux, so it's more performant than the plain `cp` command on that platform. cf. https://github.com/Homebrew/brew/pull/17373#pullrequestreview-2105629022 --- Library/Homebrew/extend/os/mac/utils/cp.rb | 30 +++++++++++++++++- Library/Homebrew/utils/cp.rb | 37 ++++------------------ 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/Library/Homebrew/extend/os/mac/utils/cp.rb b/Library/Homebrew/extend/os/mac/utils/cp.rb index ca82d67e689e4..2b71ed9497f0d 100644 --- a/Library/Homebrew/extend/os/mac/utils/cp.rb +++ b/Library/Homebrew/extend/os/mac/utils/cp.rb @@ -5,6 +5,34 @@ module Utils module Cp class << self module MacOSOverride + sig { + params( + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).returns(SystemCommand::Result) + } + def with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) + # `cp -p` on macOS guarantees to preserve extended attributes (including quarantine + # information) in addition to file mode, which is requered when copying cask artifacts. + command.run! "cp", args: ["-p", *extra_flags, *source, target], sudo:, verbose: + end + + sig { + params( + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).returns(SystemCommand::Result) + } + def recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) + command.run! "cp", args: ["-pR", *extra_flags, *source, target], sudo:, verbose: + end + private # Use the lightweight `clonefile(2)` syscall if applicable. @@ -19,7 +47,7 @@ def extra_flags if MacOS.version >= :sonoma SONOMA_FLAGS else - super + [].freeze end end end diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index a6f5b1cfd6fa5..903207f05fd3c 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -2,46 +2,21 @@ # frozen_string_literal: true require "extend/os/cp" +require "fileutils" require "system_command" module Utils - # Helper functions for interacting with the `cp` command. + # Helper functions for copying files. module Cp class << self - sig { - params( - source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), - target: T.any(String, Pathname), - sudo: T::Boolean, - verbose: T::Boolean, - command: T.class_of(SystemCommand), - ).returns(SystemCommand::Result) - } def with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) - # On macOS, `cp -p` guarantees to preserve extended attributes (including quarantine - # information) in addition to file mode. Other implementations like coreutils does not - # necessarily guarantee the same behavior, but that is fine because we don't really need to - # preserve extended attributes except when copying Cask artifacts. - command.run! "cp", args: ["-p", *extra_flags, *source, target], sudo:, verbose: + odisabled "`Utils::Cp.with_attributes` with `sudo: true` on Linux" if sudo + FileUtils.cp source, target, preserve: true, verbose: end - sig { - params( - source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), - target: T.any(String, Pathname), - sudo: T::Boolean, - verbose: T::Boolean, - command: T.class_of(SystemCommand), - ).returns(SystemCommand::Result) - } def recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) - command.run! "cp", args: ["-pR", *extra_flags, *source, target], sudo:, verbose: - end - - private - - def extra_flags - [].freeze + odisabled "`Utils::Cp.recursive_with_attributes` with `sudo: true` on Linux" if sudo + FileUtils.cp_r source, target, preserve: true, verbose: end end end From 67f280eb5396616c1d40d0e06f639ee8d49ef710 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Sun, 9 Jun 2024 05:54:36 +0900 Subject: [PATCH 13/19] Utils::Cp: Add `force_system` keyword argument This fixes the test for `UnpackStrategy::Directory`, which needs the `cp` command. --- Library/Homebrew/cask/artifact/moved.rb | 9 +-- Library/Homebrew/extend/os/mac/utils/cp.rb | 57 +++++++++++-------- Library/Homebrew/unpack_strategy/directory.rb | 1 + Library/Homebrew/utils/cp.rb | 23 ++++++-- 4 files changed, 56 insertions(+), 34 deletions(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 77043497b034d..f735e1e22c4e3 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -109,7 +109,7 @@ 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 - ::Utils::Cp.recursive_with_attributes(source.children, target, sudo: true, command:) + ::Utils::Cp.recursive_with_attributes(source.children, target, force_command: true, sudo: true, command:) end Quarantine.copy_xattrs(source, target, command:) source.rmtree @@ -118,7 +118,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. - ::Utils::Cp.recursive_with_attributes(source, target, sudo: true, command:) + ::Utils::Cp.recursive_with_attributes(source, target, force_command: true, sudo: true, command:) source.rmtree end @@ -161,8 +161,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 - # `Utils::Cp` preserves extended attributes between copies. - ::Utils::Cp.recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:) + ::Utils::Cp.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 diff --git a/Library/Homebrew/extend/os/mac/utils/cp.rb b/Library/Homebrew/extend/os/mac/utils/cp.rb index 2b71ed9497f0d..4822025502002 100644 --- a/Library/Homebrew/extend/os/mac/utils/cp.rb +++ b/Library/Homebrew/extend/os/mac/utils/cp.rb @@ -7,30 +7,41 @@ class << self module MacOSOverride sig { params( - source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), - target: T.any(String, Pathname), - sudo: T::Boolean, - verbose: T::Boolean, - command: T.class_of(SystemCommand), - ).returns(SystemCommand::Result) + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + force_command: T::Boolean, + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).void } - def with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) - # `cp -p` on macOS guarantees to preserve extended attributes (including quarantine - # information) in addition to file mode, which is requered when copying cask artifacts. - command.run! "cp", args: ["-p", *extra_flags, *source, target], sudo:, verbose: + def with_attributes(source, target, force_command: false, sudo: false, verbose: false, command: SystemCommand) + if (flags = extra_flags) + command.run! "cp", args: ["-p", *flags, *source, target], sudo:, verbose: + nil + else + super + end end sig { params( - source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), - target: T.any(String, Pathname), - sudo: T::Boolean, - verbose: T::Boolean, - command: T.class_of(SystemCommand), - ).returns(SystemCommand::Result) + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + force_command: T::Boolean, + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).void } - def recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) - command.run! "cp", args: ["-pR", *extra_flags, *source, target], sudo:, verbose: + def recursive_with_attributes(source, target, force_command: false, sudo: false, verbose: false, + command: SystemCommand) + if (flags = extra_flags) + command.run! "cp", args: ["-pR", *flags, *source, target], sudo:, verbose: + nil + else + super + end end private @@ -38,17 +49,15 @@ def recursive_with_attributes(source, target, sudo: false, verbose: false, comma # Use the lightweight `clonefile(2)` syscall if applicable. SONOMA_FLAGS = ["-c"].freeze - sig { returns(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). - if MacOS.version >= :sonoma - SONOMA_FLAGS - else - [].freeze - end + return if MacOS.version < :sonoma + + SONOMA_FLAGS end end diff --git a/Library/Homebrew/unpack_strategy/directory.rb b/Library/Homebrew/unpack_strategy/directory.rb index 717e7782b75cf..23e20027cba39 100644 --- a/Library/Homebrew/unpack_strategy/directory.rb +++ b/Library/Homebrew/unpack_strategy/directory.rb @@ -24,6 +24,7 @@ def extract_to_dir(unpack_dir, basename:, verbose:) path.children.each do |child| Utils::Cp.recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child, unpack_dir/child.basename, + force_command: true, verbose: end end diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index 903207f05fd3c..6ecdf5df00b7f 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -9,14 +9,25 @@ module Utils # Helper functions for copying files. module Cp class << self - def with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) - odisabled "`Utils::Cp.with_attributes` with `sudo: true` on Linux" if sudo - FileUtils.cp source, target, preserve: true, verbose: + def with_attributes(source, target, force_command: false, sudo: false, verbose: false, command: SystemCommand) + if force_command || sudo + command.run! "cp", args: ["-p", *source, target], sudo:, verbose: + else + FileUtils.cp source, target, preserve: true, verbose: + end + + nil end - def recursive_with_attributes(source, target, sudo: false, verbose: false, command: SystemCommand) - odisabled "`Utils::Cp.recursive_with_attributes` with `sudo: true` on Linux" if sudo - FileUtils.cp_r source, target, preserve: true, verbose: + def recursive_with_attributes(source, target, force_command: false, sudo: false, verbose: false, + command: SystemCommand) + if force_command || sudo + command.run! "cp", args: ["-pR", *source, target], sudo:, verbose: + else + FileUtils.cp_r source, target, preserve: true, verbose: + end + + nil end end end From eab1e87726e8952aa46b83e89ac34ddf3bbe0c4a Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Sun, 9 Jun 2024 12:36:04 +0900 Subject: [PATCH 14/19] Utils::Cp: Deduplicate `SystemCommand` invocations --- Library/Homebrew/extend/os/mac/utils/cp.rb | 39 ---------------------- Library/Homebrew/sorbet/rbi/upstream.rbi | 20 +++++++++++ Library/Homebrew/utils/cp.rb | 32 +++++++++++++++--- 3 files changed, 48 insertions(+), 43 deletions(-) diff --git a/Library/Homebrew/extend/os/mac/utils/cp.rb b/Library/Homebrew/extend/os/mac/utils/cp.rb index 4822025502002..ec430b7dc56a1 100644 --- a/Library/Homebrew/extend/os/mac/utils/cp.rb +++ b/Library/Homebrew/extend/os/mac/utils/cp.rb @@ -5,45 +5,6 @@ module Utils module Cp class << self module MacOSOverride - sig { - params( - source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), - target: T.any(String, Pathname), - force_command: T::Boolean, - sudo: T::Boolean, - verbose: T::Boolean, - command: T.class_of(SystemCommand), - ).void - } - def with_attributes(source, target, force_command: false, sudo: false, verbose: false, command: SystemCommand) - if (flags = extra_flags) - command.run! "cp", args: ["-p", *flags, *source, target], sudo:, verbose: - nil - else - super - end - end - - sig { - params( - source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), - target: T.any(String, Pathname), - force_command: T::Boolean, - sudo: T::Boolean, - verbose: T::Boolean, - command: T.class_of(SystemCommand), - ).void - } - def recursive_with_attributes(source, target, force_command: false, sudo: false, verbose: false, - command: SystemCommand) - if (flags = extra_flags) - command.run! "cp", args: ["-pR", *flags, *source, target], sudo:, verbose: - nil - else - super - end - end - private # Use the lightweight `clonefile(2)` syscall if applicable. diff --git a/Library/Homebrew/sorbet/rbi/upstream.rbi b/Library/Homebrew/sorbet/rbi/upstream.rbi index a00487f95aced..25724349e31e8 100644 --- a/Library/Homebrew/sorbet/rbi/upstream.rbi +++ b/Library/Homebrew/sorbet/rbi/upstream.rbi @@ -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 diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/cp.rb index 6ecdf5df00b7f..e29d2cecceab2 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/cp.rb @@ -9,9 +9,19 @@ module Utils # Helper functions for copying files. module Cp class << self + sig { + params( + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + force_command: T::Boolean, + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).void + } def with_attributes(source, target, force_command: false, sudo: false, verbose: false, command: SystemCommand) - if force_command || sudo - command.run! "cp", args: ["-p", *source, target], sudo:, verbose: + if force_command || sudo || (flags = extra_flags) + command.run! "cp", args: ["-p", *flags, *source, target], sudo:, verbose: else FileUtils.cp source, target, preserve: true, verbose: end @@ -19,16 +29,30 @@ def with_attributes(source, target, force_command: false, sudo: false, verbose: nil end + sig { + params( + source: T.any(String, Pathname, T::Array[T.any(String, Pathname)]), + target: T.any(String, Pathname), + force_command: T::Boolean, + sudo: T::Boolean, + verbose: T::Boolean, + command: T.class_of(SystemCommand), + ).void + } def recursive_with_attributes(source, target, force_command: false, sudo: false, verbose: false, command: SystemCommand) - if force_command || sudo - command.run! "cp", args: ["-pR", *source, target], sudo:, verbose: + if force_command || sudo || (flags = extra_flags) + command.run! "cp", args: ["-pR", *flags, *source, target], sudo:, verbose: else FileUtils.cp_r source, target, preserve: true, verbose: end nil end + + private + + def extra_flags; end end end end From 4edbbfd794a694870c118c231c7cc289723062d0 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Mon, 10 Jun 2024 19:02:09 +0900 Subject: [PATCH 15/19] Utils::Cp: Add tests --- Library/Homebrew/test/spec_helper.rb | 1 + Library/Homebrew/test/utils/cp_spec.rb | 146 +++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 Library/Homebrew/test/utils/cp_spec.rb diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index 4287f45a1c340..ee82b05ad84f3 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -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| diff --git a/Library/Homebrew/test/utils/cp_spec.rb b/Library/Homebrew/test/utils/cp_spec.rb new file mode 100644 index 0000000000000..1d25a51442e95 --- /dev/null +++ b/Library/Homebrew/test/utils/cp_spec.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +require "system_command" +require "utils/cp" + +RSpec.describe Utils::Cp 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" + + 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" + include_examples "copies directory" + end + end +end From 028cfe1ea63fd85748219eca53bfda8c0441e661 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Tue, 11 Jun 2024 07:53:47 +0900 Subject: [PATCH 16/19] Utils::Cp: Rename to `Utils::Copy` As per review feedback: https://github.com/Homebrew/brew/pull/17373#discussion_r1633217748 --- Library/Homebrew/cask/artifact/moved.rb | 8 ++++---- Library/Homebrew/extend/os/{cp.rb => copy.rb} | 2 +- .../Homebrew/extend/os/mac/utils/{cp.rb => copy.rb} | 2 +- Library/Homebrew/extend/pathname.rb | 4 ++-- .../Homebrew/test/utils/{cp_spec.rb => copy_spec.rb} | 4 ++-- Library/Homebrew/unpack_strategy/bzip2.rb | 4 ++-- Library/Homebrew/unpack_strategy/directory.rb | 10 +++++----- Library/Homebrew/unpack_strategy/gzip.rb | 4 ++-- Library/Homebrew/unpack_strategy/lzip.rb | 4 ++-- Library/Homebrew/unpack_strategy/lzma.rb | 4 ++-- Library/Homebrew/unpack_strategy/uncompressed.rb | 4 ++-- Library/Homebrew/unpack_strategy/xz.rb | 4 ++-- Library/Homebrew/unpack_strategy/zstd.rb | 4 ++-- Library/Homebrew/utils/{cp.rb => copy.rb} | 4 ++-- 14 files changed, 31 insertions(+), 31 deletions(-) rename Library/Homebrew/extend/os/{cp.rb => copy.rb} (50%) rename Library/Homebrew/extend/os/mac/utils/{cp.rb => copy.rb} (98%) rename Library/Homebrew/test/utils/{cp_spec.rb => copy_spec.rb} (98%) rename Library/Homebrew/utils/{cp.rb => copy.rb} (97%) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index f735e1e22c4e3..084c3316fa316 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -3,7 +3,7 @@ require "cask/artifact/relocated" require "cask/quarantine" -require "utils/cp" +require "utils/copy" module Cask module Artifact @@ -109,7 +109,7 @@ 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 - ::Utils::Cp.recursive_with_attributes(source.children, target, force_command: true, sudo: true, command:) + ::Utils::Copy.recursive_with_attributes(source.children, target, force_command: true, sudo: true, command:) end Quarantine.copy_xattrs(source, target, command:) source.rmtree @@ -118,7 +118,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. - ::Utils::Cp.recursive_with_attributes(source, target, force_command: true, sudo: true, command:) + ::Utils::Copy.recursive_with_attributes(source, target, force_command: true, sudo: true, command:) source.rmtree end @@ -161,7 +161,7 @@ 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 - ::Utils::Cp.recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:, + ::Utils::Copy.recursive_with_attributes(target, source, sudo: !source.parent.writable?, command:, # This is required to preserve extended attributes between copies. force_command: true) diff --git a/Library/Homebrew/extend/os/cp.rb b/Library/Homebrew/extend/os/copy.rb similarity index 50% rename from Library/Homebrew/extend/os/cp.rb rename to Library/Homebrew/extend/os/copy.rb index e35d5b123a6df..59dcc7da706f0 100644 --- a/Library/Homebrew/extend/os/cp.rb +++ b/Library/Homebrew/extend/os/copy.rb @@ -1,4 +1,4 @@ # typed: strict # frozen_string_literal: true -require "extend/os/mac/utils/cp" if OS.mac? +require "extend/os/mac/utils/copy" if OS.mac? diff --git a/Library/Homebrew/extend/os/mac/utils/cp.rb b/Library/Homebrew/extend/os/mac/utils/copy.rb similarity index 98% rename from Library/Homebrew/extend/os/mac/utils/cp.rb rename to Library/Homebrew/extend/os/mac/utils/copy.rb index ec430b7dc56a1..40e8ad4fc55f5 100644 --- a/Library/Homebrew/extend/os/mac/utils/cp.rb +++ b/Library/Homebrew/extend/os/mac/utils/copy.rb @@ -2,7 +2,7 @@ # frozen_string_literal: true module Utils - module Cp + module Copy class << self module MacOSOverride private diff --git a/Library/Homebrew/extend/pathname.rb b/Library/Homebrew/extend/pathname.rb index 529665123eddb..d0defa91d4c68 100644 --- a/Library/Homebrew/extend/pathname.rb +++ b/Library/Homebrew/extend/pathname.rb @@ -6,7 +6,7 @@ require "metafiles" require "extend/file/atomic" require "system_command" -require "utils/cp" +require "utils/copy" module DiskUsageExtension sig { returns(Integer) } @@ -227,7 +227,7 @@ def cp_path_sub(pattern, replacement) else dst.dirname.mkpath dst = yield(self, dst) if block_given? - Utils::Cp.with_attributes(self, dst) + Utils::Copy.with_attributes(self, dst) end end diff --git a/Library/Homebrew/test/utils/cp_spec.rb b/Library/Homebrew/test/utils/copy_spec.rb similarity index 98% rename from Library/Homebrew/test/utils/cp_spec.rb rename to Library/Homebrew/test/utils/copy_spec.rb index 1d25a51442e95..f95758efc0735 100644 --- a/Library/Homebrew/test/utils/cp_spec.rb +++ b/Library/Homebrew/test/utils/copy_spec.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true require "system_command" -require "utils/cp" +require "utils/copy" -RSpec.describe Utils::Cp do +RSpec.describe Utils::Copy do let(:path) { Pathname(Dir.mktmpdir) } let(:source) { path/"source" } let(:target) { path/"target" } diff --git a/Library/Homebrew/unpack_strategy/bzip2.rb b/Library/Homebrew/unpack_strategy/bzip2.rb index 2ca92686b9abe..30ae554dff824 100644 --- a/Library/Homebrew/unpack_strategy/bzip2.rb +++ b/Library/Homebrew/unpack_strategy/bzip2.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking bzip2 archives. @@ -21,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:) - Utils::Cp.with_attributes path, unpack_dir/basename + Utils::Copy.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "bunzip2", args: [*quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/directory.rb b/Library/Homebrew/unpack_strategy/directory.rb index 23e20027cba39..b8c841932ac07 100644 --- a/Library/Homebrew/unpack_strategy/directory.rb +++ b/Library/Homebrew/unpack_strategy/directory.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking directories. @@ -22,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| - Utils::Cp.recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child, - unpack_dir/child.basename, - force_command: true, - verbose: + Utils::Copy.recursive_with_attributes (child.directory? && !child.symlink?) ? "#{child}/." : child, + unpack_dir/child.basename, + force_command: true, + verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/gzip.rb b/Library/Homebrew/unpack_strategy/gzip.rb index 8e2b869eeb088..c343a173a526e 100644 --- a/Library/Homebrew/unpack_strategy/gzip.rb +++ b/Library/Homebrew/unpack_strategy/gzip.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking gzip archives. @@ -21,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:) - Utils::Cp.with_attributes path, unpack_dir/basename + Utils::Copy.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "gunzip", args: [*quiet_flags, "-N", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzip.rb b/Library/Homebrew/unpack_strategy/lzip.rb index 8da86400881a3..f676e12509f84 100644 --- a/Library/Homebrew/unpack_strategy/lzip.rb +++ b/Library/Homebrew/unpack_strategy/lzip.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking lzip archives. @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.with_attributes path, unpack_dir/basename + Utils::Copy.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "lzip", args: ["-d", *quiet_flags, unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/lzma.rb b/Library/Homebrew/unpack_strategy/lzma.rb index b9a8f4e2d444f..85c0898aa5dcb 100644 --- a/Library/Homebrew/unpack_strategy/lzma.rb +++ b/Library/Homebrew/unpack_strategy/lzma.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking LZMA archives. @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.with_attributes path, unpack_dir/basename + Utils::Copy.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unlzma", args: [*quiet_flags, "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/uncompressed.rb b/Library/Homebrew/unpack_strategy/uncompressed.rb index ae36bd17f54d2..ab2ba36fbf15c 100644 --- a/Library/Homebrew/unpack_strategy/uncompressed.rb +++ b/Library/Homebrew/unpack_strategy/uncompressed.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking uncompressed files. @@ -24,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) - Utils::Cp.with_attributes path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: + Utils::Copy.with_attributes path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), verbose: end end end diff --git a/Library/Homebrew/unpack_strategy/xz.rb b/Library/Homebrew/unpack_strategy/xz.rb index 94177c804fdd5..77d4e85da765f 100644 --- a/Library/Homebrew/unpack_strategy/xz.rb +++ b/Library/Homebrew/unpack_strategy/xz.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking xz archives. @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.with_attributes path, unpack_dir/basename + Utils::Copy.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unxz", args: [*quiet_flags, "-T0", "--", unpack_dir/basename], diff --git a/Library/Homebrew/unpack_strategy/zstd.rb b/Library/Homebrew/unpack_strategy/zstd.rb index f1a89a4519581..4c2571df2d53c 100644 --- a/Library/Homebrew/unpack_strategy/zstd.rb +++ b/Library/Homebrew/unpack_strategy/zstd.rb @@ -1,7 +1,7 @@ # typed: true # frozen_string_literal: true -require "utils/cp" +require "utils/copy" module UnpackStrategy # Strategy for unpacking zstd archives. @@ -25,7 +25,7 @@ def dependencies sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } def extract_to_dir(unpack_dir, basename:, verbose:) - Utils::Cp.with_attributes path, unpack_dir/basename + Utils::Copy.with_attributes path, unpack_dir/basename quiet_flags = verbose ? [] : ["-q"] system_command! "unzstd", args: [*quiet_flags, "-T0", "--rm", "--", unpack_dir/basename], diff --git a/Library/Homebrew/utils/cp.rb b/Library/Homebrew/utils/copy.rb similarity index 97% rename from Library/Homebrew/utils/cp.rb rename to Library/Homebrew/utils/copy.rb index e29d2cecceab2..7833e69675d43 100644 --- a/Library/Homebrew/utils/cp.rb +++ b/Library/Homebrew/utils/copy.rb @@ -1,13 +1,13 @@ # typed: true # frozen_string_literal: true -require "extend/os/cp" +require "extend/os/copy" require "fileutils" require "system_command" module Utils # Helper functions for copying files. - module Cp + module Copy class << self sig { params( From 8e8d0c024857fce648c86c8edcaad9974d085e75 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Wed, 12 Jun 2024 12:28:42 +0900 Subject: [PATCH 17/19] brew style --fix --- Library/Homebrew/cask/artifact/moved.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/artifact/moved.rb b/Library/Homebrew/cask/artifact/moved.rb index 084c3316fa316..d22e4c76c08a0 100644 --- a/Library/Homebrew/cask/artifact/moved.rb +++ b/Library/Homebrew/cask/artifact/moved.rb @@ -109,7 +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 - ::Utils::Copy.recursive_with_attributes(source.children, target, force_command: true, sudo: true, command:) + ::Utils::Copy.recursive_with_attributes(source.children, target, + force_command: true, sudo: true, command:) end Quarantine.copy_xattrs(source, target, command:) source.rmtree From e4fefc94ebf7e3f3e289fbee9d2efabb7115dcd7 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Wed, 12 Jun 2024 19:30:04 +0900 Subject: [PATCH 18/19] Utils::Copy: Fix tests for generic OS --- Library/Homebrew/test/utils/copy_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/utils/copy_spec.rb b/Library/Homebrew/test/utils/copy_spec.rb index f95758efc0735..9d9ee33c1f577 100644 --- a/Library/Homebrew/test/utils/copy_spec.rb +++ b/Library/Homebrew/test/utils/copy_spec.rb @@ -75,7 +75,7 @@ end context "when running on Linux or macOS Ventura or earlier" do - include_context "with macOS version", "13" + include_context "with macOS version", "13" if OS.mac? include_examples "copies files", method_name @@ -139,7 +139,7 @@ end context "when running on Linux or macOS Ventura or earlier" do - include_context "with macOS version", "13" + include_context "with macOS version", "13" if OS.mac? include_examples "copies directory" end end From a30cd15a73cf9835be2b3aaeb5e1cfa4f8b6dd07 Mon Sep 17 00:00:00 2001 From: Daiki Mizukami Date: Thu, 13 Jun 2024 06:56:18 +0900 Subject: [PATCH 19/19] extend/os/mac/utils/copy: `typed: strict` --- Library/Homebrew/extend/os/mac/utils/copy.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/extend/os/mac/utils/copy.rb b/Library/Homebrew/extend/os/mac/utils/copy.rb index 40e8ad4fc55f5..f262524c8a4c1 100644 --- a/Library/Homebrew/extend/os/mac/utils/copy.rb +++ b/Library/Homebrew/extend/os/mac/utils/copy.rb @@ -1,4 +1,4 @@ -# typed: true +# typed: strict # frozen_string_literal: true module Utils @@ -8,7 +8,7 @@ module MacOSOverride private # Use the lightweight `clonefile(2)` syscall if applicable. - SONOMA_FLAGS = ["-c"].freeze + SONOMA_FLAGS = T.let(["-c"].freeze, T::Array[String]) sig { returns(T.nilable(T::Array[String])) } def extra_flags