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

Add async into doc features #1349

Merged
merged 6 commits into from
May 12, 2022

Conversation

HaoYang670
Copy link
Contributor

@HaoYang670 HaoYang670 commented Feb 22, 2022

Signed-off-by: remzi [email protected]

Which issue does this PR close?

Closes #1307.
Closes #1617

Rationale for this change

What changes are included in this PR?

Add async to default enabled features, so that the link arrow::async_reader is active.

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Feb 22, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1349 (0814078) into master (89ee9ac) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1349   +/-   ##
=======================================
  Coverage   83.03%   83.03%           
=======================================
  Files         181      181           
  Lines       52936    52936           
=======================================
+ Hits        43955    43958    +3     
+ Misses       8981     8978    -3     
Impacted Files Coverage Δ
arrow/src/array/transform/mod.rs 84.52% <0.00%> (+0.13%) ⬆️
parquet_derive/src/parquet_field.rs 66.21% <0.00%> (+0.22%) ⬆️
arrow/src/datatypes/datatype.rs 66.80% <0.00%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89ee9ac...0814078. Read the comment docs.

@alamb
Copy link
Contributor

alamb commented Feb 28, 2022

THank you @HaoYang670

Changing the default features of the arrow crate may have some non trivial downstream consequences for users of arrow.

What would you think about just updating the metadata for the docs.rs build following https://docs.rs/about/metadata? Perhaps something like:

[package.metadata.docs.rs]

# Whether to pass `--all-features` to Cargo (default: false)
all-features = true

?

@HaoYang670
Copy link
Contributor Author

Thank you for your review! I will update my PR.

@HaoYang670
Copy link
Contributor Author

Neither all_features = true nor features = ["default", "async"] works on my desktop.

@alamb
Copy link
Contributor

alamb commented Mar 1, 2022

Neither all_features = true nor features = ["default", "async"] works on my desktop.

Can you share what you tried? Perhaps the output of git diff ?

@HaoYang670
Copy link
Contributor Author

Neither all_features = true nor features = ["default", "async"] works on my desktop.

Can you share what you tried? Perhaps the output of git diff ?

I add

[package.metadata.docs.rs]
all_features = true

in parquet/Cargo.toml.

@alamb
Copy link
Contributor

alamb commented Mar 2, 2022

🤔 I haven't had a chance to really test, but it looks like docs.rs has some specific build process (that isn't just cargo doc) https://docs.rs/about/builds

@HaoYang670 HaoYang670 changed the title Add async to default features Add async into doc features Mar 5, 2022
@alamb
Copy link
Contributor

alamb commented Mar 6, 2022

Thanks @HaoYang670 -- I am trying to test this change using a local docs.rs build -- https://github.com/rust-lang/docs.rs/blob/master/README.md#build-subcommand

WIll report how it goes.

@alamb
Copy link
Contributor

alamb commented Mar 7, 2022

I wasn't able to get the local docs.rs build to work and then ran out of time ;( Hopefully we'll get this into the next arrow release (it missed the cutoff for 10.0.0 sadly)

@HaoYang670
Copy link
Contributor Author

I wasn't able to get the local docs.rs build to work and then ran out of time ;( Hopefully we'll get this into the next arrow release (it missed the cutoff for 10.0.0 sadly)

Thanks a lot @alamb

@alamb
Copy link
Contributor

alamb commented Mar 30, 2022

FWIW I have tried several times to build docs.rs on my laptop and have not had any luck for some reason.

@alamb
Copy link
Contributor

alamb commented Mar 30, 2022

I am wary of merging this in without testing it as it might cause the docs.rs build to fail 😭

@alamb
Copy link
Contributor

alamb commented Apr 15, 2022

Marking as draft until we sort out testing

@alamb
Copy link
Contributor

alamb commented May 12, 2022

TIL docker system prune --all --force which was the source of one of my issue. However now I am seeing other problems locally testing. So I think it is time to YOLO this issue and give it a try!

@alamb alamb marked this pull request as ready for review May 12, 2022 14:46
@alamb
Copy link
Contributor

alamb commented May 12, 2022

Thanks again @HaoYang670 for giving it a try.

@alamb
Copy link
Contributor

alamb commented May 12, 2022

I'll try and test this using a -dev release on crates

@alamb alamb merged commit 101edc9 into apache:master May 12, 2022
@alamb
Copy link
Contributor

alamb commented May 12, 2022

I published a pre-release version to crates to check the documentation build: https://crates.io/crates/parquet/14.0.0-dev

@alamb
Copy link
Contributor

alamb commented May 12, 2022

🤔 I can't figure out the documentation build for dev

@alamb
Copy link
Contributor

alamb commented May 12, 2022

I published old versions here:

will watch doc builds

https://crates.io/crates/arrow/0.18.0
https://crates.io/crates/parquet/0.18.0

@alamb
Copy link
Contributor

alamb commented May 12, 2022

Glad I tested: https://docs.rs/crate/parquet/0.18.0 seems to have failed -- https://docs.rs/crate/parquet/0.18.0/builds/557342

[INFO] [stderr] WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
[INFO] [stdout] c7db694a1e0ec0a7df60f4774a81f3d15861a69233e9177971982b83d9da03dd
[INFO] running `Command { std: "docker" "start" "-a" "c7db694a1e0ec0a7df60f4774a81f3d15861a69233e9177971982b83d9da03dd", kill_on_drop: false }`
[INFO] [stderr]  Documenting parquet v0.18.0 (/opt/rustwide/workdir)
[INFO] [stderr] error[E0658]: `#[doc(cfg)]` is experimental
[INFO] [stderr]    --> src/arrow/mod.rs:124:21
[INFO] [stderr]     |
[INFO] [stderr] 124 | #[cfg_attr(doc_cfg, doc(cfg(feature = "async")))]
[INFO] [stderr]     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[INFO] [stderr]     |
[INFO] [stderr]     = note: see issue #43781 <https://github.com/rust-lang/rust/issues/43781> for more information
[INFO] [stderr]     = help: add `#![feature(doc_cfg)]` to the crate attributes to enable
[INFO] [stderr] 
[INFO] [stderr] error: Compilation failed, aborting rustdoc
[INFO] [stderr] 
[INFO] [stderr] For more information about this error, try `rustc --explain E0658`.
[INFO] [stderr] error: could not document `parquet`
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   process didn't exit successfully: `rustdoc --edition=2021 --crate-type lib --crate-name parquet src/lib.rs --target x86_64-unknown-linux-gnu -o /opt/rustwide/target/x86_64-unknown-linux-gnu/doc --cfg 'feature="arrow"' --cfg 'feature="async"' --cfg 'feature="base64"' --cfg 'feature="brotli"' --cfg 'feature="clap"' --cfg 'feature="cli"' --cfg 'feature="default"' --cfg 'feature="experimental"' --cfg 'feature="flate2"' --cfg 'feature="futures"' --cfg 'feature="lz4"' --cfg 'feature="serde_json"' --cfg 'feature="snap"' --cfg 'feature="test_common"' --cfg 'feature="tokio"' --cfg 'feature="zstd"' --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat -C metadata=78f9c9e2e62425b8 -L 
...

Looking into that

@alamb
Copy link
Contributor

alamb commented May 12, 2022

Well, I am now trying https://crates.io/crates/parquet/0.18.1 , with this change: alamb@3f76ef3

@alamb alamb mentioned this pull request May 12, 2022
@alamb
Copy link
Contributor

alamb commented May 12, 2022

Filed #1695 to track this issue and will provide updates there

@alamb alamb mentioned this pull request May 12, 2022
@HaoYang670
Copy link
Contributor Author

Great work @alamb !

@HaoYang670 HaoYang670 deleted the issue1307_add_async_to_default branch May 12, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate
Projects
None yet
3 participants