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: go generate should be more resilient to source changes #36068

Open
rogpeppe opened this issue Dec 10, 2019 · 5 comments
Open

cmd/go: go generate should be more resilient to source changes #36068

rogpeppe opened this issue Dec 10, 2019 · 5 comments
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Dec 10, 2019

go version devel +3c0fbeea7d Tue Nov 5 05:22:07 2019 +0000 linux/amd64

When go generate runs, it's quite likely to add and remove source files.
This can result in errors which are inappropriate.

Here's a testscript command script that demonstrates the issue. It's a pared-down version of some real code:

go generate ./...
cp names.new names
go generate ./...

-- names --
a
b
c
-- names.new --
a
b
d
-- gen.go --
// +build ignore

package main

import (
	"io/ioutil"
	"log"
	"os"
	"path/filepath"
	"strings"
)

func main() {
	os.RemoveAll("generated")
	testData, err := ioutil.ReadFile("names")
	if err != nil {
		log.Fatal(err)
	}
	for _, name := range strings.Fields(string(testData)) {
		if err := os.MkdirAll(filepath.Join("generated", name), 0777); err != nil {
			log.Fatal(err)
		}
		err := ioutil.WriteFile(filepath.Join("generated", name, "f.go"), []byte(`
// Code generated for this test. DO NOT EDIT.

package `+name+`

func F() {}
`), 0666)
		if err != nil {
			log.Fatal(err)
		}
	}
}
-- go.mod --
module m

go 1.14
-- main.go --
package main

//go:generate go run gen.go

func main() {
}

When I run it, I see this:

% testscript goissue.txtar
> go generate ./...
> cp names.new names
> go generate ./...
[stderr]
generate: open $WORK/generated/c/f.go: no such file or directory

[exit status 1]
FAIL: /tmp/testscript089422577/0/script.txt:3: unexpected go command failure
error running tst.txtar in /tmp/testscript089422577/0

That is, the second time that go generate runs, it fails.
It seems that go generate is evaluating all the files when it runs, and not being resilient when they change (in this case a file changed name).

Perhaps it should be silent in cases like this.

There may also be other cases where go generate could be quieter about errors (for example when there's a package name mismatch in files in a package, which can be caused when a package is renamed and go generate is called again to regenerate the new files).

One possible approach might be for go generate to ignore any errors on files which contain the standard "DO NOT EDIT" comment.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Dec 10, 2019

@toothrot toothrot added this to the Backlog milestone Dec 10, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Dec 10, 2019

@rogpeppe, it seems very odd for go generate . to produce source files for a different package (./generated/c). (I would expect that to be pretty common for testadata directories, but testdata directories are ignored for patterns like ./....)

I'm assuming that you ran into this with a more realistic example somewhere. Can you give some more detail about the concrete case?

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 10, 2019

In this particular case, I was writing tests for a code generator. The generator generates code in a package, and the go generate command iterates over a bunch of test data, creating a directory, running the generator in it and generating some test code to test the generated code.

The advantage of doing this is that I can check in the resulting test code, which serves as unit tests for the imported package used by the generated code. It also means that I can easily obtain coverage info for the code covered by the test cases.

There's nothing about go generate that says that it should only generate code in the same directory, so I thought this could work ok, but it turns out a bit awkwardly, hence this issue.

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 11, 2019

The generator generates code in a package, and the go generate command iterates over a bunch of test data, creating a directory, running the generator in it and generating some test code to test the generated code.

So the go generate command is being run for a newly-generated package?

It still seems like the solution here belongs on the caller side: if go generate foo might delete the package foo/generated/c entirely, then the user should not run go generate on foo and foo/generated/c simultaneously.

OTOH, if the package foo/generated/c is just testdata for foo, then it should probably be named foo/testdata/c instead, and then the ./... pattern won't match it at all.

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 11, 2019

It still seems like the solution here belongs on the caller side: if go generate foo might delete the package foo/generated/c entirely, then the user should not run go generate on foo and foo/generated/c simultaneously.

I don't really see how this is so different from running go generate on a package where some of the files in the package are generated and might appear or disappear. This is just doing the generation at a package level rather than a file level.

OTOH, if the package foo/generated/c is just testdata for foo, then it should probably be named foo/testdata/c instead, and then the ./... pattern won't match it at all.

If I did that, I couldn't run the generated tests by running go test ./..., which is one of the main reasons I'm doing this this way.

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
3 participants
You can’t perform that action at this time.