-
-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
goproxy: avoid subshells #90701
goproxy: avoid subshells #90701
Conversation
Formula/goproxy.rb
Outdated
@@ -25,8 +25,8 @@ def install | |||
test do | |||
bind_address = "127.0.0.1:#{free_port}" | |||
begin | |||
server = IO.popen("#{bin}/goproxy -proxy=https://goproxy.io -listen=#{bind_address}", err: [:child, :out]) | |||
sleep 1 | |||
server = IO.popen("#{bin}/goproxy", "-proxy=https://goproxy.io", "-listen=#{bind_address}", err: [:child, :out]) |
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.
I think this needs to be:
server = IO.popen("#{bin}/goproxy", "-proxy=https://goproxy.io", "-listen=#{bind_address}", err: [:child, :out]) | |
server = IO.popen(["#{bin}/goproxy", "-proxy=https://goproxy.io", "-listen=#{bind_address}"], err: [:child, :out]) |
because it takes a string mode
as a second argument.
Though we should probably do a run without any changes to see if it is reproducible otherwise it's a pointless comparison.
Maybe even remove the Process.kill
stuff to try force it.
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.
I don't think it's reproducible when testing just this formula by itself, but: https://github.com/Homebrew/homebrew-core/actions/runs/1552430219
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.
Temporarily removing the Process.kill
and wait
stuff might be a way to make it happen. It'll either get auto-killed after 5 minutes or not at all. I'm mostly concerned about the timeout bypass.
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.
Ok, let's try that.
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.
Yes, it looks like omitting Process.kill
and Process.wait
causes the test to hang.
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.
Ah, but it still gets caught by the timeout.
This seems to make the test hang indefinitely (e.g. Homebrew#90350). Let's also sleep for longer since short `sleep`s tend to cause trouble in CI.
cfdb792
to
19ccb57
Compare
Also get rid of the `ensure` block to see if we can get this to start timing out. This reverts commit 19ccb57.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?Subshells seem to make the test hang indefinitely (e.g. #90350). Let's also
sleep for longer since short
sleep
s tend to cause trouble in CI.