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

fix(ci, core)!: ensure committed and code generated from protobuf spec match #1825

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

SuperFluffy
Copy link
Member

@SuperFluffy SuperFluffy commented Nov 19, 2024

Summary

Deletes and regenerates all code in astria-core/src/generated. Fixes bad imports by nesting generated Astria APIs under a new astria module. Fixes badly named modules.

Background

#1236 noticed that some protobuf files were skipped during code generation because of a tonic_build::Builder::extern_path statement. This showed up again when reworking the protobuf compilation tool to first clear the target directory for storing the generated rust files, where now an include! statement in astria_core::generated failed because the file was not in fact generated. This was not detected because the file was still committed to the repository at an earlier point.

#1707 then added new generated code under a wrongly named optimistic_block module, which should have been named optimistic to be in line with its protobuf package counterpart.

The present change is primarily to the tools/protobuf-compiler binary, which first purges the output directory (minus the handwritten mod.rs) before generating new code.

Changes

  • Update tools/protobuf-compiler to clear astria-core/src/generated prior to repopulating it from proto/
  • Remove the module sequencerblock::optimisticblock
  • Add module sequencerblock::optimistic
  • Update all services that import Astria APIs from astria_core::generated to astria_core::generated::astria.

Testing

This is just code organization. Import paths were updated, code still compiles and tests pass.

Changelogs

Changelogs updated.

Breaking Changelist

  • This is a breaking change for all consumers of astria_core that now need to import many items from astria_core::generated::astria that were directly under astria_core::generated before.

Override Freeze

This code touches services by changing their imports (astria_core::generated -> astria_core::generated::astria) but does change any implementation details.

Related Issues

Fixes and amends #1707.
Supersedes #1824
Closes #1823

@github-actions github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels Nov 19, 2024
@SuperFluffy SuperFluffy force-pushed the superfluffy/delete-old-generated-code branch from 9671baa to eacfdcc Compare November 19, 2024 17:01
@SuperFluffy SuperFluffy changed the title fix(ci, core): delete unused generated code, generate overlooked code fix(ci, core): delete unused generated code, generate skipped code Nov 19, 2024
@SuperFluffy SuperFluffy changed the title fix(ci, core): delete unused generated code, generate skipped code fix(ci, core)!: ensure committed and code generated from protobuf spec match Nov 19, 2024
@SuperFluffy SuperFluffy force-pushed the superfluffy/delete-old-generated-code branch from eacfdcc to 66704f2 Compare November 19, 2024 17:23
@SuperFluffy SuperFluffy marked this pull request as ready for review November 19, 2024 17:24
@SuperFluffy SuperFluffy requested review from a team, joroshiba and noot as code owners November 19, 2024 17:24
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor comments, but non-blocking. LGTM otherwise.

tools/protobuf-compiler/CHANGELOG.md Outdated Show resolved Hide resolved
tools/protobuf-compiler/src/main.rs Outdated Show resolved Hide resolved
tools/protobuf-compiler/src/main.rs Outdated Show resolved Hide resolved
@SuperFluffy SuperFluffy force-pushed the superfluffy/delete-old-generated-code branch from 786c9de to 04d2617 Compare December 5, 2024 10:15
@SuperFluffy SuperFluffy force-pushed the superfluffy/delete-old-generated-code branch from 04d2617 to 6d0092d Compare December 5, 2024 10:27
@SuperFluffy SuperFluffy enabled auto-merge December 5, 2024 10:29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this out of the generated folder since it is a manually edited file? I recognize that this breaks our convention generally, but I think for generated files the convention should be a directory should be wholly generated files.

Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with note that I think we should migrate the generated mod.rs

@SuperFluffy SuperFluffy added this pull request to the merge queue Dec 5, 2024
Merged via the queue into main with commit fc10a63 Dec 5, 2024
46 checks passed
@SuperFluffy SuperFluffy deleted the superfluffy/delete-old-generated-code branch December 5, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composer pertaining to composer conductor pertaining to the astria-conductor crate override-freeze proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proto: remove optimistic block generated proto files
3 participants