Skip to content

datafusion-substrait API docs on docs.rs are broken #13853

Closed
#14048
Closed
datafusion-substrait API docs on docs.rs are broken#13853
#14048
Labels
bugSomething isn't workinghelp wantedExtra attention is needed

Description

@alamb
alamb
opened on Dec 19, 2024

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

Screenshot 2024-12-19 at 5 26 12 PM

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

Activity

  1. alamb
    added
    bugSomething isn't working
    on Dec 19, 2024

  2. alamb commented on Dec 19, 2024

    @alamb
    alamb
    on Dec 19, 2024
    ContributorAuthor

    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

  3. alamb
    added
    help wantedExtra attention is needed
    on Dec 19, 2024

  4. westonpace commented on Dec 20, 2024

    @westonpace
    westonpace
    on Dec 20, 2024
    Member

    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

  5. westonpace commented on Dec 20, 2024

    @westonpace
    westonpace
    on Dec 20, 2024
    Member

    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.

  6. alamb commented on Dec 21, 2024

    @alamb
    alamb
    on Dec 21, 2024
    ContributorAuthor

    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

    🤔

  7. alamb commented on Dec 21, 2024

    @alamb
    alamb
    on Dec 21, 2024
    ContributorAuthor

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

    Screenshot 2024-12-20 at 8 32 47 PM

  8. westonpace commented on Dec 21, 2024

    @westonpace
    westonpace
    on Dec 21, 2024
    Member

    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.

  9. westonpace commented on Dec 21, 2024

    @westonpace
    westonpace
    on Dec 21, 2024
    Member

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

  10. wackywendell commented on Jan 8, 2025

    @wackywendell
    wackywendell
    on Jan 8, 2025
    Contributor

    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?

  11. wackywendell commented on Jan 8, 2025

    @wackywendell
    wackywendell
    on Jan 8, 2025
    Contributor

    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?

  12. alamb commented on Jan 8, 2025

    @alamb
    alamb
    on Jan 8, 2025
    ContributorAuthor

    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

  13. alamb
    closed this as completedin #14048on Jan 9, 2025

  14. wackywendell commented on Jan 10, 2025

    @wackywendell
    wackywendell
    on Jan 10, 2025
    Contributor

    FYI - the similar changes for substrait-validator attached to substrait-io/substrait-validator#355 does seem to have fixed the docs there! Hopefully we'll see the changes here present in the datafusion-substrait docs in the next release.

  15. alamb commented on Jan 10, 2025

    @alamb
    alamb
    on Jan 10, 2025
    ContributorAuthor

    FYI - the similar changes for substrait-validator attached to substrait-io/substrait-validator#355 does seem to have fixed the docs there! Hopefully we'll see the changes here present in the datafusion-substrait docs in the next release.

    🤞 -- Thanks @wackywendell

  16. alamb commented on Feb 7, 2025

    @alamb
    alamb
    on Feb 7, 2025
    ContributorAuthor

    After releasing version 45 the docs are back ❤ 👓 . Thanks again @wackywendell

    https://docs.rs/datafusion-substrait/latest/datafusion_substrait/

    Image

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions