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

Improve locking UX #17904

Merged
merged 1 commit into from
Jul 30, 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
2 changes: 2 additions & 0 deletions Library/Homebrew/brew.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
if internal_cmd || Commands.external_ruby_v2_cmd_path(cmd)
cmd = T.must(cmd)
cmd_class = Homebrew::AbstractCommand.command(cmd)
Homebrew.running_command = cmd
if cmd_class
command_instance = cmd_class.new

Expand All @@ -97,6 +98,7 @@
Homebrew.public_send Commands.method_name(cmd)
end
elsif (path = Commands.external_ruby_cmd_path(cmd))
Homebrew.running_command = cmd
require?(path)
exit Homebrew.failed? ? 1 : 0
elsif Commands.external_cmd_path(cmd)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/cleanup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ def cleanup_unreferenced_downloads
(downloads - referenced_downloads).each do |download|
if self.class.incomplete?(download)
begin
LockFile.new(download.basename).with_lock do
DownloadLock.new(download).with_lock do
download.unlink
end
rescue OperationInProgressError
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/cmd/vendor-install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ homebrew-vendor-install() {

CACHED_LOCATION="${HOMEBREW_CACHE}/${VENDOR_FILENAME}"

lock "vendor-install-${VENDOR_NAME}"
lock "vendor-install ${VENDOR_NAME}"
fetch
install

Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/download_strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ def initialize(url, name, version, **meta)
def fetch(timeout: nil)
end_time = Time.now + timeout if timeout

download_lock = LockFile.new(temporary_path.basename)
download_lock = DownloadLock.new(temporary_path)
download_lock.lock

urls = [url, *mirrors]
Expand Down
4 changes: 4 additions & 0 deletions Library/Homebrew/env_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ module EnvConfig
"or `$HOME/.homebrew/livecheck_watchlist.txt` otherwise.",
default: "#{ENV.fetch("HOMEBREW_USER_CONFIG_HOME")}/livecheck_watchlist.txt",
},
HOMEBREW_LOCK_CONTEXT: {
description: "If set, Homebrew will add this output as additional context for locking errors. " \
"This is useful when running `brew` in the background.",
},
HOMEBREW_LOGS: {
description: "Use this directory to store log files.",
default_text: "macOS: `$HOME/Library/Logs/Homebrew`, " \
Expand Down
10 changes: 7 additions & 3 deletions Library/Homebrew/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -359,10 +359,14 @@ def initialize(name)

# Raised when another Homebrew operation is already in progress.
class OperationInProgressError < RuntimeError
def initialize(name)
sig { params(locked_path: Pathname).void }
def initialize(locked_path)
full_command = Homebrew.running_command_with_args.presence || "brew"
lock_context = if (env_lock_context = Homebrew::EnvConfig.lock_context.presence)
"\n#{env_lock_context}"
end
message = <<~EOS
Operation already in progress for #{name}
Another active Homebrew process is already using #{name}.
A `#{full_command}` process has already locked #{locked_path}.#{lock_context}
Please wait for it to finish or terminate it to continue.
EOS

Expand Down
10 changes: 10 additions & 0 deletions Library/Homebrew/global.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,16 @@ def running_as_root_but_not_owned_by_root?
def auto_update_command?
ENV.fetch("HOMEBREW_AUTO_UPDATE_COMMAND", false).present?
end

sig { params(cmd: T.nilable(String)).void }
def running_command=(cmd)
@running_command_with_args = "#{cmd} #{ARGV.join(" ")}"
end

sig { returns String }
def running_command_with_args
"brew #{@running_command_with_args}".strip
end
end
end

Expand Down
30 changes: 21 additions & 9 deletions Library/Homebrew/lock_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@

require "fcntl"

# A lock file.
# A lock file to prevent multiple Homebrew processes from modifying the same path.
class LockFile
attr_reader :path

def initialize(name)
@name = name.to_s
@path = HOMEBREW_LOCKS/"#{@name}.lock"
sig { params(type: Symbol, locked_path: Pathname).void }
def initialize(type, locked_path)
@locked_path = locked_path
lock_name = locked_path.basename.to_s
@path = HOMEBREW_LOCKS/"#{lock_name}.#{type}.lock"
@lockfile = nil
end

Expand All @@ -18,7 +20,7 @@ def lock
create_lockfile
return if @lockfile.flock(File::LOCK_EX | File::LOCK_NB)

raise OperationInProgressError, @name
raise OperationInProgressError, @locked_path
end

def unlock
Expand Down Expand Up @@ -51,14 +53,24 @@ def create_lockfile

# A lock file for a formula.
class FormulaLock < LockFile
def initialize(name)
super("#{name}.formula")
sig { params(rack_name: String).void }
def initialize(rack_name)
super(:formula, HOMEBREW_CELLAR/rack_name)
end
end

# A lock file for a cask.
class CaskLock < LockFile
def initialize(name)
super("#{name}.cask")
sig { params(cask_token: String).void }
def initialize(cask_token)
super(:cask, HOMEBREW_PREFIX/"Caskroom/#{cask_token}")
MikeMcQuaid marked this conversation as resolved.
Show resolved Hide resolved
end
end

# A lock file for a download.
class DownloadLock < LockFile
sig { params(download_path: Pathname).void }
def initialize(download_path)
super(:download, download_path)
end
end
3 changes: 3 additions & 0 deletions Library/Homebrew/sorbet/rbi/dsl/homebrew/env_config.rbi

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Library/Homebrew/test/exceptions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,9 @@ class Baz < Formula; end
end

describe OperationInProgressError do
subject(:error) { described_class.new("foo") }
subject(:error) { described_class.new(Pathname("foo")) }

it(:to_s) { expect(error.to_s).to match(/Operation already in progress for foo/) }
it(:to_s) { expect(error.to_s).to match(/has already locked foo/) }
end

describe FormulaInstallationAlreadyAttemptedError do
Expand Down
6 changes: 3 additions & 3 deletions Library/Homebrew/test/lock_file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require "lock_file"

RSpec.describe LockFile do
subject(:lock_file) { described_class.new("foo") }
subject(:lock_file) { described_class.new(:lock, Pathname("foo")) }

describe "#lock" do
it "does not raise an error when already locked" do
Expand All @@ -16,7 +16,7 @@
lock_file.lock

expect do
described_class.new("foo").lock
described_class.new(:lock, Pathname("foo")).lock
end.to raise_error(OperationInProgressError)
end
end
Expand All @@ -30,7 +30,7 @@
lock_file.lock
lock_file.unlock

expect { described_class.new("foo").lock }.not_to raise_error
expect { described_class.new(:lock, Pathname("foo")).lock }.not_to raise_error
end
end
end
29 changes: 19 additions & 10 deletions Library/Homebrew/utils/lock.sh
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
# create a lock using `flock(2)`. A name is required as first argument.
# the lock will be automatically unlocked when the shell process quits.
# Noted due to the fixed FD, a shell process can only create one lock.
# Create a lock using `flock(2)`. A command name with arguments is required as
# first argument. The lock will be automatically unlocked when the shell process
# quits. Note due to the fixed FD, a shell process can only create one lock.
# HOMEBREW_LIBRARY is by brew.sh
# HOMEBREW_PREFIX is set by extend/ENV/super.rb
# shellcheck disable=SC2154
lock() {
local name="$1"
local command_name_and_args="$1"
# use bash to replace spaces with dashes
local lock_filename="${command_name_and_args// /-}"
local lock_dir="${HOMEBREW_PREFIX}/var/homebrew/locks"
local lock_file="${lock_dir}/${name}"
local lock_file="${lock_dir}/${lock_filename}"
[[ -d "${lock_dir}" ]] || mkdir -p "${lock_dir}"
if [[ ! -w "${lock_dir}" ]]
then
odie <<EOS
Can't create ${name} lock in ${lock_dir}!
Can't create \`brew ${command_name_and_args}\` lock in ${lock_dir}!
Fix permissions by running:
sudo chown -R ${USER-\$(whoami)} ${HOMEBREW_PREFIX}/var/homebrew
EOS
Expand All @@ -28,18 +30,25 @@ EOS
exec 200>&-
# open the lock file to FD, so the shell process can hold the lock.
exec 200>"${lock_file}"
if ! _create_lock 200 "${name}"

if ! _create_lock 200 "${command_name_and_args}"
then
local lock_context
if [[ -n "${HOMEBREW_LOCK_CONTEXT}" ]]
then
lock_context="\n${HOMEBREW_LOCK_CONTEXT}"
fi

odie <<EOS
Another active Homebrew ${name} process is already in progress.
Another \`brew ${command_name_and_args}\` process is already running.${lock_context}
Please wait for it to finish or terminate it to continue.
EOS
fi
}

_create_lock() {
local lock_file_descriptor="$1"
local name="$2"
local command_name_and_args="$2"
local ruby="/usr/bin/ruby"
local python="/usr/bin/python"
[[ -x "${ruby}" ]] || ruby="$(type -P ruby)"
Expand All @@ -57,6 +66,6 @@ _create_lock() {
then
"${python}" -c "import fcntl; fcntl.flock(${lock_fd}, fcntl.LOCK_EX | fcntl.LOCK_NB)"
else
onoe "Cannot create ${name} lock due to missing/too old ruby/flock/python, please avoid running Homebrew in parallel."
onoe "Cannot create \`brew ${command_name_and_args}\` lock due to missing/too old ruby/flock/python, please avoid running Homebrew in parallel."
fi
}
5 changes: 5 additions & 0 deletions docs/Manpage.md
Original file line number Diff line number Diff line change
Expand Up @@ -3872,6 +3872,11 @@ command execution e.g. `$(cat file)`.
`$XDG_CONFIG_HOME` is set or `$HOME/.homebrew/livecheck_watchlist.txt`
otherwise.

`HOMEBREW_LOCK_CONTEXT`

: If set, Homebrew will add this output as additional context for locking
errors. This is useful when running `brew` in the background.

`HOMEBREW_LOGS`

: Use this directory to store log files.
Expand Down
3 changes: 3 additions & 0 deletions manpages/brew.1
Original file line number Diff line number Diff line change
Expand Up @@ -2526,6 +2526,9 @@ Consult this file for the list of formulae to check by default when no formula a
\fIDefault:\fP \fB$XDG_CONFIG_HOME/homebrew/livecheck_watchlist\.txt\fP if \fB$XDG_CONFIG_HOME\fP is set or \fB$HOME/\.homebrew/livecheck_watchlist\.txt\fP otherwise\.
.RE
.TP
\fBHOMEBREW_LOCK_CONTEXT\fP
If set, Homebrew will add this output as additional context for locking errors\. This is useful when running \fBbrew\fP in the background\.
.TP
\fBHOMEBREW_LOGS\fP
Use this directory to store log files\.
.RS
Expand Down
Loading