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

add a ModifyCmdFunc ExecAllocatorOption #674

Merged
merged 2 commits into from May 9, 2021
Merged

Conversation

pmurley
Copy link
Contributor

@pmurley pmurley commented Aug 3, 2020

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

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.
@pmurley
Copy link
Contributor Author

pmurley commented Aug 3, 2020

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.

@pmurley
Copy link
Contributor Author

pmurley commented Aug 3, 2020

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).

@pmurley
Copy link
Contributor Author

pmurley commented Aug 3, 2020

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 ).

@joshrickard
Copy link

This should also resolve #562.

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, thank you for your work!

Unfortunately, I no longer have write access to this repository, so someone like @kenshaw or @rnd would have to hit the merge button.

@mvdan
Copy link
Contributor

mvdan commented Aug 12, 2020

Also, like you said, the CI failure is an existing flake that can be ignored here for now.

@falnyr
Copy link

falnyr commented Aug 14, 2020

bump @kenshaw @rnd

@pmurley
Copy link
Contributor Author

pmurley commented Aug 17, 2020

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?

@kenshaw
Copy link
Member

kenshaw commented Aug 25, 2020

@pmurley yes, the repository is actively maintained.

@pmurley
Copy link
Contributor Author

pmurley commented Aug 26, 2020

@kenshaw Glad to hear it! Anything else I can do in regard to this PR?

@pmurley
Copy link
Contributor Author

pmurley commented Sep 9, 2020

Just pinging on this PR again.

@ysmood
Copy link

ysmood commented Sep 9, 2020

@pmurley
Copy link
Contributor Author

pmurley commented Sep 9, 2020

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.

@ysmood
Copy link

ysmood commented Sep 13, 2020

@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)

@pmurley
Copy link
Contributor Author

pmurley commented Sep 13, 2020

@kenshaw Can we either get this merged or get some feedback to improve it?

@pmurley
Copy link
Contributor Author

pmurley commented Sep 29, 2020

ping @kenshaw @rnd. Any feedback would be appreciated.

Copy link
Contributor

@rnd rnd left a 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?

@pmurley
Copy link
Contributor Author

pmurley commented Oct 11, 2020

ping @kenshaw

@pmurley
Copy link
Contributor Author

pmurley commented Oct 19, 2020

Closing due to lack of dialogue/action over the last couple months.

@pmurley pmurley closed this Oct 19, 2020
@lostsnow
Copy link

I think this is a better solution, commit 7073d01 just use a little trick

@ZekeLu
Copy link
Member

ZekeLu commented May 9, 2021

@pmurley Thank you for your work! Do you mind that I reopen this PR and get it merged?

@pmurley
Copy link
Contributor Author

pmurley commented May 9, 2021

I don't mind at all -- Go for it!

@ZekeLu
Copy link
Member

ZekeLu commented May 9, 2021

@pmurley Thank you very much!

@ZekeLu ZekeLu reopened this May 9, 2021
@ZekeLu ZekeLu merged commit 538d83f into chromedp:master May 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants