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
add a ModifyCmdFunc ExecAllocatorOption #674
Conversation
ModifyCmdFunc allows a function to be passed which will run on the exec.Cmd used to start the browser. This can be used to prevent chromedp from automatically passing os signals through to managed browsers, and to prevent browsers from being automatically killed on program exit.
Looking into this. Tests pass on my local machine (OSX, go 1.14.4), but appear to hang here in CI. If anyone knows why that might be, let me know. Thanks. |
Still unsure what's going on here. A fresh fork (without my changes) hangs when running tests sometimes (not all the time). I've observed this on both Ubuntu and OSX now (go 14 both places). |
Okay, I'd like to get someone else to take a look at this if possible, as I no longer believe the issue I'm having is related to the PR. At least in the environment I'm running in, a fresh clone of chromedp/chromedp is leading to tests that hang. Opening up a separate issue with more details ( #675 ). |
This should also resolve #562. |
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.
Also, like you said, the CI failure is an existing flake that can be ignored here for now. |
I've noticed a bit of a drop off in the maintenance on this repository lately. No offense intended, and of course I am grateful for the work of all the developers in creating and releasing this great tool. However, I want to ask for my own planning purposes (especially given than @mvdan says he no longer has write privileges): Is the plan to actively maintain the repo moving forward, or has it become less of a priority? |
@pmurley yes, the repository is actively maintained. |
@kenshaw Glad to hear it! Anything else I can do in regard to this PR? |
Just pinging on this PR again. |
@pmurley why not just use |
Timezone doesn't have anything to do with the main added functionality of this PR. This PR is intended to allow for modifying the code that starts the browser process so that we can choose whether we want to pass OS signals through the browser to the browser or not. I'm assuming you are talking about my code in the test case I introduced? In this case, yes, I could have done it another way, but I was trying to be consistent with existing tests. |
@pmurley I feel the injection way to customize the cmd is a little bit hacky. Here's my solution, you don't have to use rod, you can use the launcher lib only to just get the control URL: package launcher_test
import (
"os/exec"
"github.com/go-rod/rod/lib/launcher"
"github.com/go-rod/rod/lib/utils"
)
func Example_custom_launch() {
// get the browser executable path
bin, err := launcher.NewBrowser().Get()
utils.E(err)
// use the helper to construct args, this line is optional, you can construct the args manually
args := launcher.New().Headless(false).Env("TZ=Asia/Tokyo").FormatArgs()
parser := launcher.NewURLParser()
cmd := exec.Command(bin, args...)
cmd.Stderr = parser
err = cmd.Start()
utils.E(err)
<-parser.URL // the URL you get can be used with chromedp
} So that you can do whatever you like with the cmd, such as pipe both the browser's stdout and stderr to os.Stdout: cmd.Stdout = os.Stdout
cmd.Stderr = io.MultiWriter(os.Stdout, parser) |
@kenshaw Can we either get this merged or get some feedback to improve 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.
thanks! this is SGTM, @kenshaw do you anything to add here?
ping @kenshaw |
Closing due to lack of dialogue/action over the last couple months. |
I think this is a better solution, commit 7073d01 just use a little trick |
@pmurley Thank you for your work! Do you mind that I reopen this PR and get it merged? |
I don't mind at all -- Go for it! |
@pmurley Thank you very much! |
ModifyCmdFunc allows a function to be passed which will run
on the exec.Cmd used to start the browser. This can be used to
prevent chromedp from automatically passing os signals through
to managed browsers, and to prevent browsers from being automatically
killed on program exit.
Addresses #670