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

cmd/go: build -o dir ./... should build (and discard) non main packages #37378

Open
perillo opened this issue Feb 22, 2020 · 25 comments
Open

cmd/go: build -o dir ./... should build (and discard) non main packages #37378

perillo opened this issue Feb 22, 2020 · 25 comments
Milestone

Comments

@perillo
Copy link
Contributor

@perillo perillo commented Feb 22, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14rc1 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/manlio/.local/bin"
GOCACHE="/home/manlio/.cache/go-build"
GOENV="/home/manlio/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/manlio/.local/lib/go:/home/manlio/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/manlio/sdk/go1.14rc1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/manlio/sdk/go1.14rc1/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build397314077=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.14rc1 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.14rc1
uname -sr: Linux 5.5.4-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.31.
gdb --version: GNU gdb (GDB) 9.1

What did you do?

I have a module, with a main package in the module root directory and another package in a subdirectory.
A .go file in the nested package has a syntax error.

I ran the following commands

go build ./...
go build -o build/ ./...

What did you expect to see?

Both commands to report an error

# github.com/perillo/go-init/internal/data
internal/data/data.go:12:25: syntax error: unexpected { after top level declaration

What did you see instead?

Only the first command reports the syntax error.

@perillo perillo changed the title cmd/go: build -o dir ./... does not build (and discard) packages in subdirectories cmd/go: build -o dir ./... should build (and discard) non main packages Feb 22, 2020
@perillo
Copy link
Contributor Author

@perillo perillo commented Feb 22, 2020

A similar problem is that when there are no main packages, go build -o build/ reports an error

go build: no main packages to build

It is questionable if this is the right thing to do, but I think it is incovenient.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 26, 2020

Loading

@bcmills bcmills added this to the Backlog milestone Feb 26, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Feb 26, 2020

I think at least building the matching packages makes sense, although it would make the use-case in #14295 arbitrarily slower in some cases.

Jay, Michael: any thoughts?

Loading

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 26, 2020

No strong opinion, but I think we should keep the current behavior.

go build -o dir ./... is the simplest way to write executables to a directory. Let's not slow it down.

If the intent is to check whether packages build, there are a few ways to do that, most precisely go build -o /dev/null ./....

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Feb 26, 2020

go build -o /dev/null ./... works. But why?

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 26, 2020

But why?

Because of the other bug you observed: we are failing to detect collisions when building multiple executables with the same binary name.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 26, 2020

Hmm... The obvious workaround is to run go build ./... && go build -o dir ./..., but that will also be suboptimally slow due to #31629.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Feb 26, 2020

I asked why because, on the same module with several main packages and several non main packages, go build -o file ./... reports an error:

go build: cannot write multiple packages to non-directory file

but go build -o /dev/null ./... works fine.

Both file and /dev/null are files, so, unless I'm missing something, why go build behave differently?

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Feb 27, 2020

@jayconrod I have read again the documentation of go build, for go1.14. It says:

When compiling multiple packages or a single non-main package,
build compiles the packages but discards the resulting object,
serving only as a check that the packages can be built.

I'm not sure to understand why to explicitly add or a single non-main package, but the documentation indirectly says that go build ./... can be used as a check that all the packages can be build.

Now, without the -o dir flag, executables are written in the current directory and with the -o dir flag they are written to the specified directory. So I have to disagree when you say is the simplest way to write executables to a directory. Let's not slow it down. They should have the same behavior.

$ go build -o build ./...                                                                                                                               
go build: no main packages to build
$ go build  ./...

To reduce build time, it is always possible to use -o file for each main package.

Loading

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 27, 2020

Both file and /dev/null are files, so, unless I'm missing something, why go build behave differently?

It's a special case. If the -o flag is os.DevNull, go build will build the packages but won't produce output files.

I think I misunderstood the reason for its existence though. I've always used it to test whether packages build without writing them anywhere. However, it's not documented. It looks like it was added as a fix for #16811, where go build -o /dev/null would actually replace /dev/null if it had permission to do so.

So nevermind, let's not rely on -o /dev/null.

(Sorry for the slow response, I typed this a few hours ago, but GitHub comments were down)

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 27, 2020

Thinking about this some more. In most repos that mix main packages and other packages, I would expect the vast majority of packages to be imported by the main ones. So, while it's true that go build -o dir ./... would possibly become slower, I don't think it would be much slower in practice.

And, for the few cases that do matter, users can always select only the main packages explicitly, using something like go build -o dir $(go list -f '{{if eq .Name "main"}}{{.ImportPath}}{{end}}' ./...).

I think it makes sense for the -o flag to be orthogonal to the package pattern. Right now it is not: -o changes the meaning of ./... to select only main packages.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 20, 2021

I just found recently that, if I'm correct, in order to discard the build artifacts a simple go build -o "" ./... is enough.
I notice this after looking at https://golang.org/cl/31657, that simply set *buildO to "" if *buildO == os.DevNull.

The current code is in cmd/go/internal/work/build.go:

	// Special case -o /dev/null by not writing at all.
	if cfg.BuildO == os.DevNull {
		cfg.BuildO = ""
	}

However, if it is possible to simply set BuildO to "", then there is no reason for supporting the special -o /dev/null case.

go build allows the -o flag to be set to the empty string since go1.2.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 20, 2021

See also #36784.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 20, 2021

Sorry, this of course don't discard the generation of the executable file, in the case of a main package.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 21, 2021

I wrote a patch to make go build more consistent when the -o flag is specified and is a directory:
https://gist.github.com/perillo/a1381d24eaf9ce4f98d428f6ee83f0b5

The main change is that when the -o flag is not specified, it is set to the current working directory '.'. This will allow to handle go build ./... and go build -o build/ ./... the same.

The default behavior when the -o is a directory is to:

  1. Write all executable files to that directory
  2. Build and discard non main packages

I'm not sure how buildmode=shared" is supposed to work; I keep getting cannot implicitly include runtime/cgo in a shared library.

There is another subtle change.
The original code do something like:

explicitO := len(cfg.BuildO) > 0
// ...
if cfg.BuildO != "" {
	if is-a-directory {
			if !explicitO {
				base.Fatalf("go build: build output %q already exists and is a directory", cfg.BuildO)
			}

I think the base.Fatalf is never going to be called, so I have removed it.

Thanks.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 22, 2021

Change https://golang.org/cl/312391 mentions this issue: cmd/go/internal/work: make go build more consistent

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 23, 2021

The main change is that when the -o flag is not specified, it is set to the current working directory '.'. This will allow to handle go build ./... and go build -o build/ ./... the same.

Unfortunately, making go build ./... equivalent to go build -o . ./... would break existing users who assume that go build with a package pattern does not build executables. (See #14295 (comment).)

I still agree that it would make sense for go build ./... and go build -o . ./... to build the same set of packages, though. I think they should vary only in whether and where they write the output.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 23, 2021

I modified the CL to keep the existing behavior when the -o flag is not specified, but still build and discard all the objects of non main packages.

This change still causes a test to fail:

--- FAIL: TestScript (0.00s)
    --- FAIL: TestScript/build_multi_main (0.60s)
        script_test.go:252: 
            # Verify build -o can output multiple executables to a directory. (0.601s)
            > mkdir $WORK/bin
            > go build -o $WORK/bin ./cmd/c1 ./cmd/c2
            > ! stderr 'multiple packages'
            > ! go build -o $WORK/bin ./pkg1 ./pkg1
            FAIL: testdata/script/build_multi_main.txt:7: unexpected command success

The current implementation reports go build: no main packages to build, but I think it is really wrong and confusing.

I have not updated the documentation, but probably there is no need to explicitly say that when -o is a directory, all non main packages are built and discarded.

Thanks.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 23, 2021

I updated the test:

# Verify that build -o compiles non main packages but discards the resulting objects.
go build -o $WORK/bin ./pkg1 ./pkg1
! exists $WORK/bin/pkg1.a

I don't like how the test is done. The test just need to verify that the $WORK/bin directory is empty. What about an empty command?

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 27, 2021

@bcmills, when you have time can you review https://golang.org/cl/312391?
I'm not sure it will not break existing users.

Thanks.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 27, 2021

@jayconrod had some concerns about building non-main packages, so I would prefer to wait until he is back to review as well, or perhaps @matloob and I can perhaps review once we're in a more stable state for #36460 heading into the freeze.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 28, 2021

Thinking about it, what happens to the non-main packages object files? Will go build cache them, resulting in a possible waste of cache space?

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 28, 2021

Yes, go build will cache built artifacts for the non-main packages if they are built.

(But, again, I suspect that in most repos containing main packages, the non-main packages — or at least the vast majority of them — are already built as transitive dependencies anyway.)

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 28, 2021

As a test, I compared the time required to check if all packages can be built and to write the executables, using the current go build against my CL, using restic.

$ go clean -cache
$ time godevel build -o /dev/null ./... && godevel build -o build/ $(go list -f '{{if eq .Name "main"}}{{.ImportPath}}{{end}}' ./...)
godevel build -o /dev/null $(go list ./...)  35.32s user 3.15s system 341% cpu 11.282 total
$ go clean -cache
$ time godevel build -o build/ ./...
godevel build -o build/ ./...  33.02s user 2.87s system 350% cpu 10.243 total

So, the difference is minimal, but still I think that go build -o build/ ./... should build non-main packages, since the user explicitly requested it. There is also the problem that the shell command is much more complex.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 29, 2021

This seems a better alternative for checking that all packages can be built and writing all executables to a build directory:

go build -o /dev/null ./... && (go build -o build ./... 2>/dev/null || true)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants