Open abitrolly opened 11 months ago
@meatballhat Can you review as this is more in your area ?
@abitrolly sorry for my big delay! I had thought that using the built-in Go tooling for this generation hook was preferable đ¤ Can you help me understand why you think this approach is better?
@meatballhat go generate
is calling make
, and there seems to be no benefit in going another level of indirection when it is possible to just call make.
I would prefer to get rid of the Makefile towards a build that uses only //go:generate
@dolmen what is the point in go generate
if go build
doesn't invoke it automatically? To build the project, you still need some script/Makefile that will call go generate
before go build
.
Resolved conflict and rebased.
It would be nice to change -- generating Go source files --
comment to mention what and why is generated.
The principle with "go generate" is that it is run by the developer and its result is committed in the repo so when you run "go build" on any platform you don't need to run "go generate" again.
The principle with "go generate" is that it is run by the developer and its result is committed in the repo so when you run "go build" on any platform you don't need to run "go generate" again.
I haven't found this principle in go generate
help. go generate
is way to run preprocessor in go
when there is no other executors. We use build.go
and make
as build executors. Chaining a third one here complicates the build system, and makes it harder for people to support it. It took me several days to trace problems with generated flags. Then I decided to send PR that removes extra step to simplify build chain. See the https://github.com/urfave/cli/issues/1801 for diagram.
From Rob Pike himself on the Go blog (emphasis mine):
Go generate does nothing that couldnât be done with Make or some other build mechanism, but it comes with the go toolâno extra installation requiredâand fits nicely into the Go ecosystem. Just keep in mind that it is for package authors, not clients, if only for the reason that the program it invokes might not be available on the target machine. Also, if the containing package is intended for import by go get, once the file is generated (and tested!) it must be checked into the source code repository to be available to clients.
I understand that you want the code generation to be simplified, but I disagree with the choice of switching to make instead of using just the Go toolchain.
I'll propose something else. But I'm concerned with the dual maintenance of v2 and v3.
@dolmen I could agree with the argument if it was go generate
as a replacement for make
, but we are already using make
as a primary tool for v2
, and go generate
doesn't add any value to it - only complexity.
I'm still feeling â 0ď¸âŁ on this one @abitrolly đ¤ˇđź It's true that we have a Makefile, but it's pretty slim, and I want to make sure the project doesn't slip into requiring make
to build a usable library. I tend to agree with @dolmen and I would also prefer to move towards getting rid of make
instead of getting rid of the use of go generate
. </2cents> â¤ď¸
I'm personally against having a makefile in a go project, except if it's really necessary.
@julian7 would you be able to come up with PR to remove Makefile?
@meatballhat it is not about Makefile
vs go generate
. It is about reducing build chain complexity, so that maintainers who still need to run and patch v2
to update their codebase, spend less time than me on debugging how it works (especially when it doesn't). I believe it took me few days to draw this diagram from #1801.
graph LR
make --> build.go --> gogen[go generate] --> cli.go --> urfave-cli-genflags/make --> urfave-cli-genflags --> goimports
What type of PR is this?
What this PR does / why we need it:
Simplifies build process.
Which issue(s) this PR fixes:
Fixes #1801
Testing
Run
make
. Remove generated files. Run again. See they are regenerated and identical.Release Notes
(REQUIRED)