Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(10)

Issue 8797049: code review 8797049: cmd/go: undo CL 8119049 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years ago by rsc
Modified:
11 years ago
Reviewers:
CC:
golang-dev, iant, r
Visibility:
Public.

Description

cmd/go: undo CL 8119049 Manual undo due to later changes in doc/go1.1.html; cmd/go/test.bash still passes. Rationale, from CL 8119049 review log: This makes the 'go run' command different from every other command. For example, 'go test' does not mean 'go test *.go'. If we were going to handle the no arguments case in 'go run', I would hope that it would scan the current directory to find a package just like 'go build' or 'go test' would, and then it would require that package to be 'package main', and then it would run that package. This would make it match 'go test' and 'go build' and 'go install' and so on. It would mean that if you are working on a command in a directory that is 'go install'able, then 'go run' will run the binary for you. The current CL does not accomplish that when build constraints or file name constraints are involved. For example, if I am working on a program like: $ ls main.go main_386.s main_arm.s main_amd64.s $ Then 'go run' will fail here because the .s files are ignored. If instead I am working on a program like: $ ls main.go main_386.go main_arm.go main_amd64.go $ then 'go run' will fail because too many files are included. I would like to see this command implemented so that it is compatible with the other go subcommands. Since it is too late to do that for Go 1.1, I would like to see this CL reverted, to preserve the option to do it better later.

Patch Set 1 #

Patch Set 2 : diff -r e357a18564cd https://code.google.com/p/go/ #

Patch Set 3 : diff -r e357a18564cd https://code.google.com/p/go/ #

Patch Set 4 : diff -r cdedb129e020 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -15 lines) Patch
M src/cmd/go/doc.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/go/run.go View 1 2 chunks +2 lines, -14 lines 0 comments Download

Messages

Total messages: 4
rsc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
11 years ago (2013-04-29 19:04:15 UTC) #1
iant
LGTM Please reopen issue 5164.
11 years ago (2013-04-29 19:12:03 UTC) #2
r
LGTM
11 years ago (2013-04-29 19:26:06 UTC) #3
rsc
11 years ago (2013-04-30 21:05:01 UTC) #4
*** Submitted as https://code.google.com/p/go/source/detail?r=dddf80567e0d ***

cmd/go: undo CL 8119049

Manual undo due to later changes in doc/go1.1.html; cmd/go/test.bash still
passes.

Rationale, from CL 8119049 review log:

This makes the 'go run' command different from every other command.
For example, 'go test' does not mean 'go test *.go'.

If we were going to handle the no arguments case in 'go run', I would hope that
it would scan the current directory to find a package just like 'go build' or
'go test' would, and then it would require that package to be 'package main',
and then it would run that package. This would make it match 'go test' and 'go
build' and 'go install' and so on. It would mean that if you are working on a
command in a directory that is 'go install'able, then 'go run' will run the
binary for you. The current CL does not accomplish that when build constraints
or file name constraints are involved.

For example, if I am working on a program like:

$ ls
main.go
main_386.s
main_arm.s
main_amd64.s
$

Then 'go run' will fail here because the .s files are ignored.

If instead I am working on a program like:

$ ls
main.go
main_386.go
main_arm.go
main_amd64.go
$

then 'go run' will fail because too many files are included.

I would like to see this command implemented so that it is compatible with the
other go subcommands. Since it is too late to do that for Go 1.1, I would like
to see this CL reverted, to preserve the option to do it better later.

R=golang-dev, iant, r
CC=golang-dev
https://codereview.appspot.com/8797049
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b