redress 1.2.0 (new formula)

[0xdevalias]

• Have you followed the guidelines for contributing?
• Have you ensured that your commits follow the commit style guide?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

---

Add new formula for redress CLI:

• https://github.com/goretk/redress

  • Redress - A tool for analyzing stripped binaries
    The redress software is a tool for analyzing stripped Go binaries compiled with the Go compiler. It extracts data from the binary and uses it to reconstruct symbols and performs analysis. It essentially tries to "re-dress" a "stripped" binary.

[0xdevalias]

@chenrui333 Looks like this is finally ready to go 🎉

[SMillerDev]

Can't it figure out the go version from the compiler?
And what is the gore version for?

Suggested change

	# https://github.com/goretk/redress/blob/develop/Makefile#L11-L14
	redress_version = version
	gore_version = File.read(buildpath/"go.mod").scan(%r{goretk/gore v(\S+)}).flatten.first
	go_version = Formula["go"].version
	ldflags = %W[
	-s -w
	-X main.redressVersion=#{redress_version}
	-X main.goreVersion=#{gore_version}
	-X main.compilerVersion=#{go_version}
	]
	# https://github.com/goretk/redress/blob/develop/Makefile#L11-L14
	gore_version = File.read(buildpath/"go.mod").scan(%r{goretk/gore v(\S+)}).flatten.first
	ldflags = %W[
	-s -w
	-X main.redressVersion=#{version}
	-X main.goreVersion=#{gore_version}
	-X main.compilerVersion=#{Formula["go"].version}
	]

[0xdevalias]

These are flags taken directly out of the Makefile for the project; and match the implementation there.

I haven't deeply evaluated the source code to see if/how/where they make use of them.

Gore is a library it depends on.

Presumably it uses these for the version command or similar.

[SMillerDev]

Can we check what happens if we don't pass them?

[0xdevalias]

Anything is possible. Though I would ask why and whether it's important to spend time doing so.

Wouldn't it make the most sense to keep the way homebrew builds the program as close to the way it's officially built?

That way it prevents introducing issues in the homebrew version through diverging from the official build.

---

Makefile defining the variables as I implemented them here:

• https://github.com/goretk/redress/blob/master/Makefile#L11-L14

Variables set in code by those ldflags:

• https://github.com/goretk/redress/blob/develop/main.go#L19-L22

Which are used later on in that same file:

• https://github.com/goretk/redress/blob/develop/main.go#L42-L44

Not setting them would not be in line with users expectations of how the CLI reports it's versions.

[SMillerDev]

But if we wanted to do that, why not run the makefile? We're now adding a second implementation of the same logic that is up to the Homebrew community to keep in sync and maintain.

[SMillerDev]

If you don't want to use the upstream version because you think it's too much code and don't want to try and reduce your ruby copy of it here I'm afraid we're at an impasse, because I don't want this merged before we at least attempt to minimise the code in this formula.

[0xdevalias]

I'm afraid we're at an impasse, because I don't want this merged before we at least attempt to minimise the code in this formula.

I'm confused.. I don't believe I said anything about not being open to reducing code here?

All I've done is answer your questions and provide relevant context?

Aside from a high level 'attempt to minimise the code', do you have more concrete suggestions of areas that are currently 'unacceptable'; that I haven't already spoken to above/elsewhere?

[0xdevalias]

Now that I'm at a computer, I've accepted the suggested changes above in 2548aa0

Though since the enforced standard is that I need to manually squash commits (which is why I didn't accept them straight away, as that workflow doesn't work with squashed commits), that reference will no longer be valid after I force push.

[0xdevalias]

@SMillerDev Does the above satisfy your desires for landing this PR? Or do you still consider it unmergeable in it's current state?

[0xdevalias]

See this comment for latest updates, which sort of sidesteps the core testing issue being discussed here:

• redress 1.2.0 (new formula) #155724 (comment)

[SMillerDev]

Can it check test_fixtures("elf/hello")?

[0xdevalias]

Not unless that test fixture is a golang binary.

---

Edit: Now that i'm on a computer, I tried this, and it technically runs, but IMO it's probably not really any better than a test that checks the version number of a CLI; it's not really demonstrating that the core functionality works well, and given this tool is only meant to be run against Golang binaries, there's a high likelihood that the test would break at some future point for no good reason.

Formula diff patch (doesn't currently pass the test):

diff --git a/Formula/r/redress.rb b/Formula/r/redress.rb
index c5ecfbcdc9e..333d7214be0 100644
--- a/Formula/r/redress.rb
+++ b/Formula/r/redress.rb
@@ -52,10 +52,12 @@ class Redress < Formula
     else
       # on supported architectures, redress can just check it's own binary
       test_module_root = "github.com/goretk/redress"
-      test_bin_path = bin/"redress"
+      # test_bin_path = bin/"redress"
+      test_bin_path = test_fixtures("elf/hello")
     end
 
     output = shell_output("#{bin}/redress info '#{test_bin_path}'")
+    print output
     assert_match(/Main root\s+#{Regexp.escape(test_module_root)}/, output)
   end
 end

Output from CLI:

..snip..

==> Testing redress
==> /usr/local/Cellar/redress/1.1.1/bin/redress version
==> /usr/local/Cellar/redress/1.1.1/bin/redress info '/usr/local/Homebrew/Library/Homebrew/test/support/fixtures/elf/hello'
OS        EM_X86_64
Arch      amd64
# main    0
# std     0
# vendor  0

..snip..

There is no "Main root" in the output when run against a non-Golang binary, which is what we were using to demonstrate that the CLI is working as intended.

[SMillerDev]

Is there some hello-world binary we can download somewhere?

[0xdevalias]

Somewhere unrelated on the internet; maybe. Though I'm not sure how it would be a better option to download an arbitrary binary just to run in the test?

This is in line with how tests are implemented for other Golang formula like this; which is what I based it off. (Ref)

Everything I've done here was already discussed earlier on this PR (including alternative options) and the approach taken was based on that discussion, eg.:

• Alternative options were considered in this comment
• References to other formula that depend on Golang for their tests (and that use the same pattern of 'compile a small test binary') were listed and used as references in this comment
• Confirming that the additional test logic worked around the initial problem when building on Apple Silicon in this comment
• Which was all confirmed as a good direction to go in by a maintainer in this comment
• etc

If you think this pattern is common enough among current formula to warrant extracting some helpers/similar into homebrew so that the boilerplate in each individual formula is abstracted; that could ultimately also be a good solution; but:

• a) from my brief review, I don't think there are enough formula currently implementing this that it would be worthwhile
• b) even if it was something to be done, feels like something that doesn't need to block this PR, and could be carried out as an improvement to homebrew separately after the fact

[chenrui333]

it does not have to be that arbitrary (as it can be trusted domain hosted on github.com or something alike) and we also use checksum to guard the integrity of the binary.

Let me see what I can help with.

(Sorry about the hassle, we are trying to get it right from the beginning :) )

[0xdevalias]

it does not have to be that arbitrary (as it can be trusted domain hosted on github.com or something alike) and we also use checksum to guard the integrity of the binary.

@chenrui333 I guess by 'arbitrary', I was less concerned about file integrity (because checksum/etc), and more that it just seems like a worse solution to download a random binary from somewhere that's unrelated to the project/homebrew, just to avoid using an existing pattern of compiling a small binary to be used in the tests.

Let me see what I can help with.

@chenrui333 I'd appreciate that.

(Sorry about the hassle, we are trying to get it right from the beginning :) )

@chenrui333 nods even though it's frustrating from a "trying to contribute" perspective, I do understand the 'why' of it, at least at a high level (even if I don't personally agree with the specifics of it in this instance)

[0xdevalias]

See this comment for latest updates, which sort of sidesteps the core testing issue being discussed here:

• redress 1.2.0 (new formula) #155724 (comment)

[0xdevalias]

@chenrui333 @SMillerDev Bumped from 1.1.1 to 1.2.0, which seemed to add support for processing ARM binaries as per:

Please check if it's still an issue with the latest release

@TcM1911 From a quick initial test, I don't think so. I'm running the original test that failed currently, so will check if that works.

Looking at the commit changes, I didn't see where/how this would have been fixed though. Are you able to elaborate on where the expected fix came from?

I'm guessing it was probably this one:

• goretk/redress@058070b

  • Sync with gore develop branch

Along with this?

• Add support for MachO/ARM64 goretk/gore#72

Originally posted by @0xdevalias in goretk/redress#35 (comment)

Since that worked, I simplified the test code back to the original 'redress processes it's own binary as a test' method that I had before working around the ARM failures (which is also the same test process I used in #155716)

[chenrui333]

can the ' be removed?

Suggested change

	output = shell_output("#{bin}/redress info '#{test_bin_path}'")
	output = shell_output("#{bin}/redress info #{test_bin_path}")

[0xdevalias]

@chenrui333 It could.. but why? If I did that, then if test_bin_path is updated in future to contain spaces, it will break. It was explicitly written this way to defensibly avoid creating future problems for maintainers.

It could also have been fully inlined like this, but I chose not to do that to improve the readability of it:

    test_module_root = "github.com/goretk/redress"
-   test_bin_path = bin/"redress"
-
-   output = shell_output("#{bin}/redress info '#{test_bin_path}'")
+   output = shell_output("#{bin}/redress info #{bin/"redress"}")

My previous PR that followed this same pattern had the ' and it wasn't an issue there?

• https://github.com/Homebrew/homebrew-core/pull/155716/files#diff-66f44763165ac3f00d41e3cb26f674db9a1be2a3f7572b46c6c26b9a302abfb8R26

test do
  json_output = JSON.parse(shell_output("#{bin}/goresym '#{bin}/goresym'"))
  assert_equal json_output["BuildInfo"]["Main"]["Path"], "github.com/mandiant/GoReSym"
end

[chenrui333]

yeah, it is fine, just some code style thing. sounds good then.

[chenrui333]

lgtm, just one nit comment

[github-actions]

@fxcoudert has requested bottles to be published to this PR.