datafusion-substrait API docs on docs.rs are broken

[alamb]

Describe the bug

While reviewing #13803 from @vbarua I noticed that the substrait API docs on docs.rs are broken

To Reproduce

Go to: https://docs.rs/crate/datafusion-substrait/latest

According to the build log

https://docs.rs/crate/datafusion-substrait/43.0.0/builds/1509810

It appears that the issue is that issue is with the protobuf compiler for some reason 🤔

# rustc version
rustc 1.84.0-nightly (b91a3a056 2024-11-07)# docs.rs version
docsrs 0.6.0 (36c721fb 2024-11-06)# build log
[INFO] running `Command { std: "docker" "create" "-v" "/home/cratesfyi/workspace/builds/datafusion-substrait-43.0.0/target:/opt/rustwide/target:rw,Z" "-v" "/home/cratesfyi/workspace/builds/datafusion-substrait-43.0.0/source:/opt/rustwide/workdir:ro,Z" "-v" "/home/cratesfyi/workspace/cargo-home:/opt/rustwide/cargo-home:ro,Z" "-v" "/home/cratesfyi/workspace/rustup-home:/opt/rustwide/rustup-home:ro,Z" "-e" "SOURCE_DIR=/opt/rustwide/workdir" "-e" "CARGO_TARGET_DIR=/opt/rustwide/target" "-e" "DOCS_RS=1" "-e" "CARGO_HOME=/opt/rustwide/cargo-home" "-e" "RUSTUP_HOME=/opt/rustwide/rustup-home" "-w" "/opt/rustwide/workdir" "-m" "6442450944" "--cpus" "6" "--user" "1001:1001" "--network" "none" "ghcr.io/rust-lang/crates-build-env/linux@sha256:4a844ea9eb2546a2d2c7022eacef16ef2e8229c7fbb2c7d4d55a9ceca922f72d" "/opt/rustwide/cargo-home/bin/cargo" "+nightly" "rustdoc" "--lib" "-Zrustdoc-map" "--config" "build.rustdocflags=[\"--cfg\", \"docsrs\", \"-Z\", \"unstable-options\", \"--emit=invocation-specific\", \"--resource-suffix\", \"-20241107-1.84.0-nightly-b91a3a056\", \"--static-root-path\", \"/-/rustdoc.static/\", \"--cap-lints\", \"warn\", \"--extern-html-root-takes-precedence\"]" "--offline" "-Zunstable-options" "--config=doc.extern-map.registries.crates-io=\"https://docs.rs/{pkg_name}/{version}/x86_64-unknown-linux-gnu\"" "-Zrustdoc-scrape-examples" "-j6" "--target" "x86_64-unknown-linux-gnu", kill_on_drop: false }`
[INFO] [stdout] de980afb472a6e766f3e4cf8abce4491593d423f3235b3103d0ab7cb4458b1ad
[INFO] [stderr] WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
[INFO] running `Command { std: "docker" "start" "-a" "de980afb472a6e766f3e4cf8abce4491593d423f3235b3103d0ab7cb4458b1ad", kill_on_drop: false }`
[INFO] [stderr] warning: target filter specified, but no targets matched; this is a no-op
[INFO] [stderr]    Compiling substrait v0.45.5
[INFO] [stderr] error: failed to run custom build command for `substrait v0.45.5`
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   process didn't exit successfully: `/opt/rustwide/target/debug/build/substrait-c1b9d73c7644dfe1/build-script-build` (exit status: 1)
[INFO] [stderr]   --- stdout
[INFO] [stderr]   cargo:rerun-if-env-changed=FORCE_REBUILD
[INFO] [stderr]   cargo:rerun-if-changed=substrait
[INFO] [stderr]   cargo:rerun-if-changed=substrait/text/simple_extensions_schema.yaml
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/extended_expression.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/algebra.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/function.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/type.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/capabilities.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/plan.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/type_expressions.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/parameterized_types.proto
[INFO] [stderr]   cargo:rerun-if-changed=substrait/proto/substrait/extensions/extensions.proto
[INFO] [stderr] 
[INFO] [stderr]   --- stderr
[INFO] [stderr]   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" }
[INFO] running `Command { std: "docker" "inspect" "de980afb472a6e766f3e4cf8abce4491593d423f3235b3103d0ab7cb4458b1ad", kill_on_drop: false }`
[INFO] running `Command { std: "docker" "rm" "-f" "de980afb472a6e766f3e4cf8abce4491593d423f3235b3103d0ab7cb4458b1ad", kill_on_drop: false }`
[INFO] [stdout] de980afb472a6e766f3e4cf8abce4491593d423f3235b3103d0ab7cb4458b1ad

Expected behavior

I expect them to look like the last successful build: https://docs.rs/crate/datafusion-substrait/41.0.0

Additional context

No response

[alamb]

Looks like it is trying to compile substrait 0.45 but itself seems to build just fine:

https://docs.rs/substrait/0.45.5/substrait/index.html

[westonpace]

A recent change was made to Substrait that used a feature that was only stabilized in protoc versions greater than 3.12 (I'm not actually sure the exact version it was stailized). In version 3.12 it was available in an unstable format but requires a special flag to be passed to protoc. The prost crate has no way of passing that special flag. As a result, the minimum version of protoc is greater than 3.12 and this is the error you see if you are using 3.12.

3.12 is a significant version because it is the version of protobuf-compiler that ships with Ubuntu 22.04 so that may explain why the docs build fails (no idea what it is building on so this is speculation but Ubuntu 22.04 is usually the culprit when I see this error).

Note that DF explicitly states 3.15 protobuf compiler is required as of #11006

[westonpace]

Ah, yes, according to https://github.com/rust-lang/crates-build-env it does appear that docs.rs uses Ubuntu 22.04 to build crate docs.

[alamb]

Thanks @westonpace

One thing I couldn't understand is how substrait the substrait docs themselves are built, seemingly just fine on docs.rs (the same runners): https://docs.rs/substrait/latest/substrait/

It doesn't seem to use any docs.rs speciic stuff:
https://github.com/substrait-io/substrait-rs/blob/bbcc9f6d0b084a13706f39a43bbba9d37bf2a959/Cargo.toml#L62

Maybe we need to use a special flag for build or something:
https://github.com/substrait-io/substrait-rs/blob/bbcc9f6d0b084a13706f39a43bbba9d37bf2a959/Cargo.toml#L53

🤔

[alamb]

Also, randomly, I learned today that @andygrove also is an owner of the substrait crate. Fascinating!

[westonpace]

One thing I couldn't understand is how substrait the substrait docs themselves are built, seemingly just fine on docs.rs (the same runners)

@alamb oh, interesting. I hadn't thought about that question. Looking futher it seems it uses https://docs.rs/protobuf-src/latest/protobuf_src/ to build a vendored copy of protoc.

I'm not sure of the implications but it would be an interesting way to solve the problem.

[westonpace]

Here's the relevant bit: https://github.com/substrait-io/substrait-rs/blob/bbcc9f6d0b084a13706f39a43bbba9d37bf2a959/build.rs#L285

[wackywendell]

Ah! We just ran into this in substrait-io/substrait-validator#355, and like substrait-rs, added a protoc feature for using protobuf-src to get a protoc compiler, and then enabled that feature for docs.rs. See the issue for the breakdown of steps.

substrait-validator hasn't yet been released with that change, so docs.rs still shows an error, but I think we're on the way there.

Would that change make sense here?

[wackywendell]

After looking more closely, it looks like datafusion-substrait has a protoc feature:

datafusion/datafusion/substrait/Cargo.toml

Line 56 in 7af6aa9

protoc = ["substrait/protoc"]

Building locally, it looks like this feature covers everything we need it to for docs. Using protoc@3.12.4 downloaded from here and adding it to my path:

datafusion ❯ 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 ❯ PATH="$HOME/protobuf/bin:$PATH" cargo doc --features protoc
Finished `dev` profile [unoptimized + debuginfo] target(s) in 7m 10s
Generated /Users/wendell.smith/go/src/github.com/DataDog/datafusion/target/doc/datafusion/index.html and 42 other files

So I think probably all that needs doing is adding the directive for docs.rs to use the feature:
https://github.com/substrait-io/substrait-rs/blob/bbcc9f6d0b084a13706f39a43bbba9d37bf2a959/Cargo.toml

That said - I think that should be done in datafusion/substrait/Cargo.toml, and not the workspace Cargo.toml, and I'm not entirely certain, and I'm not sure how to find out. I've tried to run docs.rs locally following the instructions here and here, but I haven't been able to reproduce the experimental_allow_proto3_optional was not set error.

We could just add that directive to datafusion/substrait/Cargo.toml and see if it fixes it in the next version? Any other ideas?

[alamb]

We could just add that directive to datafusion/substrait/Cargo.toml and see if it fixes it in the next version? Any other ideas?

This sounds like a great idea to me -- thank you @wackywendell