-
Notifications
You must be signed in to change notification settings - Fork 211
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
[2.0] Add docsrs-builder #311
Conversation
0720d78
to
2365743
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start, thanks for putting this together! I have a handful of nits, but otherwise i'm excited to see what we can build on top of this.
builder/src/package_metadata.rs
Outdated
|
||
/// System dependencies. | ||
/// | ||
/// Docs.rs is running on a Debian jessie. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically not accurate any more. We're running on Ubuntu Bionic now. Might as well update the comment since we're migrating the code to a fresh start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More importantly system dependencies is not being used anywhere. Maybe we can actually try to install them this time since docsrs-builder
will be run in a disposable container. Or better; we can try to install system dependencies, only if docsrs-builder is running in a docs.rs disposable container.
Awesome catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed that comment and added code to install system dependencies if it's running in our build environment.
I'd like to mention a huge issue that is still exists in docsrs-builder. I don't think it is wise to solve entire issue in this PR (it might block us), but I think it's better if we take necessary steps in this PR. Currently docs.rs is building and adding adding default target documentation twice. For example: https://docs.rs/acme-client/0.5.3/acme_client/ and https://docs.rs/acme-client/0.5.3/x86_64-unknown-linux-gnu/acme_client/ is exactly identical and have been built and added into database twice. I caused this issue when I introduced builds for other target triples. Initial design of docs.rs didn't support builds for different targets. I kept ignored until now, but since there are some crazy crates out there (stm32) using more than 10GB space, we really need to fix this issue. Issue is caused by using cargo without a target argument for crates that don't have a default-target in [package.metadata.docs.rs] in their Cargo.toml. Docs.rs is building crate without using a default target for this crates and building again same target with target argument. Currently, docs.rs always uses We can fix this by using current platform as default for crates that don't have default-target definition in Cargo.toml and use something similar to your proposal (#224) for serving docs. For example we can set default target to This will make sure docs.rs avoids compilation of same target twice. What do you think @QuietMisdreavus? |
This is exciting, that we're finally using this configuration option, but i'm also highly concerned that we should ensure that our build container is actually getting scrubbed every time. Since that part of the code isn't part of this PR, i'm not sure that should get in the way of landing it, though. I just would really like someone more security-minded to look over the design of the new system before we start using it. |
The way i would approach this has two parts: one at the doc generation level, where, like you mention, we generate the "test build" under our "default target" if the crate doesn't set one. (We may not even have to modify the target loop, since i assume Cargo would see that the docs already exist, and skip rebuilding them. We could also just detect whether the target we're trying to build is the "default" and skip that loop iteration, to ensure that we don't invoke more commands than necessary.) The other part would be at the web layer, where we route the default URLs to load the "default target"'s docs, like in my old PR, where we default to I think this is roughly what you just described? Let me know if i missed something. |
I am 100% sure that's how docker containers works. All data written in our container isn't persistent and will be lost unless we use volume option and mount some directories to outside of our container (which we are not going to do that). And we will always spawn a new container with k8s jobs and remove old jobs (along with container). For example try to install some package in our development environment and exit from that container, when you try to run a new container with LXC and docker works different in this manner. We are currently using a single LXC container for our build environment but we will spawn a new container for each build with docker.
Yes exactly, I changed that part to use DEFAULT_TARGET (switched all |
dc8ea41
to
79ad8e0
Compare
7d978b2
to
3951faf
Compare
Also added few log messages to make it easy to debug build failures, since we will save its output. I think this PR is truly ready now, sorry it took so many changes but it evolved around some ideas 😄 |
This has been mostly obsoleted by the switch to rustwide and docker-compose. There are lots of good ideas here, like compressing the docs and separating the builder from the web server, but I think those would be better served by incremental changes to the existing code base, not by an entirely new program. Other things like installing dependencies automatically I am not sure we want to do anymore - I know @pietroalbini mentioned that he wants dependencies to be in https://github.com/rust-lang/crates-build-env so that Crater also can build those crates. @onur feel free to let me know if I am missing something. |
This is first commit for docsrs-2.0. It is setting up project structure and
development environment.
Our development environment can easily be installed with docker-compose. docker
and docker-compose is only requirement to set up a development environment
for docsrs. People can use their desired OS, set up and work on exactly
same environment. Since we are dumping LXC and planning to use docker, I believe
using docker for development environment is also good choice. Also our CI
(Azure Pipelines) supports docker-compose. We can use exactly same environment
to develop and execute tests. Our CI will also ensure our development environment
always works.
Project is also using Rust 2018 by default (we may only use Rust 2015 for our
old web module). rustfmt and clippy will also be enforced by CI.
Setting up a development environment
docsrs-builder
This is core part of docsrs. Its job is fairly simple; using cargo to build
a docsrs documentation package. It is built on top of our old
build_doc
function. Instead of generating documentation, it is building a complete
package. Our package format is pretty simple. A zip archive with a
docsrs.json
on the root of the archive. docsrs.json contains build metadata. All build
metadata will be generated during build process. This will allow us to completely
isolate build environment. All cargo related tasks will only be executed during
build and necessary info will be saved in our package metadata. debug directory
can also be cached to avoid recompilation of previously compiled dependencies.
docsrs-builder will only clean target/doc directories before building any crate.
docsrs-builder is designed to work on build environments. We can use
crates-build-env or create a new image for this.
We can also build another wrapper around docsrs-builder and allow people to
build and upload their documentation built in their own environments
(something like cargo-docsrs). That was also one of the most requested feature
but it's not in my priority list.