search

Homebrew / brew

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

Error handler prints number of bytes instead of actual output #8244

Closed claui closed 4 years ago

claui commented 4 years ago

Please note we will close your issue without comment if you delete, do not read or do not fill out the issue checklist below and provide ALL the requested information. If you repeatedly fail to use the issue template, we will block you from ever submitting issues to Homebrew again.

What you were trying to do (and why)

I was trying to version bump a formula whose inline (:DATA) patch would no longer apply to the newer version.

What happened (include command output)

An error message is printed along with the length of the output.

(To make the case reproducible with minimal effort, I’m using the rp formula in the following output.)

Command output 

==> Patching
patching file CMakeLists.txt
Hunk #1 FAILED at 36.
1 out of 1 hunk FAILED -- saving rejects to file CMakeLists.txt.rej
==> Kept temporary files
Temporary files retained at /private/tmp/rp-20200807-70880-1k3vn48
NOTE:  Most system logs have moved to a new logging system.  See log(1) for more information.
Error: Failure while executing; `patch -g 0 -f -p1` exited with 1. Here's the output:
707

What you expected to happen

An error message is printed along with the actual output of the failed command.

Step-by-step reproduction instructions (by running brew commands)

  1. To simulate a failing patch, run brew edit rp.
  2. In the formula, change the word BeaEngine to BeerEngine.
  3. Save the modified formula.
  4. Run the following command line:
brew install -i --ignore-dependencies rp

(Note that neither -i nor --ignore-dependencies are needed for the error to occur, it’s just to make reproduction easier.)

Output of brew config and brew doctor commands

$ brew config
HOMEBREW_VERSION: 2.4.9-294-g934e70c
ORIGIN: https://github.com/Homebrew/brew.git
HEAD: 934e70c8ee5693df6f47a9bada3bb7b57227e121
Last commit: 2 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: ed3856d30e7e818f88ec4d82e8b8444e6f10cb1f
Core tap last commit: 2 hours ago
Core tap branch: master
HOMEBREW_PREFIX: /usr/local
HOMEBREW_BAT: set
HOMEBREW_CASK_OPTS: []
HOMEBREW_DEVELOPER: set
HOMEBREW_EDITOR: '/usr/local/bin/code' -w
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_MAKE_JOBS: 8
HOMEBREW_NO_ANALYTICS: set
HOMEBREW_NO_AUTO_UPDATE: set
CPU: octa-core 64-bit kabylake
Homebrew Ruby: 2.6.3 => /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/bin/ruby
Clang: 11.0 build 1100
Git: 2.28.0 => /usr/local/bin/git
Curl: 7.54.0 => /usr/bin/curl
Java: 14.0.2, 12.0.2, 11.0.8, 10.0.2, 9, 1.8.0_262, 1.8.0_202
macOS: 10.14.6-x86_64
CLT: 10.3.0.0.1.1562985497
Xcode: 11.3.1

$ brew doctor
Error: Cask 'tm-ledger' is unreadable: undefined method `method_missing_message' for Cask::Utils:Module
Bo98 commented 4 years ago

The actual output is printed a little bit above, though I don't know where the number in the error is coming from.

claui commented 4 years ago

though I don't know where the number in the error is coming from.

@Bo98 I think I can explain that part.

In patch.rb, we return the output of Ruby’s IO#write, which according to the API documentation, is the number of bytes written.

Where I’m actually stuck is in deciding whether it’s a bug in patch.rb or rather in Homebrew’s Utils::popen. The latter simply returns the result of the block to which it yields.

Bo98 commented 4 years ago

Oh I was looking ExternalPatch by mistake. That makes sense.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

claui commented 4 years ago

After leaving this sitting for a while, I’m now pretty sure that patch.rb just fails to observe the contracts of both Utils::popen and IO#write.

What the block should actually do is capture and return the process’s standard output.

Edit: see below.

claui commented 4 years ago

Changed my mind. Coordinating reads and writes from/to a child process, maybe even interleaved with each other, isn’t something you can do in a single line.

It would be unreasonable to demand from the caller of safe_popen_write to do that. The caller should only concern themselves with writing to stdin, while safe_popen_write should transparently capture stdout, ignoring the block return value.

PR incoming.