-
Notifications
You must be signed in to change notification settings - Fork 265
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
chore: (no changes) #3078
chore: (no changes) #3078
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Creating a new package makes sense.
However, I think instead of using a build.rs
script, the ticket asks us to check in the generated files. So I think the acceptance criteria here are:
- No
build.rs
should needprotoc
. All theprotoc
files should be generated manually when the files are changed.- As part of this, we should be able to remove all CI job lines that require protoc. For example:
lance/.github/workflows/rust.yml
Line 70 in 638eaf4
sudo apt install -y protobuf-compiler libssl-dev
- As part of this, we should be able to remove all CI job lines that require protoc. For example:
- There needs to be a CI job to validate that the generated protobuf definitions are in sync with the
.proto
files. One possibility is running the protobuf build script and then usinggit
to see if there are any changes.
|
I think as long as there is a Here is a good inspiration from another project that demonstrates putting generating the protobuf artifacts in a separate script: apache/arrow-rs#3927 |
Is this pull request still in progress? i am still unable to compile due to issues with |
Weird that this was closed. It has zero commits. @imotai do you still plan on working on this? If so, feel free to open a fresh PR. |
Based on the requirements outlined in issue #3073, I believe moving all proto files to a standalone module would provide a better experience for library users.
By the way. I am one of the users.