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

Add buf generate --clean flag #3124

Merged
merged 6 commits into from
Jul 4, 2024
Merged

Add buf generate --clean flag #3124

merged 6 commits into from
Jul 4, 2024

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Jul 3, 2024

Fixes #2437.

@bufdev bufdev requested a review from doriable July 3, 2024 23:29
Copy link
Contributor

github-actions bot commented Jul 3, 2024

The latest Buf updates on your PR.

NameStatus
build✅ passed
lint✅ passed
format✅ passed
breaking✅ passed

@perezd
Copy link
Member

perezd commented Jul 4, 2024

extremely drive by comment: wdyt about --clean ?

--rm isn't as immediately obvious to me, given I had to go read the bug to get what this was gonna do.

my 0.0000002BTC.

@bufdev
Copy link
Member Author

bufdev commented Jul 4, 2024

I think I've seen --rm in some other tool, but that could be made up. But I don't know - --clean could infer to me "generate a clean version of the code" or something, whereas --rm is basically saying to me "run the command you'd otherwise have to run manually". But open to more opinions here.

@timostamm
Copy link
Member

In my opinion, it would be great to avoid overwriting files that don't change, similar to #3058.

For example:

  • make considers files to be changed if they are deleted and re-created, triggering unnecessary work.
  • Similar for other build tools triggering tasks via file watchers.
  • Many IDEs will close an editor window when the file is deleted, so it's tedious to see how generated code changes with schema changes.

I think it would also be nice to have the flag available in buf.gen.yaml next to out. Then users don't have to specify the flag all the time. And specifying the flag next to the out directory path makes it more obvious what is going to be deleted.

In buf.gen.yaml, clean: true seems like a better fit than rm: true, and I think the flag should have the same name.

Obviously that's much more complex than deleting out before generate, just wanted to throw it out there. Maybe it's an option to just add the buf.gen.yaml field, and possibly follow up with a smarter write, if we don't consider that to be a breaking change.

@doriable
Copy link
Member

doriable commented Jul 4, 2024

Re: clean vs. rm

--clean could infer to me "generate a clean version of the code"

I think this messaging makes sense to me -- in this case, you are "resetting" the state of your output dir and getting a "clean" version of the output directory. I suppose that is not what you mean by "clean version of the code", but I think given the context of the output directory, clean seems pretty clear to me, although that is definitely subjective.

Re: Timo's comment on desired behaviours

I think it would also be nice to have the flag available in buf.gen.yaml next to out.

This seems good to me, and agree this further supports the name "clean". It would also make the association of "clean" with the output directory much clearer. I think this seems very reasonable to do.

In my opinion, it would be great to avoid overwriting files that don't change, similar to #3058.
...
Maybe it's an option to just add the buf.gen.yaml field, and possibly follow up with a smarter write, if we don't consider that to be a breaking change.

I like this idea, and also like it as a follow-up. I also think it's a little adjacent to this PR, it's addressing a slightly different behaviour/concern and I don't think I would consider that a breaking change/in need of a flag, but I think that is a separate UX and we can probably defer discussing that until we choose to pursue it.

@bufdev
Copy link
Member Author

bufdev commented Jul 4, 2024

Overall, I'll update to clean, and we can do a "smart" delete in the future in a very-backwards-compatible way. What this would look like:

  • For zip/tar files: sum the file beforehand, generate the new file, if there is a diff, overwrite, if the filename changed (somehow), delete the old one
  • For directories: sum each file beforehand, generate the new files, for each file that no longer exists, delete, for each file that changed, overwrite

This is a much more involved change, however, and --clean/--rm gets us a win off the bat. I'd like to open a new issue after this PR is merged to track the above work.

@bufdev
Copy link
Member Author

bufdev commented Jul 4, 2024

Of note: I discovered where rm is used, docker run --rm. --rm still makes more sense to me, but fine, can go with --clean.

@bufdev
Copy link
Member Author

bufdev commented Jul 4, 2024

Opened #3126 to track the "smart" version of this.

@bufdev bufdev changed the title Add buf generate --rm flag Add buf generate --clean flag Jul 4, 2024
@bufdev bufdev merged commit 70d8712 into main Jul 4, 2024
12 checks passed
@bufdev bufdev deleted the delete-outs branch July 4, 2024 18:01
doriable added a commit that referenced this pull request Aug 1, 2024
As a quick follow-up to #3124 and in response to
#3124 (comment),
this adds a `clean` config key to `v2` `buf.gen.yaml`, for example:

```yaml
version: v2
clean: true
plugins:
  - local: custom-gen-go
    out: gen/go
    opt: paths=source_relative
    strategy: directory
  - protoc_builtin: java
    out: gen/java
```

When running `buf generate` with the above configs, the outs set to each
plugin
(e.g. `gen/go` and `gen/java`) will be removed before code generation is
run.

If `buf generate --clean` flag is set, then that will always take
precedence, even if
`clean: false` in the configuration. And likewise, if `buf generate
--clean=false`,
and `clean: true` in the configuration, then we would not delete the out
directories.

---------

Co-authored-by: Oliver Sun <73540835+oliversun9@users.noreply.github.com>
Co-authored-by: Oliver Sun <osun@buf.build>
Co-authored-by: bufdev <4228796+bufdev@users.noreply.github.com>
Co-authored-by: bufdev <bufdev-github@buf.build>
emcfarlane pushed a commit that referenced this pull request Aug 2, 2024
As a quick follow-up to #3124 and in response to
#3124 (comment),
this adds a `clean` config key to `v2` `buf.gen.yaml`, for example:

```yaml
version: v2
clean: true
plugins:
  - local: custom-gen-go
    out: gen/go
    opt: paths=source_relative
    strategy: directory
  - protoc_builtin: java
    out: gen/java
```

When running `buf generate` with the above configs, the outs set to each
plugin
(e.g. `gen/go` and `gen/java`) will be removed before code generation is
run.

If `buf generate --clean` flag is set, then that will always take
precedence, even if
`clean: false` in the configuration. And likewise, if `buf generate
--clean=false`,
and `clean: true` in the configuration, then we would not delete the out
directories.

---------

Co-authored-by: Oliver Sun <73540835+oliversun9@users.noreply.github.com>
Co-authored-by: Oliver Sun <osun@buf.build>
Co-authored-by: bufdev <4228796+bufdev@users.noreply.github.com>
Co-authored-by: bufdev <bufdev-github@buf.build>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add opt-in flag for buf generate to delete out dir
4 participants