Skip to content

Conversation

kmahar
Copy link
Contributor

@kmahar kmahar commented May 10, 2022

Attempts to better simulate the docs.rs build environment in our check-rustdoc test by making the cargo registry directory read-only.
Since the chmod makes the cargo registry unusable for future tasks that need to e.g. download different dependencies than the docs one, this also required changing how we install rustup cargo so that it happens in the project directory and is cleaned up at the end of each task, which is the "right" thing to do on Evergreen anyway rather than writing to the shared home directory.

Here's a patch showing that check-rustdoc now fails if using the old version of rustc_version_runtime that we ran into issues with.

I was concerned this change might lead to our tasks taking a lot longer since we are starting from scratch with rustup and cargo each time now, but the difference between my patch on this PR and the latest commit on main in terms of total runtime across all tasks is 293 hours (mine) vs 283 hours (main) so it doesn't seem to make a huge difference.

@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/bash
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the configure-rust script uses bash-only syntax; I took the opportunity to standardize all of our scripts to use bash (even if they don't call configure-rust.sh) to try to avoid any problems around this in the future

# this is to help us avoid introducing problems like those described here
# https://docs.rs/about/builds#read-only-directories where we or a dependency modify source code during the
# build process.
cargo +nightly build
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these might not all be strictly necessary but I just copied the exact invocations that we run cargo rustdoc with below for completeness

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth putting these into their own little script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't end up doing this since we don't use it anywhere else and the number of scripts we have already feels a little out of hand, but per my comment below I did abstract out the "writing out feature combinations to test" part

export MONGO_ATLAS_TLS11_URI_SRV='${MONGO_ATLAS_TLS11_URI_SRV}'
export MONGO_ATLAS_TLS12_URI='${MONGO_ATLAS_TLS12_URI}'
export MONGO_ATLAS_TLS12_URI_SRV='${MONGO_ATLAS_TLS12_URI_SRV}'
export PROJECT_DIRECTORY='${PROJECT_DIRECTORY}'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is needed because configure-rust.sh uses it. for all of the other tasks we run $PREPARE_SHELL which already does this for us.

@kmahar kmahar marked this pull request as ready for review May 11, 2022 16:52
@kmahar kmahar changed the title RUST-1272 Build docs in read-only directory and install cargo in project directory on Evergreen RUST-1283 Build docs in read-only directory and install cargo in project directory on Evergreen May 12, 2022
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One minor suggestion.

# this is to help us avoid introducing problems like those described here
# https://docs.rs/about/builds#read-only-directories where we or a dependency modify source code during the
# build process.
cargo +nightly build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth putting these into their own little script?

@kmahar
Copy link
Contributor Author

kmahar commented May 18, 2022

@abr-egn @patrickfreed I gave up on figuring out what docs.rs does, but I did add a new script feature-combinations.sh that generates a list of feature combinations we can iterate through, and updated the relevant places (check-rustdoc.sh, check-clippy.sh) to do so. we still have to manually update this when we add new features but at least it is all in one place now.
here's a patch I ran with xtrace on for the relevant tasks where you can see the correct flag combinations are indeed tested: https://spruce.mongodb.com/version/62841c215623430430b68009/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

lmk what you think

@abr-egn
Copy link
Contributor

abr-egn commented May 19, 2022

@abr-egn @patrickfreed I gave up on figuring out what docs.rs does, but I did add a new script feature-combinations.sh that generates a list of feature combinations we can iterate through, and updated the relevant places (check-rustdoc.sh, check-clippy.sh) to do so. we still have to manually update this when we add new features but at least it is all in one place now. here's a patch I ran with xtrace on for the relevant tasks where you can see the correct flag combinations are indeed tested: https://spruce.mongodb.com/version/62841c215623430430b68009/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

lmk what you think

I like this approach! LGTM.

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, the feature combinator script seems like a big improvement. I think it would be nice to use it for our compile tasks too, but it's okay to defer that. For generally reducing the number of scripts we have further, I filed RUST-1330.

@kmahar kmahar merged commit edb65e1 into mongodb:main May 19, 2022
@kmahar kmahar deleted the RUST-1272/read-only-docs-build branch May 19, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants