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

sslsplit 0.5.0 (new formula) #2401

Closed
wants to merge 1 commit into from
Closed

Conversation

@cgroschupp
Copy link
Contributor

@cgroschupp cgroschupp commented Jun 26, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --strict --online <formula> (after doing brew install <formula>)?

@cgroschupp cgroschupp force-pushed the cgroschupp:sslsplit-0.5.0 branch 3 times, most recently Jun 26, 2016
@DomT4
DomT4 reviewed Jun 26, 2016
View changes
Formula/sslsplit.rb Outdated
class Sslsplit < Formula
desc "man-in-the-middle attacks against SSL encrypted network connections"
homepage "https://www.roe.ch/SSLsplit"
url "http://mirror.roe.ch/rel/sslsplit/sslsplit-0.5.0.tar.bz2"

This comment has been minimized.

@DomT4

DomT4 Jun 26, 2016
Contributor

Looks like this works over https:// just fine.

This comment has been minimized.

@cgroschupp

cgroschupp Jun 26, 2016
Author Contributor

Done

@DomT4
DomT4 reviewed Jun 26, 2016
View changes
Formula/sslsplit.rb Outdated
depends_on "openssl"

def install
ENV.deparallelize

This comment has been minimized.

@DomT4

DomT4 Jun 26, 2016
Contributor

Can you report this upstream as a bug? Things should build in parallel.

This comment has been minimized.

@DomT4

DomT4 Aug 8, 2016
Contributor

Ping on this?

This comment has been minimized.

@cgroschupp

cgroschupp Aug 8, 2016
Author Contributor

Fixed in the develop branch: droe/sslsplit@197b60e

Waiting for new release.

@DomT4
DomT4 reviewed Jun 26, 2016
View changes
Formula/sslsplit.rb Outdated
args = %W[
PREFIX=#{prefix}
]
inreplace "GNUmakefile", "-o $(BINUID) -g $(BINGID)", "" # Remove UID 0,GID 0 setting for install

This comment has been minimized.

@DomT4

DomT4 Jun 26, 2016
Contributor

Would also be nice to report this upstream as well. It wouldn't be particularly difficult to implement an option in the Makefile to support this formally.

@DomT4
DomT4 reviewed Jun 26, 2016
View changes
Formula/sslsplit.rb Outdated
]
inreplace "GNUmakefile", "-o $(BINUID) -g $(BINGID)", "" # Remove UID 0,GID 0 setting for install
inreplace "GNUmakefile", "-o $(MANUID) -g $(MANGID)", "" # Remove UID 0,GID 0 setting for install
system "make", "test", *args

This comment has been minimized.

@DomT4

DomT4 Jun 26, 2016
Contributor

Surprised you need to specify the prefix for make test?

This comment has been minimized.

@cgroschupp

cgroschupp Jun 26, 2016
Author Contributor

I have removed the prefix from "make test".

@cgroschupp cgroschupp force-pushed the cgroschupp:sslsplit-0.5.0 branch 2 times, most recently Jun 26, 2016
@ghost ghost removed the needs response label Jun 26, 2016
@UniqMartin UniqMartin changed the title sslsplit 0.5.0 sslsplit 0.5.0 (new formula) Jul 6, 2016
@DomT4
DomT4 reviewed Aug 8, 2016
View changes
Formula/sslsplit.rb Outdated

def install
ENV.deparallelize
inreplace "GNUmakefile", "-o $(BINUID) -g $(BINGID)", "" # Remove UID 0,GID 0 setting for install

This comment has been minimized.

@DomT4

DomT4 Aug 8, 2016
Contributor

Should be using the block form of inreplace if inreplacing the same file twice.

This comment has been minimized.

@cgroschupp

cgroschupp Aug 8, 2016
Author Contributor

"inreplace" is not more necessary.
droe/sslsplit@a81dbdb

@DomT4
DomT4 reviewed Aug 8, 2016
View changes
Formula/sslsplit.rb Outdated

test do
assert_match version.to_s, shell_output("#{bin}/sslsplit -V 2>&1").split("\n").first
pid_webrick = Process.fork { exec "ruby", "-rwebrick", "-e", "s = WEBrick::HTTPServer.new(:Port => 8000); s.mount_proc(\"/\") { |req,res| res.body = \"sslsplit test\"} ; s.start" }

This comment has been minimized.

@DomT4

DomT4 Aug 8, 2016
Contributor

Shouldn't need to call Process directly.

This comment has been minimized.

@cgroschupp

cgroschupp Aug 8, 2016
Author Contributor

Ok

@ghost ghost removed the needs response label Aug 8, 2016
@cgroschupp cgroschupp force-pushed the cgroschupp:sslsplit-0.5.0 branch Aug 8, 2016
@DomT4
DomT4 reviewed Aug 10, 2016
View changes
Formula/sslsplit.rb Outdated
def install
if ! build.head?
ENV.deparallelize
inreplace "GNUmakefile", "-o $(BINUID) -g $(BINGID)", "" # Remove UID 0,GID 0 setting for install

This comment has been minimized.

@DomT4

DomT4 Aug 10, 2016
Contributor

Please use the inreplace do format when inreplacing the same file more than once, Thanks!

This comment has been minimized.

@cgroschupp

cgroschupp Sep 3, 2016
Author Contributor

Done

@DomT4
DomT4 reviewed Aug 10, 2016
View changes
Formula/sslsplit.rb Outdated
ensure
begin
Process.kill 9, pid_sslsplit_child
rescue Errno::ESRCH => ex

This comment has been minimized.

@DomT4

DomT4 Aug 10, 2016
Contributor

I don't think these rescues are particularly necessary given we don't use them anywhere else, but no strong feelings.

This comment has been minimized.

@cgroschupp

cgroschupp Sep 3, 2016
Author Contributor

Removed the rescues.

@dunn dunn added the needs response label Aug 10, 2016
@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Sep 3, 2016

@cgroschupp Could you update this based on the comments? Thanks!

@cgroschupp cgroschupp force-pushed the cgroschupp:sslsplit-0.5.0 branch Sep 3, 2016
@ghost ghost removed the needs response label Sep 3, 2016
depends_on "openssl"

def install
unless build.head?

This comment has been minimized.

@DomT4

DomT4 Sep 3, 2016
Contributor

Just to check, have these issues been fixed in HEAD?

This comment has been minimized.

@cgroschupp

cgroschupp Sep 3, 2016
Author Contributor

Yes, it's fixed in the development branch.
droe/sslsplit@a81dbdb

@DomT4
DomT4 reviewed Sep 3, 2016
View changes
Formula/sslsplit.rb Outdated
# Workaround to kill all processes from sslsplit
pid_sslsplit_child = `pgrep -P #{pid_sslsplit}`.to_i

build_version = build.head? ? version.commit : version.to_s

This comment has been minimized.

@DomT4

DomT4 Sep 3, 2016
Contributor

Don't really need the version test, since you've added a stronger test. I'd just cut this line & the one below.

This comment has been minimized.

@cgroschupp

cgroschupp Sep 3, 2016
Author Contributor

I have removed the version test.

Christian Groschupp
@cgroschupp cgroschupp force-pushed the cgroschupp:sslsplit-0.5.0 branch to 1916f5b Sep 3, 2016
@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Sep 4, 2016

Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.