Do not write to user data directories on build. Fix docs.rs documentation generation #231

AlexTMjugador · 2022-05-02T18:27:33Z

This change addresses the same problem as #185. It turns out that build scripts are only meant to write to OUT_DIR, and in general it is untidy to write things all over the place. docs.rs is specially sensitive to this.

Also, docs.rs does not set the feature flag docsrs by itself, but a Cargo package metadata section can be used to instruct docs.rs to pass that flag to Cargo.

I have not tested the change.

This change addresses the same problem as #185. It turns out that build scripts are only meant to write to OUT_DIR, and in general it is untidy to write things all over the place. docs.rs is specially sensitive to this. Also, docs.rs does not set the feature flag "docsrs" by itself, but a Cargo package metadata section can be used to instruct docs.rs to pass that flag to Cargo.

AlexTMjugador · 2022-05-02T19:25:37Z

Tested by making the usual build artifacts folder a symlink to /dev/null on a Linux machine, so any I/O write attempt to it would fail, and then running cargo build and cargo doc on the latest nightly.

smoelius · 2022-05-02T23:31:44Z

Tested by making the usual build artifacts folder a symlink to /dev/null on a Linux machine, so any I/O write attempt to it would fail, and then running cargo build and cargo doc on the latest nightly.

That is very(!) clever. But if I am understanding correctly, this only tells us that building for doc.rs is likely to succeed. We still have the issue of building for conventional platforms, and this line concerns me:

afl.rs/src/common.rs

Line 19 in 25d37ce

env::var("OUT_DIR").map_or_else(

Won't this line break non-doc.rs builds? In other words, won't OUT_DIR be set when afl.rs's build script is run for platforms other than doc.rs?

I am new to this issue, so please forgive me if my comments are completely off-base.

But assuming that's right, I wonder how hard it would be to incorporate one of the techniques mentioned in this issue: rust-lang/docs.rs#147

AlexTMjugador · 2022-05-03T12:52:26Z

Won't this line break non-doc.rs builds? In other words, won't OUT_DIR be set when afl.rs's build script is run for platforms other than doc.rs?

OUT_DIR will be set for non-doc.rs builds too, but so far I have not noticed any issue with it.

However, to play it safe, I could refactor the code to only pay attention to OUT_DIR if the build is being done on a docs.rs environment. Do you think this would be a welcome change? 😄

smoelius · 2022-05-03T13:48:49Z

OUT_DIR will be set for non-doc.rs builds too, but so far I have not noticed any issue with it.

Well, let me make sure I am not misunderstanding. When I build this, I see the following directories created in my target directory:

target/debug/build/afl-dbc0a948e6d7c62c/out/afl
target/debug/build/afl-dbc0a948e6d7c62c/out/afl-llvm-rt

I am afraid they are being created there instead of in:

$HOME/.local/share/afl.rs/rustc-1.60.0-7737e0b/afl.rs-0.12.2

Am I making a mistake?

However, to play it safe, I could refactor the code to only pay attention to OUT_DIR if the build is being done on a docs.rs environment. Do you think this would be a welcome change? smile

Yes, and thank you for doing this. We should definitely get this working.

Also, I noticed this: https://docs.rs/about/builds#detecting-docsrs

AlexTMjugador · 2022-05-08T14:16:10Z

Am I making a mistake?

No, you're not making any mistakes 😄. That was an intended change on my part, and as I've said I didn't notice anything wrong with it.

However, I didn't look too deep into the assumptions the code makes about the directory layout, so in the commits I've just pushed I brought back the previous behavior for non-docs.rs builds as discussed. I'm fairly sure that for docs.rs builds the changes made by this PR are appropriate.

smoelius · 2022-05-09T09:16:56Z

src/common.rs

+    // The Cargo documentation recommends that build scripts
+    // place their generated files at OUT_DIR too, but we
+    // don't change that for now for normal builds.
+    if cfg!(docsrs) {


I was expecting to see if std::env::var("DOCS_RS").is_ok() here.

Is there a reason to prefer this approach?

This source file is used by both the build script and the application code. By checking for the feature flag we can prevent the DOCS_RS environment variable from interferring with the application code, and make sure that this code path is taken only on the intended environment.

That's a good argument. Let's see what happens.

AlexTMjugador · 2022-05-09T20:09:53Z

It's sad that the documentation build still fails, but now at least now we know it's because the docrs feature is not set.

Maybe we should set rustc-args on the docs.rs Cargo metadata section instead. Anyway, because I do not fancy the idea of doing shotgun debugging that much, I'll try to set up a docs.rs container to test the changes more thoroughly when I have more free time, and then submit a hopefully more definitive PR. Sorry about this 😅

smoelius · 2022-05-09T23:52:15Z

Maybe we should set rustc-args on the docs.rs Cargo metadata section instead.

I think this is worth a shot.

smoelius · 2022-05-09T23:57:20Z

Huh. No change: https://docs.rs/crate/afl/0.12.4/builds/555863

smoelius · 2022-05-10T09:50:58Z

Here is the rustdoc command from the most recent build logs (note the --target option at the end):

"/opt/rustwide/cargo-home/bin/cargo" "+nightly" "rustdoc" "--lib" "-Zrustdoc-map" "-Z" "unstable-options" "--config" "build.rustflags=[\"--cfg\", \"docsrs\"]" "--config" "build.rustdocflags=[\"-Z\", \"unstable-options\", \"--emit=invocation-specific\", \"--resource-suffix\", \"-20220508-1.62.0-nightly-cb1219871\", \"--static-root-path\", \"/\", \"--cap-lints\", \"warn\", \"--disable-per-crate-search\"]" "-Zunstable-options" "--config=doc.extern-map.registries.crates-io=\"https://docs.rs/{pkg_name}/{version}/x86_64-unknown-linux-gnu\"" "-j3" "--target" "x86_64-unknown-linux-gnu"

If I run this command, a target/x86_64-unknown-linux-gnu/debug/build/afl-035f5540df137eb5/out directory is created, but nothing is placed inside it.

But if I remove the --target option, then a target/debug/build/afl-af566d4caf516f58/out directory is created and afl and afl-llvm-rt are created inside there.

Also, I noticed this in the Configuration keys section of the Cargo Book:

[target.$triple]
...
# custom flags to pass to all compiler invocations that target $triple
# this value overrides build.rustflags when both are present
rustflags = ["..", ".."]

This suggests to me that rustdoc is silently passing target.x86_64-unknown-linux-gnu configuration options to Cargo. This would also explain why the package.metadata.docs.rs option is listed under Cross Compiling, as opposed to elsewhere.

AlexTMjugador · 2022-05-11T10:39:04Z

Good catch! The target specification may have something to do with it. I'll think about it.

smoelius · 2022-05-11T11:38:36Z

You raised a good point here.

One thought I had was, maybe we switch to checking the DOCS_RS environment variable, but only during the build script's execution.

smoelius · 2022-06-02T11:00:09Z

@AlexTMjugador I feel like we were close to getting this over the finish line.

Do you think you might still work on this?

AlexTMjugador · 2022-06-03T08:39:13Z

I want to work on this (and other things), but I don't have much free time available right now. I hope to be more available in a week or two.

AlexTMjugador · 2022-06-28T12:06:28Z

This docs.rs generation issue is trickier than I anticipated. I might get back to it at some point, but at the moment I'm not doing it. If anyone else is interested in taking care of it, feel free to let me know and go ahead 😄

AlexTMjugador marked this pull request as draft May 2, 2022 19:09

AlexTMjugador added 2 commits May 2, 2022 21:16

Hide types likely not meant to be seen by users from the docs

2823da3

Fix OUT_DIR environment variable read not working as intended

25d37ce

AlexTMjugador marked this pull request as ready for review May 2, 2022 19:25

AlexTMjugador changed the title ~~Use OUT_DIR as a data directory in build scripts~~ Do not write to user data directories on build. Fix docs.rs documentation generation May 2, 2022

AlexTMjugador added 2 commits May 8, 2022 16:00

Set docsrs feature for docs.rs builds

b63b43e

Only store AFL files in OUT_DIR during docs.rs builds

077ca44

Remove unintended declaration in Cargo.toml docsrs metadata

b35d31c

smoelius reviewed May 9, 2022

View reviewed changes

smoelius merged commit f40d69e into rust-fuzz:master May 9, 2022

This was referenced Aug 30, 2022

Check for DOCS_RS environment variable in build.rs #249

Merged

Build AFL in a temporary directory in OUT_DIR #254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not write to user data directories on build. Fix docs.rs documentation generation #231

Do not write to user data directories on build. Fix docs.rs documentation generation #231

AlexTMjugador commented May 2, 2022 •

edited

Loading

AlexTMjugador commented May 2, 2022

smoelius commented May 2, 2022

AlexTMjugador commented May 3, 2022

smoelius commented May 3, 2022

AlexTMjugador commented May 8, 2022 •

edited

Loading

smoelius May 9, 2022

AlexTMjugador May 9, 2022

smoelius May 9, 2022

AlexTMjugador commented May 9, 2022 •

edited

Loading

smoelius commented May 9, 2022

smoelius commented May 9, 2022

smoelius commented May 10, 2022

AlexTMjugador commented May 11, 2022

smoelius commented May 11, 2022

smoelius commented Jun 2, 2022

AlexTMjugador commented Jun 3, 2022

AlexTMjugador commented Jun 28, 2022

Do not write to user data directories on build. Fix docs.rs documentation generation #231

Do not write to user data directories on build. Fix docs.rs documentation generation #231

Conversation

AlexTMjugador commented May 2, 2022 • edited Loading

AlexTMjugador commented May 2, 2022

smoelius commented May 2, 2022

AlexTMjugador commented May 3, 2022

smoelius commented May 3, 2022

AlexTMjugador commented May 8, 2022 • edited Loading

smoelius May 9, 2022

Choose a reason for hiding this comment

AlexTMjugador May 9, 2022

Choose a reason for hiding this comment

smoelius May 9, 2022

Choose a reason for hiding this comment

AlexTMjugador commented May 9, 2022 • edited Loading

smoelius commented May 9, 2022

smoelius commented May 9, 2022

smoelius commented May 10, 2022

AlexTMjugador commented May 11, 2022

smoelius commented May 11, 2022

smoelius commented Jun 2, 2022

AlexTMjugador commented Jun 3, 2022

AlexTMjugador commented Jun 28, 2022

AlexTMjugador commented May 2, 2022 •

edited

Loading

AlexTMjugador commented May 8, 2022 •

edited

Loading

AlexTMjugador commented May 9, 2022 •

edited

Loading