build: improve go generate generation time by checking git status #607
Conversation
There was a problem hiding this comment.
Looks very nice PR, however there are a few issues atm (more specific in comments):
- we don't handle errors when calling
git status. It may lead to not generating when we need to in build environment. - imo
forcegentag doesn't work right now as is. We still rungo run main.goskipping force regeneration. - currently this approach is a bit tricky in cases where we make a change to the template, generate and then revert the changes to template. Then it is really difficult to revert the changes in the generated files. Maybe we should add some message to the console or change the default behaviour? E.g. instead of
forcegentag we havelazygenetc.? - one of the issues I have had is when I have changed the path of generates files. Then when generating the old files still stay around and I have to manually remove them. Maybe it would be worth it to keep track which files have been generated and remove them automatically somehow?
| dir = filepath.Clean(dir) | ||
| once.Do(func() { | ||
| cmd := exec.Command("git", "status", "--porcelain") | ||
| output, _ := cmd.Output() |
There was a problem hiding this comment.
I'd also handle errors. One thing what can happen is that git doesn't exist on the machine (a la compiled in Docker etc) and in that case we may have empty output etc. In this case the test strings.Contains would not include the input path.
My recommendation is that when the command fails to fallback to always generate files instead.
| gitStatusDiff = string(output) | ||
| }) | ||
|
|
||
| // if status contains bavard or addchain, we return true by default |
There was a problem hiding this comment.
Comment doesn't match the implementation? Or did you mean that if the bavard/addchain import dependency is updated?
| var gitStatusDiff string | ||
| var once sync.Once | ||
|
|
||
| func HasChanges(dir string) bool { |
There was a problem hiding this comment.
Short method documentation would be good:
HasChanged returns true when the templates in directory dir have changed. This method allows to conditionally omit code generation when there are no template updates. The path should be relative to the repository root.
Always returns true when the dependencies are changed.
To force code generation even when no changes then use the build tag `forcegen`
| @@ -19,7 +19,7 @@ jobs: | |||
| run: if [[ -n $(gofmt -l .) ]]; then echo "please run gofmt"; exit 1; fi | |||
| - name: generated files should not be modified | |||
| run: | | |||
| go generate ./... | |||
| go generate -tags=forcegen ./... | |||
There was a problem hiding this comment.
I'm not sure we can use -tags=forcegen that easily. This imo defines the build tag for building the generator, but not how we run the commands to actually run later. At least when I'm running locally go generate -tags=forcegen ./... then it doesn't force the generation.
There was a problem hiding this comment.
Maybe we can get it working though - in the packages where we have directives
//go:generate go run main.go
we define a global constant which is defined when we call go generate -tags=forcegen ./... and then refer to it in the subgenerators?
|
indeed, I rushed that one, will think of something better and test more, closing for now :) |
Description
Not fail proof, but added a
git.HasChanges(..)internal api, which check if templates have changes before regen files.The CI has a special build tag that bypasses this, in case some templates end up having some weird dependency chain, we should detect it.
Speeds up dev process when changing only one part of the code base, doesn't regenerate everything.