-
-
Notifications
You must be signed in to change notification settings - Fork 11.4k
Change formula to use sshuttle/sshuttle fork #39968
Conversation
|
||
head 'https://github.com/apenwarr/sshuttle.git' | ||
head 'https://github.com/sshuttle/sshuttle.git' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a test be added to do something more substantial than e.g. --version
or --help
? See cmake.rb
for an example of an application formula with a good test and tinyxml2.rb
for an example of a library formula with a good test. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - it did complain during the build. I suppose requirements have changed since this was first created (not by me).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
||
head 'https://github.com/apenwarr/sshuttle.git' | ||
head 'https://github.com/sshuttle/sshuttle.git' | ||
|
||
def install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the other two PRs, libexec.install Dir['*']
should be libexec.install Dir["src/*"]
now?
I'd also like clarification on #38581 whether this requires Yosemite. |
Yes, I still need to add a test. This is my first (attempted) contribution, so I'm bound to make a few mistakes. I've fixed the lib exec path. |
I only have access to Yosemite machines, so I can't directly test whether it will work on older releases. As for a test, I can't see anything that doesn't either require sudo access or starts listening to the network e.g. sshuttle --server, which I'm assuming isn't acceptable. I could use sshuttle --firewall without arguments, which should provoke a specific error message. Is that better than just --help? |
@verdurin Listening on the network is OK; you could start it up with |
Ping? |
I've tried to add a check that meets what @MikeMcQuaid wanted, please take a look. |
Looks like it fails on Mountain Lion, though the test passes locally on Yosemite. |
bin.write_exec_script libexec/'sshuttle' | ||
end | ||
test do | ||
cmd = '#{bin}/sshuttle --server' | ||
Open3.popen3(cmd) do |stdin, stdout, stderr, wait_thr| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to use the fork
method here instead? Up to you.
It might be a bit verbose but I prefer this approach, if that's okay. |
Problem is it isn't working on 10.8: http://bot.brew.sh/job/Homebrew%20Pull%20Requests/27300/version=mountain_lion/testReport/junit/brew-test-bot/mountain_lion/test_sshuttle/ |
That's true, as I did mention above, but does this fork work on 10.8 anyway? As I noted either in this PR or the other one, I don't have access to a 10.8 machine on which to test. |
@verdurin I don't know. We can guard it to 10.9 and above if that's what upstream supports with |
This'll need rebasing on master here before the next push. |
I added the version restriction and followed the instructions at https://github.com/Homebrew/homebrew/blob/master/share/doc/homebrew/How-To-Open-a-Homebrew-Pull-Request-(and-get-it-merged).md |
Looks like this still needs rebasing against master. If you could squash your commits when you do so, that'd be 🏆, Thanks! |
Forgive my Git ignorance, but trying to rebase isn't going too well: git rebase --interactive origin/master When you have resolved this problem, run "git rebase --continue". I see a similar message if I try to rebase to an updated upstream remote. git rebase homebrew/master When you have resolved this problem, run "git rebase --continue". |
You may find it easier at this point to do something like:
|
Thanks, @DomT4 This is the current status: git status Is this a safe state from which to push? Or do I need to collapse those 12 commits again? git branch -a
Thanks again for your patience on this. |
Make sure you've backed up your desired |
Still needs to be rebased on an up-to-date master apparently:
|
Ah, an alternative version was merged this morning in b76121d. If you want to open an updated PR to add the improved test and MacOS limit that'd be perfect. Thanks for all your effort in this PR! 🍸 |
This is a simple change to use the fork at https://github.com/sshuttle/sshuttle that includes fixes for sshuttle to work in recent releases of OS X. See the discussion at #37907