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

docs(ci): use up-to-date protoc with docs.rs #14048

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

wackywendell
Copy link
Contributor

@wackywendell wackywendell commented Jan 8, 2025

Which issue does this PR close?

Closes #13853.

Rationale for this change

This uses the same basic solution as in substrait-rs:

https://github.com/substrait-io/substrait-rs/blob/bbcc9f6d0b084a13706f39a43bbba9d37bf2a959/Cargo.toml#L62-L63

What changes are included in this PR?

Given that datafusion-substrait only uses the protoc compiler with the substrait-rs crate and already has a protoc feature, we should be able to solve this problem by enabling the protoc feature for docs.rs, as done here.

Are these changes tested?

This is difficult to test, as it depends on the docs.rs environment, which is hard to replicate.

I have verified locally that adding the same version of protoc to my path as in docs.rs leads to the problem, and is solved with enabling all features:

datafusion/datafusion/substrait ❯ PATH="$HOME/protobuf/bin:$PATH" cargo doc
…
Error: Custom { kind: Other, error: "protoc failed: substrait/algebra.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.\n" }
datafusion/datafusion/substrait ❯ PATH="$HOME/protobuf/bin:$PATH" cargo doc --all-features
…
Documenting datafusion-substrait v44.0.0
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1m 14s
   Generated /Users/wendell.smith/go/src/github.com/DataDog/datafusion/target/doc/datafusion_substrait/index.html

I have attempted to replicate this in a docs.rs environment following the instructions here and here, but running it on any local crate gives an error like invalid type: null, expected a string at line 1 column 2617950 and fails, so I haven't been able to do so.

Are there any user-facing changes?

No, other than that docs.rs should work again.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @wackywendell ❤️

I tested locally that building the docs with all features seems to work great:

$ cd datafusion/substrait/
$ cargo doc --all-features

Thank you for debugging this and your efforts to fix it ❤️

@alamb alamb merged commit 40f1ddd into apache:main Jan 9, 2025
28 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 9, 2025

Thanks again @wackywendell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datafusion-substrait API docs on docs.rs are broken
3 participants