code review 8119049: cmd/go: allow run if non-test files are listed

cmd/go: run main package when no files are listed

Fixes 5164.

[titanous, 2013-03-30 23:30:31]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2013-03-31 03:25:29]

It's hard for me to convince myself that it's a good idea to ignore files that
were explicitly listed on the command line.

[titanous, 2013-03-31 13:51:10]

On 2013/03/31 03:25:29, iant wrote:
> It's hard for me to convince myself that it's a good idea to ignore files that
> were explicitly listed on the command line.

The check was originally added to fix this issue:
https://code.google.com/p/go/issues/detail?id=4032

Are you suggesting that the fix for 4032 should be reverted instead?

[iant, 2013-03-31 22:35:38]

On 2013/03/31 13:51:10, titanous wrote:
> On 2013/03/31 03:25:29, iant wrote:
> > It's hard for me to convince myself that it's a good idea to ignore files
that
> > were explicitly listed on the command line.
> 
> The check was originally added to fix this issue:
> https://code.google.com/p/go/issues/detail?id=4032
> 
> Are you suggesting that the fix for 4032 should be reverted instead?

I'm not sure I understand exactly what the fix changed.

If I write "go run a.go b.go" I don't see why it should ignore any of the files
I explicitly listed.

I guess I am not sure that 5164 is actually a bug.

[titanous, 2013-03-31 22:46:44]

On 2013/03/31 22:35:38, iant wrote:
> On 2013/03/31 13:51:10, titanous wrote:
> > On 2013/03/31 03:25:29, iant wrote:
> > > It's hard for me to convince myself that it's a good idea to ignore files
> that
> > > were explicitly listed on the command line.
> > 
> > The check was originally added to fix this issue:
> > https://code.google.com/p/go/issues/detail?id=4032
> > 
> > Are you suggesting that the fix for 4032 should be reverted instead?
> 
> I'm not sure I understand exactly what the fix changed.
> 
> If I write "go run a.go b.go" I don't see why it should ignore any of the
files
> I explicitly listed.
> 
> I guess I am not sure that 5164 is actually a bug.

Before 58e1987657d6, `go run` silently ignored *_test.go files, allowing `go run
*.go` to work when there are *_test.go files in the current directory listed by
the *.go  glob. This also caused 4032, because of the silence ignorance.

The fix for 4032 causes `go run` to error if any *_test.go files are listed,
causing `go run *.go` to fail.

I and others on a golang-nuts thread[1] feel like this is a regression since Go
1.0.3, as `go run *.go` previously could be used as a shortcut for `go build;
./app` and now causes an error.

[1] https://groups.google.com/forum/#!topic/golang-nuts/EOi0VAQNA4c

[iant, 2013-03-31 22:51:02]

On 2013/03/31 22:46:44, titanous wrote:
> On 2013/03/31 22:35:38, iant wrote:
> > On 2013/03/31 13:51:10, titanous wrote:
> > > On 2013/03/31 03:25:29, iant wrote:
> > > > It's hard for me to convince myself that it's a good idea to ignore
files
> > that
> > > > were explicitly listed on the command line.
> > > 
> > > The check was originally added to fix this issue:
> > > https://code.google.com/p/go/issues/detail?id=4032
> > > 
> > > Are you suggesting that the fix for 4032 should be reverted instead?
> > 
> > I'm not sure I understand exactly what the fix changed.
> > 
> > If I write "go run a.go b.go" I don't see why it should ignore any of the
> files
> > I explicitly listed.
> > 
> > I guess I am not sure that 5164 is actually a bug.
> 
> Before 58e1987657d6, `go run` silently ignored *_test.go files, allowing `go
run
> *.go` to work when there are *_test.go files in the current directory listed
by
> the *.go  glob. This also caused 4032, because of the silence ignorance.
> 
> The fix for 4032 causes `go run` to error if any *_test.go files are listed,
> causing `go run *.go` to fail.
> 
> I and others on a golang-nuts thread[1] feel like this is a regression since
Go
> 1.0.3, as `go run *.go` previously could be used as a shortcut for `go build;
> ./app` and now causes an error.
> 
> [1] https://groups.google.com/forum/#%21topic/golang-nuts/EOi0VAQNA4c


I would like to suggest that we make "go run" (with no arguments) pick up all
the non _test*.go files, build them, and run them.

Would that be a sufficient fix for this problem?

[titanous, 2013-03-31 22:51:41]

On 2013/03/31 22:51:02, iant wrote:
> On 2013/03/31 22:46:44, titanous wrote:
> > On 2013/03/31 22:35:38, iant wrote:
> > > On 2013/03/31 13:51:10, titanous wrote:
> > > > On 2013/03/31 03:25:29, iant wrote:
> > > > > It's hard for me to convince myself that it's a good idea to ignore
> files
> > > that
> > > > > were explicitly listed on the command line.
> > > > 
> > > > The check was originally added to fix this issue:
> > > > https://code.google.com/p/go/issues/detail?id=4032
> > > > 
> > > > Are you suggesting that the fix for 4032 should be reverted instead?
> > > 
> > > I'm not sure I understand exactly what the fix changed.
> > > 
> > > If I write "go run a.go b.go" I don't see why it should ignore any of the
> > files
> > > I explicitly listed.
> > > 
> > > I guess I am not sure that 5164 is actually a bug.
> > 
> > Before 58e1987657d6, `go run` silently ignored *_test.go files, allowing `go
> run
> > *.go` to work when there are *_test.go files in the current directory listed
> by
> > the *.go  glob. This also caused 4032, because of the silence ignorance.
> > 
> > The fix for 4032 causes `go run` to error if any *_test.go files are listed,
> > causing `go run *.go` to fail.
> > 
> > I and others on a golang-nuts thread[1] feel like this is a regression since
> Go
> > 1.0.3, as `go run *.go` previously could be used as a shortcut for `go
build;
> > ./app` and now causes an error.
> > 
> > [1] https://groups.google.com/forum/#%2521topic/golang-nuts/EOi0VAQNA4c
> 
> 
> I would like to suggest that we make "go run" (with no arguments) pick up all
> the non _test*.go files, build them, and run them.
> 
> Would that be a sufficient fix for this problem?

Yes, that would solve the problem for me.

[titanous, 2013-03-31 23:14:36]

I've added a naive implementation of the described functionality. Please let me
know if there's a better approach that I don't know about.

[adg, 2013-04-03 22:06:25]

This needs a corresponding documentation change. The code looks ok to me.

[titanous, 2013-04-03 22:42:56]

On 2013/04/03 22:06:25, adg wrote:
> This needs a corresponding documentation change. The code looks ok to me.

Done.

[adg, 2013-04-03 22:54:55]

https://codereview.appspot.com/8119049/diff/17001/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/8119049/diff/17001/doc/go1.1.html#newcode400
doc/go1.1.html:400: an error is now returned if they are listed.
Is the second sentence a change to the status quo? Doesn't seem so to me.

https://codereview.appspot.com/8119049/diff/17001/src/cmd/go/run.go
File src/cmd/go/run.go (right):

https://codereview.appspot.com/8119049/diff/17001/src/cmd/go/run.go#newcode20
src/cmd/go/run.go:20: If no files are listed, all non-test Go source files in
the current working
If no files are named, it compiles and runs all non-test Go source files.

[titanous, 2013-04-03 22:58:14]

On 2013/04/03 22:54:55, adg wrote:
> https://codereview.appspot.com/8119049/diff/17001/doc/go1.1.html
> File doc/go1.1.html (right):
> 
> https://codereview.appspot.com/8119049/diff/17001/doc/go1.1.html#newcode400
> doc/go1.1.html:400: an error is now returned if they are listed.
> Is the second sentence a change to the status quo? Doesn't seem so to me.

Yes, see comment #5.

> 
> https://codereview.appspot.com/8119049/diff/17001/src/cmd/go/run.go
> File src/cmd/go/run.go (right):
> 
> https://codereview.appspot.com/8119049/diff/17001/src/cmd/go/run.go#newcode20
> src/cmd/go/run.go:20: If no files are listed, all non-test Go source files in
> the current working
> If no files are named, it compiles and runs all non-test Go source files.

Done.

[adg, 2013-04-04 00:47:02]

https://codereview.appspot.com/8119049/diff/15002/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/8119049/diff/15002/doc/go1.1.html#newcode399
doc/go1.1.html:399: directory if no file arguments are listed. Because test
files are not run,
Also, the <code>go run</code> command now returns an error if test files are
provided on the command line. In this sense, "<code>go run</code>"  replaces
"<code>go run *.go</code>".

[titanous, 2013-04-04 00:58:48]

On 2013/04/04 00:47:02, adg wrote:
> https://codereview.appspot.com/8119049/diff/15002/doc/go1.1.html
> File doc/go1.1.html (right):
> 
> https://codereview.appspot.com/8119049/diff/15002/doc/go1.1.html#newcode399
> doc/go1.1.html:399: directory if no file arguments are listed. Because test
> files are not run,
> Also, the <code>go run</code> command now returns an error if test files are
> provided on the command line. In this sense, "<code>go run</code>"  replaces
> "<code>go run *.go</code>".

Done.

[adg, 2013-04-04 01:03:48]

LGTM

Thanks!

[adg, 2013-04-04 01:04:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=2fc72d55ed99 ***

cmd/go: run main package when no files are listed

Fixes 5164.

R=golang-dev, iant, adg
CC=golang-dev
https://codereview.appspot.com/8119049

Committer: Andrew Gerrand <adg@golang.org>

[minux1, 2013-04-04 07:14:44]

i'm a little worried by this change.
i think we shouldn't enhance "go run" beyond providing just a way to
run some one-off throwaway scripts.
i think people are already abusing the command.

[dave_cheney.net, 2013-04-04 07:15:12]

I second this concern.

On Thu, Apr 4, 2013 at 6:14 PM, minux <minux.ma@gmail.com> wrote:
> i'm a little worried by this change.
> i think we shouldn't enhance "go run" beyond providing just a way to
> run some one-off throwaway scripts.
> i think people are already abusing the command.
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[0xjnml, 2013-04-04 07:18:03]

On Thu, Apr 4, 2013 at 9:15 AM, Dave Cheney <dave@cheney.net> wrote:
> I second this concern.

Me too.

(I would go as far as removing go run at all. Perhaps in one CL with
disabling relative imports support. Okay, I know I'm only
daydreaming.)

-j

[iant, 2013-04-04 17:41:27]

On Thu, Apr 4, 2013 at 12:14 AM, minux <minux.ma@gmail.com> wrote:
> i'm a little worried by this change.
> i think we shouldn't enhance "go run" beyond providing just a way to
> run some one-off throwaway scripts.
> i think people are already abusing the command.

As long as we are keeping "go run" this seems like the right approach
to me.  It corresponds to "go build".

I'm also OK with removing "go run".

Ian

[r, 2013-04-04 18:13:27]

I'm fine with "go run", and believe that if it's there, it should
behave like "go build", which this CL establishes. It's not really an
enhancement, more a simplification by making it consistent.

-rob

[adg, 2013-04-04 20:55:58]

I doesn't quite behave like "go build", as I don't think it will build c or
assembly files.


On 5 April 2013 05:13, Rob Pike <r@golang.org> wrote:

> I'm fine with "go run", and believe that if it's there, it should
> behave like "go build", which this CL establishes. It's not really an
> enhancement, more a simplification by making it consistent.
>
> -rob
>

[rsc, 2013-04-29 18:35:14]

I would like to revert this CL for Go 1.1.

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, 2013-04-29 18:38:51]

As you can see from the comments, this one took a while to sort out,
and the decision wasn't filled with conviction. Happy to roll it back
for now. If you prepare the CL, I'll approve it. If you'd prefer I
prepare it, just say so.

-rob

[hightower, 2014-03-10 02:13:57]

On 2013/04/29 18:35:14, rsc wrote:
> I would like to revert this CL for Go 1.1.
> 
> 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.

I've submitted a CL: https://codereview.appspot.com/73350043 that works as
outlined above.