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

Switch to rustwide for sandboxing #407

Merged
merged 23 commits into from
Sep 30, 2019
Merged

Switch to rustwide for sandboxing #407

merged 23 commits into from
Sep 30, 2019

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Sep 13, 2019

This PR replaces the whole sandboxing and build system with rustwide (Crater's build system extracted into a library). The practical effects of this are:

  • The build environment will be the same as Crater.
  • Docker is used instead of LXC to sandbox the builds.
  • Nightlies are updated every day automatically.
  • There is less reliance on Cargo as a library.

Because this will change the sandbox some crates will be broken. I'll write a post for blog.rust-lang.org explaining the changes in the next few days.

This is ready for review, but there are things that needs to be done before this can be deployed:

r? @QuietMisdreavus
cc @Mark-Simulacrum @onur

@pietroalbini
Copy link
Member Author

It should be possible to review this commit-by-commit.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Didn't review in super great detail but looks good overall. Left some comments.

src/utils/build_doc_rustwide.rs Outdated Show resolved Hide resolved
src/db/add_package.rs Outdated Show resolved Hide resolved
src/docbuilder/limits.rs Show resolved Hide resolved
src/docbuilder/rustwide_builder.rs Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

I would like to also not have a "number of lines" limit since that seems needless, we have a limit on how many bytes you're emitting which is all we should care about presumably?

@pietroalbini
Copy link
Member Author

I would like to also not have a "number of lines" limit since that seems needless, we have a limit on how many bytes you're emitting which is all we should care about presumably?

Already removed it.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Networking looks good now.

src/docbuilder/limits.rs Outdated Show resolved Hide resolved
@pietroalbini
Copy link
Member Author

Addressed all of Mark's review comments.

@pietroalbini
Copy link
Member Author

Updated the rustwide's build environment with the list of crates installed in the current container and wrote a draft of the blog post announcing the changes.

@pietroalbini
Copy link
Member Author

Also created #409 to properly alert docs.rs users of the changes in this PR.

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

A couple comments from reading over it yesterday. Still reading through everything.

src/db/add_package.rs Outdated Show resolved Hide resolved
];
static DEFAULT_TARGET: &str = "x86_64-unknown-linux-gnu";

static ESSENTIAL_FILES_VERSIONED: &[&str] = &[
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad these are much more easily found within the file, but i still feel like we should get rustdoc to report these itself if we're going to be automatically updating the compiler. We've had issues where docs.rs hadn't seen a new "essential file" and started posting broken docs until we manually uploaded it. (IIRC, there are still docs that don't have the right settings.js file because we went a long time without saving that...)

Copy link
Member

Choose a reason for hiding this comment

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

I continue to believe this is not that necessary for this to land; this is a good issue to file for something we need to do, but we've not added such a file in a relatively long time to rustdoc and we can add a comment to rustdoc saying "go ping docs.rs folks if you add a file here".

Copy link
Member

Choose a reason for hiding this comment

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

But if this is a blocker, then let's lock down nightly for now -- getting rustdoc to print these out seems sort of 'spec' work to me which is always hard :)

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we can get away with landing this without making rustdoc report them, but i would like to have some kind of process so we can quickly update docs.rs if/when new files are added. Making it automatic would be ideal, but in lieu of that, i believe your suggestion of adding a comment to the spot in rustdoc where the static files are emitted should be fine as a stopgap solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't think it's worth blocking on this. Another solution in the meantime is to configure highfive to ping people when that file is changed.

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

This gets a +1 from me. We've talked about the specifics on Discord already, and i feel like setting these limits for crates will begin to enforce good packaging practices (and possibly put some urgency to get performance work done on rustdoc itself!). This should be good to deploy on Monday, per the recent announcement post.

@pietroalbini
Copy link
Member Author

Ok, merging this 🎉

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