Homebrew / brew

🍺 The missing package manager for macOS (or Linux)
https://brew.sh
BSD 2-Clause "Simplified" License
40.77k stars 9.57k forks source link

HB performs an unnecessary copying of extracted sourceballs #5136

Closed chdiza closed 5 years ago

chdiza commented 5 years ago

I've noticed that when installing a formula, HB is extracting the source tarball to Dir1, and then for some reason is copying Dir1/* into Dir2. This seems totally unnecessary and doesn't match how HB used to behave.

Here's what it looks like now (with HOMEBREW_VERBOSE set). You can see that there is what appears to be a totally unnecessary copying:

==> Verifying ed7152de7b320534c4128639c05fdc149fe4db285abb8f62f8b89d6d21f631bc--blueutil-2.2.0.tar.xz checksum
tar xf /stuff/brache/downloads/ed7152de7b320534c4128639c05fdc149fe4db285abb8f62f8b89d6d21f631bc--blueutil-2.2.0.tar.xz -C /Users/chdiza/.tmp/tmpdir/d20181019-31151-4xjgj3
cp -pR /Users/chdiza/.tmp/tmpdir/d20181019-31151-4xjgj3/blueutil-2.2.0/. /Users/chdiza/.tmp/tmpdir/blueutil-20181019-31151-gjhv3j/blueutil-2.2.0

Why is it copying the unpacked contents from one temp dir to another, instead of either (a) moving them, or (b) just extracting them directly to the desired tmpdir in the first place? HB used to not do this. Here's what it used to look like:

==> Verifying blueutil-2.2.0.tar.xz checksum
tar xf /stuff/brache/blueutil-2.2.0.tar.xz -C /Users/chdiza/.tmp/tmpdir/blueutil-20181019-30795-1vujpyc

Copying them makes the build take longer (particularly for large source dirs, and particularly for sequences of many builds in a row like when I fresh-install all my packages from a script), and also is putting needless writes on my SSD.

Backtracking through the commit history, I've found that 556e76b037dcc65ea6 is the first commit to show the extra copy step in my local testing.

Doctor: Only the warning about HOMEBREW_VERBOSE, but it needs to be set in order to display the bug.

Config:

HOMEBREW_VERSION: 1.7.7-82-g4a0e97d
ORIGIN: https://github.com/Homebrew/brew
HEAD: 4a0e97d85a28921b3abe90c8a96094c58576136f
Last commit: 3 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 11df5159a651449eec40150aa8d80f5e69e166a1
Core tap last commit: 87 minutes ago
HOMEBREW_PREFIX: /usr/local
HOMEBREW_REPOSITORY: /usr/local/hb
HOMEBREW_CACHE: /stuff/brache
HOMEBREW_TEMP: /Users/chdiza/.tmp/tmpdir
HOMEBREW_BUILD_FROM_SOURCE: 1
HOMEBREW_DEV_CMD_RUN: 1
HOMEBREW_LOGS: /Users/chdiza/Library/Homebrew
HOMEBREW_NO_ANALYTICS: 1
HOMEBREW_NO_AUTO_UPDATE: 1
HOMEBREW_NO_EMOJI: 1
HOMEBREW_NO_GITHUB_API: 1
HOMEBREW_VERBOSE: 1
CPU: quad-core 64-bit ivybridge
Homebrew Ruby: 2.3.7 => /System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/bin/ruby
Clang: 10.0 build 1000
Git: 2.19.1 => /usr/local/bin/git
Curl: 7.54.0 => /usr/bin/curl
macOS: 10.13.6-x86_64
CLT: 10.0.0.0.1.1535735448
Xcode: 10.0
reitermarkus commented 5 years ago

We don't know if the tar step will be the last one, since there could be a nested archive inside it. We run cp -R as the last step since this works with all archive types. Sadly there is no such thing as mv -R.

chdiza commented 5 years ago

since this works with all archive types

Since we know the archive type (otherwise we don't know how to unarchive it), can't we: copy only if needed and move otherwise?

since there could be a nested archive inside it

But if there isn't, then HB should not be wastefully copying. Outside of cask, I'm not sure when this would ever happen anyway (except for the trivial case of .tar.[compression-suffix]).

MikeMcQuaid commented 5 years ago

Passing on this, sorry. Will review a PR.