binutils: make `keg_only` #33419

zbeekman · 2018-10-26T16:14:19Z

gnm is broken with macOS 10.14 Mojave/XCode 10. This breaks gcc when
binutils is linked.

Fixes: #32516

See also: https://sourceware.org/bugzilla/show_bug.cgi?id=23728

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

zbeekman · 2018-10-26T16:15:16Z

I also changed the test do block, since gnm is currently broken on mojave/XCode10.

sjackman · 2018-10-26T17:32:56Z

Formula/binutils.rb

@@ -32,6 +38,6 @@ def install
  end

  test do
-    assert_match "main", shell_output("#{bin}/gnm #{bin}/gnm")
+    assert_match "__TEXT", shell_output("#{bin}/gstrings #{bin}/gstrings")


Rather than an using an internal symbol like __TEXT (which though unlikely could change) I'd suggest using some text from strings --help such as the word Usage.

That's a great idea!

vszakats · 2018-10-26T17:43:23Z

Formula/binutils.rb

@@ -12,6 +13,11 @@ class Binutils < Formula
    sha256 "7c693e68ebfcd654fd781480e68469e8f5af93c7397754a8caae1937322feb8f" => :el_capitan
  end

+  # See sourceware.org/bugzilla/show_bug.cgi?id=23728 and


Suggested change

# See sourceware.org/bugzilla/show_bug.cgi?id=23728 and

# See https://sourceware.org/bugzilla/show_bug.cgi?id=23728 and

fxcoudert · 2018-10-27T08:30:08Z

Formula/binutils.rb

+  # https://github.com/Homebrew/homebrew-core/issues/32516
+  keg_only :provided_by_macos,
+           "because Apple provides binutils and gnm is broken and breaks gcc"
+
  # No --default-names option as it interferes with Homebrew builds.


Remove the comment, which is now irrelevant.

The --program-prefix=g configure option should go too, since we don't put these tools in PATH anymore.

@fxcoudert Which comment? The # No --default-names ... one? Also, should we add back --default-names?

Better to create a directory libexec/bin of symlinks with the default names, that users can add to their path if they desire.

Also, I searched for explicit dependencies on binutils, and came up with 4 formula:

crosstool-ng

lightning

pkgdiff

pwntools

(I used brew uses --include-build --include-test --include-optional --recursive binutils to search.)

I went through the four formula above and none had any obvious adjustments that were required in the formula itself, however, virtually all of them require access to binutils binaries at runtime. Will the cause issues because the binutils executables are not on the default PATH anymore? Would formula revision bumps solve this, if it is indeed an issue?

Formula/binutils.rb

zbeekman · 2018-10-28T20:28:47Z

@Homebrew/core It looks like pwntools 3.12.0 is failing due to Gallopsled/pwntools#1191. I'm having trouble figuring out what is causing the build failure for pwntools to 3.12.1. There are a bunch of undefined symbols. Do we have to add missing (new) dependencies?

I sort of pinged upstream in Gallopsled/pwntools#683.

Should I just leave pwntools as is and merge binutils?

MikeMcQuaid · 2018-10-29T13:40:19Z

Should I just leave pwntools as is and merge binutils?

@zbeekman We should avoid merging PRs we know will break other software in Homebrew/homebrew-core.

zbeekman · 2018-10-29T15:00:11Z

We should avoid merging PRs we know will break other software in Homebrew/homebrew-core.

@MikeMcQuaid I agree. I was (erroneously) under the impression that changes outside my binutils PR were responsible for breaking pwn tools.

I tested building pwntools 3.12.0 and 3.12.1 locally, without the binutils changes and the do in fact build for me. Since pwntools 3.12.1 fixes a bug that the binutils keg_only migration seems to trigger before the build fails in different ways, I'll open a PR just for upgrading pwntools, while I investigate what needs to be done to get it working with migrated binutils.

MikeMcQuaid · 2018-10-29T15:07:07Z

@zbeekman Sounds good, thanks 👍

zbeekman · 2018-10-29T20:30:35Z

Summary:

binutils was made keg_only
Installed binaries no longer have the g prefix, e.g., gnm -> nm
Symbolic links put in bin dir with the g prefix to ensure backwards compatibility for folks relying on, e.g., gstrings
Revision bump to trigger re-installation

fxcoudert · 2018-10-29T20:35:01Z

Formula/binutils.rb

+  # See https://sourceware.org/bugzilla/show_bug.cgi?id=23728 and
+  # https://github.com/Homebrew/homebrew-core/issues/32516
+  keg_only :provided_by_macos,
+           "because Apple provides binutils and gnm is broken and breaks gcc"


Either we decide that keg-only is temporary, and the comment (and links) is adequate, or we decide that even when gnm is fixed we'd prefer binutils to stay keg-only (which I think is preferable). But then discussion about gnm bugs is irrelevant.

decide that even when gnm is fixed we'd prefer binutils to stay keg-only (which I think is preferable)

👍 I'll remove comments.

`gnm` is broken with macOS 10.14 Mojave/XCode 10. This breaks gcc when binutils is linked. Fixes: Homebrew#32516 See also: https://sourceware.org/bugzilla/show_bug.cgi?id=23728

- Putting binutils on the path at build time breaks the build - The test passes without the binutils dependency

zbeekman added maintainer feedback Additional maintainers' opinions may be needed 10.14 Mojave is specifically affected xcode10 Xcode 10 is specifically affected labels Oct 26, 2018

sjackman reviewed Oct 26, 2018

View reviewed changes

vszakats reviewed Oct 26, 2018

View reviewed changes

zbeekman force-pushed the binutils branch from 5fbf3a6 to a3de5c8 Compare October 26, 2018 19:43

zbeekman added ready to merge PR can be merged once CI is green and removed maintainer feedback Additional maintainers' opinions may be needed labels Oct 26, 2018

fxcoudert reviewed Oct 27, 2018

View reviewed changes

zbeekman force-pushed the binutils branch from a3de5c8 to 5666f9f Compare October 27, 2018 18:56

zbeekman commented Oct 27, 2018

View reviewed changes

Formula/binutils.rb Show resolved Hide resolved

zbeekman added maintainer feedback Additional maintainers' opinions may be needed and removed ready to merge PR can be merged once CI is green labels Oct 27, 2018

zbeekman force-pushed the binutils branch 2 times, most recently from 8016b79 to b325233 Compare October 28, 2018 19:19

zbeekman mentioned this pull request Oct 28, 2018

Capstone/Keystone vs. Binutils Gallopsled/pwntools#683

Closed

zbeekman force-pushed the binutils branch from b325233 to 28acd0b Compare October 28, 2018 19:35

zbeekman mentioned this pull request Oct 29, 2018

pwntools 3.12.1 #33485

Closed

zbeekman force-pushed the binutils branch 2 times, most recently from 8f6cec2 to 61130fb Compare October 29, 2018 19:56

zbeekman removed the maintainer feedback Additional maintainers' opinions may be needed label Oct 29, 2018

fxcoudert approved these changes Oct 29, 2018

View reviewed changes

zbeekman force-pushed the binutils branch from 61130fb to e7ac2e8 Compare October 29, 2018 20:56

binutils: make keg_only

dedb3a7

`gnm` is broken with macOS 10.14 Mojave/XCode 10. This breaks gcc when binutils is linked. Fixes: Homebrew#32516 See also: https://sourceware.org/bugzilla/show_bug.cgi?id=23728

pwntools: remove dependency on binutils

a302ca7

- Putting binutils on the path at build time breaks the build - The test passes without the binutils dependency

zbeekman force-pushed the binutils branch from 3a55e83 to a302ca7 Compare October 29, 2018 23:53

zbeekman closed this in a85359b Oct 30, 2018

lock bot added the outdated PR was locked due to age label Nov 29, 2018

lock bot locked as resolved and limited conversation to collaborators Nov 29, 2018

zbeekman deleted the binutils branch April 11, 2019 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

binutils: make `keg_only` #33419

binutils: make `keg_only` #33419

zbeekman commented Oct 26, 2018 •

edited

zbeekman commented Oct 26, 2018

sjackman Oct 26, 2018

zbeekman Oct 26, 2018

vszakats Oct 26, 2018

fxcoudert Oct 27, 2018

zbeekman Oct 27, 2018

sjackman Oct 27, 2018

zbeekman Oct 27, 2018 •

edited

zbeekman Oct 27, 2018

zbeekman commented Oct 28, 2018

MikeMcQuaid commented Oct 29, 2018

zbeekman commented Oct 29, 2018

MikeMcQuaid commented Oct 29, 2018

zbeekman commented Oct 29, 2018

fxcoudert Oct 29, 2018

zbeekman Oct 29, 2018

	# See sourceware.org/bugzilla/show_bug.cgi?id=23728 and
	# See https://sourceware.org/bugzilla/show_bug.cgi?id=23728 and

binutils: make keg_only #33419

binutils: make keg_only #33419

Conversation

zbeekman commented Oct 26, 2018 • edited

zbeekman commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbeekman Oct 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbeekman commented Oct 28, 2018

MikeMcQuaid commented Oct 29, 2018

zbeekman commented Oct 29, 2018

MikeMcQuaid commented Oct 29, 2018

zbeekman commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binutils: make `keg_only` #33419

binutils: make `keg_only` #33419

zbeekman commented Oct 26, 2018 •

edited

zbeekman Oct 27, 2018 •

edited