Skip to content

Comments

fix: move proto code generation output to OUT_DIR, out of tree#1703

Open
drahnr wants to merge 10 commits intonextfrom
bernhard-externalize-protobuf-artifacts
Open

fix: move proto code generation output to OUT_DIR, out of tree#1703
drahnr wants to merge 10 commits intonextfrom
bernhard-externalize-protobuf-artifacts

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented Feb 24, 2026

Motivation

Committing generated protobuf code is discouraged and creates review churn. Moving artifacts to OUT_DIR removes ~10k LOC from the repo and avoids frequent diffs when protobuf definitions change. rust-analyzer resolves include! paths, so location is no longer a blocker when it comes to symbol resolution / editor support.

Implementation

We keep the existing integration wiring by continuing to use i.e. generated/mod.rs and include!(concat!(env!("OUT_DIR"), "/generated/…")). This minimizes import-path changes across the codebase and keeps the diff concise.

Side effects

  • Removes the docs.rs-specific BUILD_PROTO=1 special casing.
  • Proto code is no longer tracked in git -> requires a working protobuf compiler installed

Risks

  • Build reproducibility: if protobuf compiler has a breaking change we depend on the host installed compiler

@drahnr drahnr added no changelog This PR does not require an entry in the `CHANGELOG.md` file labels Feb 24, 2026
@drahnr drahnr requested a review from sergerad February 24, 2026 19:08
Copy link
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

I think is better and minimizes the review diffs when proto changes are involved.

It also makes sense to me to drop the generated/mod.rs codegen but as you say that probably makes more sense as a separate PR. This file can be created manually.

This looks correct; but PR is still in draft and description looks semi-done so I'm not approving yet.

@drahnr drahnr force-pushed the bernhard-externalize-protobuf-artifacts branch from 34b453c to d5326f2 Compare February 25, 2026 10:22
@drahnr
Copy link
Contributor Author

drahnr commented Feb 25, 2026

Added CI and Makefile changes, which were the last missing pieces.

@drahnr drahnr marked this pull request as ready for review February 25, 2026 10:25
Copy link
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

I would hold off on merging this until @bobbinth agrees with this in principle

Comment on lines +21 to +22
- uses: ./.github/actions/install-rocksdb
- uses: ./.github/actions/install-protobuf-compiler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that these are now married at the hip, should we have a single install-system-prerequisites?

Copy link
Contributor Author

@drahnr drahnr Feb 25, 2026

Choose a reason for hiding this comment

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

One CTRL-K too much. Fixed

I considered merging them, but wasn't sure if won't get more changes in rocksdb when I migrate the LargeSmtForest backend (!= LargeSmt backend).

@drahnr drahnr requested a review from bobbinth February 25, 2026 10:55
@drahnr drahnr added the blocked Something prevents working on this label Feb 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked Something prevents working on this no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants