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

Hide the tor dos window on windows #32

Merged
merged 1 commit into from Jan 24, 2020
Merged

Conversation

dballard
Copy link
Contributor

@dballard dballard commented Aug 10, 2019

Currently when running on windows, tor.Start() will create a dos console box and leave it displaying with tor running in it.

This is a proposed PR that would hide it on windows

@cretz
Copy link
Owner

cretz commented Jan 20, 2020

I would suggest something more generic like type CmdCreatorFunc func(ctx context.Context, args ...string) (*exec.Cmd, error) so anyone could do anything they wanted to the cmd instead of this specific thing. Heck, you could then just change NewCreator to defer to that.

Also, it seems your latest commit changed the package names.

@dballard
Copy link
Contributor Author

dballard commented Jan 20, 2020

aaah shoot I'll use a separate branch. The package rename was just for internal testing, with our weird built system. wasn't meant to update here

and cool, once I get our system building, I'll take a look at making it more generic as suggested :)

@dballard dballard force-pushed the hidewindows branch 2 times, most recently from 01b7836 to e063c10 Compare Jan 22, 2020
@dballard
Copy link
Contributor Author

dballard commented Jan 22, 2020

Ok here's the proper PR as I believe you requested. And I moved the hide specific attr stuff into my code base and it works :)

}

// NewCreator creates a Creator for external Tor process execution based on the
// given exe path.
func NewCreator(exePath string) Creator {
return &exeProcessCreator{exePath}
func NewCreator(exePath string, newCmd CmdCreatorFunc) Creator {
Copy link
Owner

@cretz cretz Jan 22, 2020

Choose a reason for hiding this comment

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

This breaks backwards compatibility. Rather, I was thinking of having type CmdCreatorFunc func(ctx context.Context, args ...string) (*exec.Cmd, error) and then have that implement Creator via something like func (c CmdCreatorFunc) New(ctx context.Context, args ...string) (Process, error) { /* send back error or &exeProcess after calling c(ctx, args) */ }. And leave the rest of the code alone (or even better, change NewCreator to create and return a CmdCreatorFunc).

Sorry I'm too busy to do this myself, I hope that's clear.

@dballard
Copy link
Contributor Author

dballard commented Jan 23, 2020

oh cool, didn't know you could do that. sorry for being dense. I think this is what you were asking for?

@cretz
Copy link
Owner

cretz commented Jan 24, 2020

No problem, sorry for being pedantic. That's exactly what I was thinking! Can you do me a favor and run a go fmt on the file (I think a space or two may be removed around anon func sig, but unsure). Then I'll happily merge. Thanks.

@dballard
Copy link
Contributor Author

dballard commented Jan 24, 2020

done! (sorry havent set my windows IDE to auto gofmt like I'm used to)

@cretz
Copy link
Owner

cretz commented Jan 24, 2020

Thanks

@cretz cretz merged commit f9f678b into cretz:master Jan 24, 2020
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

2 participants