Fix docs.rs #3580

avantgardnerio · 2022-09-21T20:00:18Z

Which issue does this PR close?

Closes #3538 maybe.

Rationale for this change

Fix docs.rs build.

What changes are included in this PR?

Try to fix docs.rs, but no idea how to test.

Are there any user-facing changes?

Docs should work again.

avantgardnerio · 2022-09-21T20:00:32Z

@andygrove please review and let me know if you know how to test.

avantgardnerio · 2022-09-21T20:00:50Z

datafusion/proto/Cargo.toml

@@ -28,13 +28,17 @@ keywords = ["arrow", "query", "sql"]
 edition = "2021"
 rust-version = "1.62"

+[package.metadata.docs.rs]


docs.rs will set this, according to their docs.

avantgardnerio · 2022-09-21T20:01:17Z

datafusion/proto/Cargo.toml

 [lib]
 name = "datafusion_proto"
 path = "src/lib.rs"

 [features]
 default = []
 json = ["pbjson", "pbjson-build", "serde", "serde_json"]
+docsrs = []


Did I do this correctly? Does this declare a new feature but say it has no affect on dependencies?

Yes, I think so

avantgardnerio · 2022-09-21T20:02:02Z

datafusion/proto/build.rs

        .create(true)
-        .open("src/generated/datafusion_json.rs")
+        .open("src/generated/datafusion.rs")


I was generating to two different files because it seemed like it was smooshing the code together when I switched feature sets. Now I realize I had to call .truncate(true) below.

avantgardnerio · 2022-09-21T20:02:40Z

datafusion/proto/build.rs

@@ -61,8 +61,9 @@ fn build() -> Result<(), String> {
    let json = std::fs::read_to_string(out.join("datafusion.serde.rs")).unwrap();
    let mut file = std::fs::OpenOptions::new()
        .write(true)
+        .truncate(true)


This was key to use just one output file for either feature set and avoid combinatorial explosions.

avantgardnerio · 2022-09-21T20:03:05Z

datafusion/proto/build.rs

+        .create(true)
+        .open("src/generated/datafusion.rs")
+        .unwrap();
+    file.write(proto.as_str().as_ref()).unwrap();


Generate code the same way no matter which feature is selected.

avantgardnerio · 2022-09-21T20:03:31Z

datafusion/proto/src/generated/mod.rs

 #[allow(clippy::all)]
 #[rustfmt::skip]
-#[cfg(not(feature = "json"))]
+#[cfg(not(docsrs))]


When we have a writable source folder, generate into that.

avantgardnerio · 2022-09-21T20:03:54Z

datafusion/proto/src/generated/mod.rs

-#[rustfmt::skip]
-#[cfg(feature = "json")]
-pub mod datafusion_json;
+pub mod datafusion {


When not writable, pull from OUT_DIR.

avantgardnerio · 2022-09-21T20:04:11Z

datafusion/proto/src/lib.rs

@@ -25,10 +25,7 @@ pub mod generated;
 pub mod logical_plan;
 pub mod to_proto;

-#[cfg(not(feature = "json"))]
 pub use generated::datafusion as protobuf;


Now the module name is always the same! :D

avantgardnerio · 2022-09-21T20:05:00Z

Not ready for merge... one minute.

avantgardnerio · 2022-09-21T20:13:21Z

datafusion/proto/build.rs

+    #[cfg(feature = "docsrs")]
+    let path = out.join("datafusion.rs");
+    #[cfg(not(feature = "docsrs"))]
+    let path = "src/generated/datafusion.rs";


Change generated path to src or out_dir as appropriate.

avantgardnerio · 2022-09-21T20:20:42Z

Not ready for merge

Okay, ready.

codecov-commenter · 2022-09-21T21:03:07Z

Codecov Report

Merging #3580 (c7fcac2) into master (0a2b0a7) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3580      +/-   ##
==========================================
+ Coverage   85.92%   86.05%   +0.13%     
==========================================
  Files         301      300       -1     
  Lines       56249    56328      +79     
==========================================
+ Hits        48330    48475     +145     
+ Misses       7919     7853      -66

Impacted Files	Coverage Δ
datafusion/proto/src/lib.rs	`94.35% <ø> (+0.07%)`	⬆️
.../src/physical_plan/file_format/delimited_stream.rs	`91.75% <100.00%> (ø)`
datafusion/core/src/physical_plan/hash_join.rs	`95.03% <100.00%> (ø)`
datafusion/expr/src/logical_plan/plan.rs	`77.10% <0.00%> (-1.16%)`	⬇️
datafusion/core/src/physical_plan/sorts/sort.rs	`93.86% <0.00%> (-0.60%)`	⬇️
...e/src/physical_plan/sorts/sort_preserving_merge.rs	`93.49% <0.00%> (-0.36%)`	⬇️
datafusion/expr/src/binary_rule.rs	`84.31% <0.00%> (-0.29%)`	⬇️
datafusion/core/tests/sql/decimal.rs	`100.00% <0.00%> (ø)`
datafusion/core/src/execution/context.rs	`79.33% <0.00%> (ø)`
datafusion/physical-expr/src/expressions/binary.rs	`97.63% <0.00%> (ø)`
... and 22 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

andygrove

The changes look reasonable to me, but I am not familiar with the json feature or docs.rs mechanics, unfortunately.

tustvold · 2022-09-22T10:23:46Z

According to rust-lang/docs.rs#147 (comment) we can use an environment variable instead of a feature flag, perhaps in combination with a cfg key, which would avoid this leaking into the crates public interface.

FWIW I'm not sure if any IDEs need this source hack, it was fixed for CLion a year ago (intellij-rust/intellij-rust#1908), and so perhaps we might just revisit this decision? I'm not sure if rust-analyser has issues though? Edit: apparently rust-analyser doesn't understand (rust-lang/rust-analyzer#3767) 😭

avantgardnerio · 2022-09-22T15:15:55Z

Thanks @tustvold for the links. I'll investigate further today. It would clearly be best if we didn't have to hack our build to work-around IDE limitations.

It sounds like you have no problem with "go to definition" on protobuf generated code in CLion? I'm definitely using the latest version and it was not working for me. I assume @andygrove is in the same boat.

I'll investigate further today and document how to make it work if I figure it out. Otherwise, how would a config setting (vs feature) sound to you? I don't think an environment variable is sufficient in and of itself as we need to flip this switch at compile time.

tustvold · 2022-09-22T15:37:23Z

To get CLion to work you need to run a full build so that the files exist, and then "Refresh Cargo Projects". It then finds the files. There is also an experimental option "evaluate build scripts" but I haven't had a need for this.

A config flag seems fine to me, ofc better if we didn't need to, but provided it isn't externally visible, nobody needs to know 😅

I am somewhat curious what happens when consuming the crate from crates.io, does the build script generate files in the cargo "checkout"? That feels like it might cause issues

andygrove · 2022-09-22T15:52:37Z

To get CLion to work you need to run a full build so that the files exist, and then "Refresh Cargo Projects". It then finds the files.

I wasn't aware of this action. I just updated to CLion 2022.2.3 and tried this out with the Ballista project but I still cannot "go to definition" for the generated code.

My steps were:

Delete .idea folder
Open project in CLion
Build in CLion
Refresh Cargo Projects

Am I missing a step perhaps?

avantgardnerio · 2022-09-22T16:42:48Z

To get CLion to work

From what I can tell, there are two potential issues:

The fact that code is generated into the target/ folder, which is marked as "excluded" (for good reasons)
the usage of the import! macro is confusing CLion

I haven't done a test to figure out which is the issue, but given the fix listed above, I'm inclined to think it is #1. With Java, the solution is to go find the target/myfolder/generated and mark it as a "generated sources root" overriding the "exclude" on the parent folder. With Cargo, this isn't so easy as everything under the target/ folder all gets random hash looking identifiers that change periodically making it not possible to re-include them continuously.

ursabot · 2022-09-23T12:42:36Z

Benchmark runs are scheduled for baseline = b02753c and contender = 49b9c67. 49b9c67 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

tustvold · 2022-09-23T16:42:55Z

I wasn't aware of this action. I just updated to CLion 2022.2.3 and tried this out with the Ballista project but I still cannot "go to definition" for the generated code.

I just did this and it worked correctly, although I note that the generated code it is finding is in src/generated.

To test this out I created a very basic project with the following code.

main.rs

include!(concat!(env!("OUT_DIR"), "/test.rs"));

fn main() {
    Foo {};
    println!("Hello, world!");
}

build.rs

use std::fs::File;
use std::io::Write;
use std::path::PathBuf;

fn main() {
    let path = PathBuf::from(std::env::var_os("OUT_DIR").unwrap()).join("test.rs");
    let mut file = File::create(path).unwrap();
    writeln!(file, "struct Foo {{}}").unwrap();
}

I then compiled and ran the binary, code completion was not working at this point. Running "Refresh Cargo Projects" also didn't work, nor did "Invalidate Caches and Restart".

Fortunately the fix is relatively straightforward:

Open Help > Find Action (CTRL + SHIFT + A)
Enter "Experimental" and select "Experimental Features"
Scroll down to "org.rust.cargo.evaluate.build.scripts" and enable it
File > Invalidate Caches

On restart everything should be working. I used to have this turned on, I turned it off at some point and left it off because everything continued to work, but I suspect this was the result of some caching shenanigans. Either way I think the above should work, let me know. Either way until rust-analyser fixes support for this, I suspect we will need to continue to work around it, but perhaps framing it as a rust-analyser limitation might encourage some movement towards fixing it.

Edit: I also created intellij-rust/intellij-rust#9402 to potentially get some feedback from the plugin maintainers

avantgardnerio · 2022-09-23T18:27:53Z

@tustvold awesome work, thank you! It's great to have a resolution for why you saw different results than myself and @andygrove . As you note, probably for dev-x reasons we should work around it for now, but at least there's an issue to track so we know when to un-hack this. (I'm linking to it in a comment in my other PR).

Undin · 2022-09-26T09:23:32Z

File > Invalidate Caches

@tustvold users shouldn't invalidate caches to make build script evaluation work. You just need to reload the project model after turning the feature on via Refresh Cargo Projects in Cargo tool window

tustvold · 2022-10-25T02:42:17Z

This doesn't appear to have worked with datafusion 13.0.0, the docs build is still broken, I'm getting a PR up to just check in the code like we do for arrow-rs.

I think this will fix docs.rs but no idea how to test

9f83f97

avantgardnerio commented Sep 21, 2022

View reviewed changes

Generate to correct location

52acd56

avantgardnerio commented Sep 21, 2022

View reviewed changes

Clippy

edaf322

Alphabetize

b99dc0c

andygrove approved these changes Sep 22, 2022

View reviewed changes

Brent Gardner added 2 commits September 22, 2022 10:46

Feature -> cfg

1d07f82

New clippy is more demanding

c7fcac2

github-actions bot added the core Core DataFusion crate label Sep 22, 2022

alamb merged commit 49b9c67 into apache:master Sep 23, 2022

avantgardnerio deleted the bg_fix_docs branch September 23, 2022 14:44

tustvold mentioned this pull request Sep 23, 2022

Stabilise org.rust.cargo.evaluate.build.scripts intellij-rust/intellij-rust#9402

Closed

This was referenced Apr 26, 2024

docs.rs build fails for datafusion-proto 37.0.0 #10163

Closed

Fix docs.rs build for datafusion-proto (hopefully) #10254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix docs.rs #3580

Fix docs.rs #3580

avantgardnerio commented Sep 21, 2022

avantgardnerio commented Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio Sep 21, 2022

andygrove Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio commented Sep 21, 2022

avantgardnerio Sep 21, 2022

avantgardnerio commented Sep 21, 2022

codecov-commenter commented Sep 21, 2022 •

edited

Loading

andygrove left a comment

tustvold commented Sep 22, 2022 •

edited

Loading

avantgardnerio commented Sep 22, 2022

tustvold commented Sep 22, 2022

andygrove commented Sep 22, 2022

avantgardnerio commented Sep 22, 2022 •

edited

Loading

ursabot commented Sep 23, 2022

tustvold commented Sep 23, 2022 •

edited

Loading

avantgardnerio commented Sep 23, 2022

Undin commented Sep 26, 2022

tustvold commented Oct 25, 2022 •

edited

Loading

Fix docs.rs #3580

Fix docs.rs #3580

Conversation

avantgardnerio commented Sep 21, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

avantgardnerio commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avantgardnerio commented Sep 21, 2022

Choose a reason for hiding this comment

avantgardnerio commented Sep 21, 2022

codecov-commenter commented Sep 21, 2022 • edited Loading

Codecov Report

andygrove left a comment

Choose a reason for hiding this comment

tustvold commented Sep 22, 2022 • edited Loading

avantgardnerio commented Sep 22, 2022

tustvold commented Sep 22, 2022

andygrove commented Sep 22, 2022

avantgardnerio commented Sep 22, 2022 • edited Loading

ursabot commented Sep 23, 2022

tustvold commented Sep 23, 2022 • edited Loading

avantgardnerio commented Sep 23, 2022

Undin commented Sep 26, 2022

tustvold commented Oct 25, 2022 • edited Loading

codecov-commenter commented Sep 21, 2022 •

edited

Loading

tustvold commented Sep 22, 2022 •

edited

Loading

avantgardnerio commented Sep 22, 2022 •

edited

Loading

tustvold commented Sep 23, 2022 •

edited

Loading

tustvold commented Oct 25, 2022 •

edited

Loading