replace wget with the implementation of go #25

catatsuy · 2017-07-16T16:28:46Z

refs #1

This commit works correctly. But an error is displayed when downloading LibreSSL.

Unsolicited response received on idle HTTP channel starting with "HTTP/1.0 408 Request Timeout\r\nDate: Sun, 16 Jul 2017 15:04:06 GMT\r\nServer: OpenBSD httpd\r\nConnection: close\r\nCo
ntent-Type: text/html\r\nContent-Length: 439\r\n\r\n<!DOCTYPE html>\n<html>\n<head>\n<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\"/>\n<title>408 Request Timeout</title>\n<style typ
e=\"text/css\"><!--\nbody { background-color: white; color: black; font-family: 'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\nhr { border: 0; border-bottom: 1px dashed; }\n\n--></style>\n<
/head>\n<body>\n<h1>408 Request Timeout</h1>\n<hr>\n<address>OpenBSD httpd</address>\n</body>\n</html>\n"; err=<nil>

cf: golang/go#12855
cf: golang/go#19895
cf: golang/go#2057

Even if an error is displayed, execution will not stop so there is no problem.

The location where the file is saved is the same as when wget is used because we use os.Chdir in the program. If you want to remove os.Chdir, we needs to remove tar.

Why do I want to replace wget with the implementation of go

wget is not preinstalled by default on Mac.

I often use Docker with minimal configuration Linux distribution. In such a distribution, wget may not be installed.

This commit works correctly. But an error is displayed when downloading LibreSSL. cf: golang/go#12855 cf: golang/go#19895 cf: golang/go#2057 Even if an error is displayed, execution will not stop so there is no problem. Because we use `os.Chdir` in the program, the location where the file is saved is the same as when `wget` is used. If you want to remove `os.Chdir`, we needs to remove `tar`.

cubicdaiya · 2017-07-17T11:45:08Z

download.go

-	if command.VerboseEnabled {
-		return command.Run(args)
+func downloadForBuilder(b *builder.Builder) error {
+	res, err := http.Get(b.DownloadURL())


As http.Get() uses http.DefaultClient, timeout is not set. We should set timeout for http client by default. FYI, the default timeout of wget is 900 seconds.

I created a http.Client instance which set timeout every time.

cubicdaiya · 2017-07-17T11:52:25Z

I suggest that you make nginx-build to be able to switch wget or net/http by cli-option such as -use-wget for workaround.

$ nginx-build -d work # use net/http
$ nginx-build -d work --use-wget # use wget

Or it might be good that wget is used only when -verbose is given.

catatsuy · 2017-07-17T15:00:48Z

I do not think that you want to use wget because you want to download the source code only.

The source code of this product is complicated. It is enough to just provide a way to use net/http.

the default timeout of wget

cubicdaiya · 2017-07-20T15:57:04Z

This commit works correctly. But an error is displayed when downloading LibreSSL.

Unsolicited response received on idle HTTP channel starting with "HTTP/1.0 408 Request Timeout\r\nDate: Sun, 16 Jul 2017 15:04:06 GMT\r\nServer: OpenBSD httpd\r\nConnection: close\r\nCo
ntent-Type: text/html\r\nContent-Length: 439\r\n\r\n<!DOCTYPE html>\n<html>\n<head>\n<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\"/>\n<title>408 Request Timeout</title>\n<style typ
e=\"text/css\"><!--\nbody { background-color: white; color: black; font-family: 'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\nhr { border: 0; border-bottom: 1px dashed; }\n\n--></style>\n<
/head>\n<body>\n<h1>408 Request Timeout</h1>\n<hr>\n<address>OpenBSD httpd</address>\n</body>\n</html>\n"; err=<nil>

As this behavior is very confused, I suggest it is referred in README.md.

cubicdaiya · 2017-07-20T16:00:18Z

download.go

-	args := []string{"wget", url}
-	if command.VerboseEnabled {
-		return command.Run(args)
+func downloadForBuilder(b *builder.Builder) error {


I don't think this function name should be renamed. download(b *builder.Builder) is good.

cubicdaiya · 2017-07-20T16:03:00Z

download.go

-		return command.Run(args)
+func downloadForBuilder(b *builder.Builder) error {
+	c := &http.Client{
+		Timeout: time.Duration(900) * time.Second, // the default timeout of wget


Do not use magick number. Use const. And you don't need to refer to wget.

catatsuy · 2017-07-22T07:37:25Z

@cubicdaiya

rename function name and constanize in download.go
add an error in README.md (in the chapter of LibreSSL)
remove wget from the chapter of Requirements

cubicdaiya · 2017-07-22T09:01:34Z

README.md

+
+```
+Unsolicited response received on idle HTTP channel starting with "HTTP/1.0 408 Request Timeout\r\nDate: Sat, 22 Jul 2017 06:55:35 GMT\r\nServer: OpenBSD httpd\r\nConnection: close\r\nContent-Type: text/html\r\nContent-Length: 439\r\n\r\n<!DOCTYPE html>\n<html>\n<head>\n<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\"/>\n<title>408 Request Timeout</title>\n<style type=\"text/css\"><!--\nbody { background-color: white; color: black; font-family: 'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\nhr { border: 0; border-bottom: 1px dashed; }\n\n--></style>\n</head>\n<body>\n<h1>408 Request Timeout</h1>\n<hr>\n<address>OpenBSD httpd</address>\n</body>\n</html>\n"; err=<nil>
+```


As I want to keep README.md simple, remove actual error message and link to this PR.

cubicdaiya · 2017-07-22T09:03:29Z

download.go


 	"github.com/cubicdaiya/nginx-build/builder"
 	"github.com/cubicdaiya/nginx-build/command"
 	"github.com/cubicdaiya/nginx-build/util"
 )

+const DefaultTimeout = time.Duration(900) * time.Second


It is better to specify what timeout. How about DefaultDownloadTimeout?

catatsuy · 2017-07-24T15:09:19Z

change constant name
simplified the README.md

cubicdaiya · 2017-07-28T09:15:19Z

Merged. Thanks.

catatsuy · 2017-07-28T10:04:01Z

thanks!

cubicdaiya reviewed Jul 17, 2017

View reviewed changes

cubicdaiya mentioned this pull request Jul 17, 2017

download: use net/http instead of wget by default. #11

Closed

set timeout

7b794ed

the default timeout of wget

cubicdaiya reviewed Jul 20, 2017

View reviewed changes

catatsuy added 3 commits July 22, 2017 16:00

rename function name and constanize

f86d992

add an error when downloading the source code of LibreSSL on README.md

757e231

remove wget from the chapter of Requirements

814af1a

cubicdaiya reviewed Jul 22, 2017

View reviewed changes

catatsuy added 2 commits July 22, 2017 18:23

change constant name

29bcb4c

simplified the README.md

1c55f2c

cubicdaiya added the LGTM label Jul 28, 2017

cubicdaiya merged commit 82ac3fa into cubicdaiya:master Jul 28, 2017

cubicdaiya mentioned this pull request Jul 28, 2017

Eliminate dependency of wget #1

Closed

catatsuy deleted the feature/reduce_dependence_wget branch July 28, 2017 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace wget with the implementation of go #25

replace wget with the implementation of go #25

catatsuy commented Jul 16, 2017

cubicdaiya Jul 17, 2017 •

edited

Loading

catatsuy Jul 17, 2017

cubicdaiya commented Jul 17, 2017 •

edited

Loading

catatsuy commented Jul 17, 2017

cubicdaiya commented Jul 20, 2017

cubicdaiya Jul 20, 2017

cubicdaiya Jul 20, 2017 •

edited

Loading

catatsuy commented Jul 22, 2017

cubicdaiya Jul 22, 2017 •

edited

Loading

cubicdaiya Jul 22, 2017

catatsuy commented Jul 24, 2017

cubicdaiya commented Jul 28, 2017

catatsuy commented Jul 28, 2017

replace wget with the implementation of go #25

replace wget with the implementation of go #25

Conversation

catatsuy commented Jul 16, 2017

Why do I want to replace wget with the implementation of go

cubicdaiya Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

catatsuy Jul 17, 2017

Choose a reason for hiding this comment

cubicdaiya commented Jul 17, 2017 • edited Loading

catatsuy commented Jul 17, 2017

cubicdaiya commented Jul 20, 2017

cubicdaiya Jul 20, 2017

Choose a reason for hiding this comment

cubicdaiya Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

catatsuy commented Jul 22, 2017

cubicdaiya Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

cubicdaiya Jul 22, 2017

Choose a reason for hiding this comment

catatsuy commented Jul 24, 2017

cubicdaiya commented Jul 28, 2017

catatsuy commented Jul 28, 2017

cubicdaiya Jul 17, 2017 •

edited

Loading

cubicdaiya commented Jul 17, 2017 •

edited

Loading

cubicdaiya Jul 20, 2017 •

edited

Loading

cubicdaiya Jul 22, 2017 •

edited

Loading