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

Darwin port without CGO #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kazeburo
Copy link

@kazeburo kazeburo commented May 7, 2022

Remove CGO dependencies from Darwin port. use scutil command and parse the result instead of using CGO.
I've tested in macOS Monterey.

@mattn
Copy link
Owner

mattn commented May 8, 2022

cool. someone, could please confirm this PR? I dont have mac.

@shuntagami
Copy link

Tested work on my M1 Mac and is it enough?

~/projects/go-ieproxy feature/no-cgo-darwin
❯ uname -a
Darwin shuntagami.local 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:47:26 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T8101 arm64

~/projects/go-ieproxy feature/no-cgo-darwin
❯ go test ./...
ok  	github.com/mattn/go-ieproxy	(cached)
?   	github.com/mattn/go-ieproxy/autoload	[no test files]

@thara
Copy link

thara commented May 8, 2022

LGTM (on M1 iMac 24-inch)

$ sw_vers
ProductName:    macOS
ProductVersion: 12.3.1
BuildVersion:   21E258
$ go test ./... -count 1 -v
=== RUN   TestPacfile
--- PASS: TestPacfile (0.05s)
=== RUN   TestOverrideEnv
--- PASS: TestOverrideEnv (0.00s)
=== RUN   TestParseConf
2022/05/08 21:24:29 HTTP Server Error -  accept tcp 127.0.0.1:53103: use of closed network connection
--- PASS: TestParseConf (0.00s)
PASS
ok      github.com/mattn/go-ieproxy     0.176s
?       github.com/mattn/go-ieproxy/autoload    [no test files]

@shogo82148
Copy link

It looks fine on my macOS Catalina.

スクリーンショット 2022-05-08 21 28 56

No proxy settings:

スクリーンショット 2022-05-08 21 27 59

# @v0.0.6
$ go run main.go
ieproxy.ProxyConf{Static:ieproxy.StaticProxyConf{Active:false, Protocols:map[string]string(nil), NoProxy:""}, Automatic:ieproxy.ProxyScriptConf{Active:false, PreConfiguredURL:""}}

# @v0.0.7-0.20220507162033-fb658f399d9d
$ go run main.go
ieproxy.ProxyConf{Static:ieproxy.StaticProxyConf{Active:false, Protocols:map[string]string(nil), NoProxy:""}, Automatic:ieproxy.ProxyScriptConf{Active:false, PreConfiguredURL:""}}

With proxy settings:
PreConfiguredURL
HTTP Proxy
HTTPS Proxy

# @v0.0.6
$ go run main.go
ieproxy.ProxyConf{Static:ieproxy.StaticProxyConf{Active:true, Protocols:map[string]string{"http":"example.jp:8081", "https":"example.com:8080"}, NoProxy:"*.local,169.254/16,localhost,127.0.0.1,localhost:8080"}, Automatic:ieproxy.ProxyScriptConf{Active:true, PreConfiguredURL:"https://example.com/foo.pac"}}

# @v0.0.7-0.20220507162033-fb658f399d9d
$ go run main.go
ieproxy.ProxyConf{Static:ieproxy.StaticProxyConf{Active:true, Protocols:map[string]string{"http":"example.jp:8081", "https":"example.com:8080"}, NoProxy:"*.local,169.254/16,localhost,127.0.0.1,localhost:8080"}, Automatic:ieproxy.ProxyScriptConf{Active:true, PreConfiguredURL:"https://example.com/foo.pac"}}

httpsPort := C.CFNumberRef(C.CFDictionaryGetValue(cfDictProxy, unsafe.Pointer(C.kCFNetworkProxiesHTTPSPort)))

httpProxy := fmt.Sprintf("%s:%d", cfStringGetGoString(httpsHost), cfNumberGetGoInt(httpsPort))
httpProxy := fmt.Sprintf("%s:%s", proxyMap["HTTPSProxy"], proxyMap["HTTPSPort"])

Choose a reason for hiding this comment

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

It should be:

Suggested change
httpProxy := fmt.Sprintf("%s:%s", proxyMap["HTTPSProxy"], proxyMap["HTTPSPort"])
httpProxy := net.JoinHostPort(proxyMap["HTTPSProxy"], proxyMap["HTTPSPort"])

shouldn't it?
It may not work with IPv6 addresses.

@shogo82148
Copy link

oops, I've forgotten pasting my code...

My test code is:

package main

import (
	"fmt"

	"github.com/mattn/go-ieproxy"
)

func main() {
	conf := ieproxy.GetConf()
	fmt.Printf("%#v\n", conf)
}

go.mod is:

module main

go 1.18

require github.com/mattn/go-ieproxy v0.0.6

require (
	golang.org/x/net v0.0.0-20220107192237-5cfca573fb4d // indirect
	golang.org/x/sys v0.0.0-20220110181412-a018aaa089fe // indirect
	golang.org/x/text v0.3.7 // indirect
)
module main

go 1.18

require github.com/mattn/go-ieproxy v0.0.6

require (
	golang.org/x/net v0.0.0-20220107192237-5cfca573fb4d // indirect
	golang.org/x/sys v0.0.0-20220110181412-a018aaa089fe // indirect
	golang.org/x/text v0.3.7 // indirect
)

replace github.com/mattn/go-ieproxy => github.com/kazeburo/go-ieproxy v0.0.7-0.20220507162033-fb658f399d9d

@gongqi-zhen
Copy link

The test passed. (on macOS Big Sur 11.6.5)

% git remote -v
kazeburo        https://github.com/kazeburo/go-ieproxy.git (fetch)
kazeburo        https://github.com/kazeburo/go-ieproxy.git (push)
origin  https://github.com/mattn/go-ieproxy.git (fetch)
origin  https://github.com/mattn/go-ieproxy.git (push)
% git branch
  master
* topic/check-pull-request-cgo
% sw_vers 
ProductName:    macOS
ProductVersion: 11.6.5
BuildVersion:   20G527
% go test -v ./...
=== RUN   TestPacfile
2022/05/08 21:36:19 HTTP Server Error -  accept tcp 127.0.0.1:55672: use of closed network connection
--- PASS: TestPacfile (0.03s)
=== RUN   TestOverrideEnv
--- PASS: TestOverrideEnv (0.00s)
=== RUN   TestParseConf
--- PASS: TestParseConf (0.00s)
PASS
ok      github.com/mattn/go-ieproxy     (cached)
?       github.com/mattn/go-ieproxy/autoload    [no test files]
%

@mattn
Copy link
Owner

mattn commented May 9, 2022

@laurazard from a portability point of view, I would like to replace the darwin port with this implementation. is there a problem?

@JohnRusk @adreed-msft @sethvargo

@laurazard
Copy link
Contributor

laurazard commented May 9, 2022

I'd been meaning to do something like this since the previous issue came up! Nice!

I'd like to test it a bit before it gets merged if that's alright @mattn, but other than that this is awesome :) If that's okay I'll look at it in a couple hours and post back.

@laurazard
Copy link
Contributor

laurazard commented May 9, 2022

Looking at this, CGO is still required in the pac_darwin.go file, correct? It's used for workflows that use a PAC file to process it and find the proxy to use for a given address, so that stuff like the GetProxyFunc() returns a proxy function that just works with those setups. I'm not sure how to do that same functionality strictly in Go without access to a javascript interpreter to interpret the PAC file ourselves, although maybe it is possible to replace the CGo with https://godoc.org/golang.org/x/sys/unix.

Please let me know if that's right 😅. I think the incremental change is good, although it doesn't fix the issue of requiring CGO to compile for darwin. @kazeburo ?

@chrislusf
Copy link

Is this PR stuck?

@mattn
Copy link
Owner

mattn commented Aug 24, 2022

@chrislusf This PR does not fix completely issue that go-ieproxy on macOS uses cgo yet.

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.

8 participants