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

add build tests for the doc-build #1309

Merged
merged 4 commits into from
Mar 10, 2021
Merged

Conversation

syphar
Copy link
Member

@syphar syphar commented Mar 7, 2021

We don't have any tests for the build-process, here we can add some.

I don't know the build-process in detail, so

  • What would be valid assertions here?
  • should we test add-essential-files separately?
  • what other tests should we have?

My current (not yet validated) assumption is that the container and rustwide .workspace will only set up once per job, and not per test-case.

Also, these tests would definitely fail on macs, and also on some linux-machines (cc @Nemo157 ). What would be the best rust-way of splitting them and only running them on CI? Not sure if #[ignore] and cargo test -- --ignored is too hidden

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

What would be valid assertions here?

See #822 for some suggestions.

should we test add-essential-files separately?

I don't know how useful this is to test. It depends on the toolchain used, so rustdoc can break it at any time even if the tests pass in CI. I would rather have #1302.

My current (not yet validated) assumption is that the container and rustwide .workspace will only set up once per job, and not per test-case.

Yes, it will take much too long otherwise. Even downloading crates-build-env takes a very long time, if there's a way to change that for tests it would be useful.

What would be the best rust-way of splitting them and only running them on CI? Not sure if #[ignore] and cargo test -- --ignored is too hidden

This seems reasonable as a first step.

@syphar syphar requested a review from jyn514 March 7, 2021 20:47
@syphar
Copy link
Member Author

syphar commented Mar 7, 2021

@jyn514 this is ready for another round.

Should we finish this up first?

Or we can add more, coming from #551 I could see:

  • a build with a default target not being linux
  • a build with more custom targets
    (for both it would help me to know crates that fix this description)

@jyn514
Copy link
Member

jyn514 commented Mar 8, 2021

Should we finish this up first?

Landing an MVP sounds good to me. @pietroalbini I'd be interested to know what you think about this - it's closer to #524 than anything else, but it's better than not having tests at all. It does add an extra 10 minutes to CI unfortunately.

@syphar could you try switching the docker image to something smaller for CI? Maybe a base ubuntu:20.04 image? Right now I think DOCS_RS_LOCAL_DOCKER_IMAGE only supports local tags but it shouldn't be to hard to allow setting arbitrary remote images through an env variable.

@syphar
Copy link
Member Author

syphar commented Mar 8, 2021

Should we finish this up first?

Landing an MVP sounds good to me. @pietroalbini I'd be interested to know what you think about this - it's closer to #524 than anything else, but it's better than not having tests at all. It does add an extra 10 minutes to CI unfortunately.

I'm happy to add some more tests, since it works generally.
Also, from my experience having a broken production system is worse than an extra 10 minutes in CI :)

@syphar could you try switching the docker image to something smaller for CI? Maybe a base ubuntu:20.04 image? Right now I think DOCS_RS_LOCAL_DOCKER_IMAGE only supports local tags but it shouldn't be to hard to allow setting arbitrary remote images through an env variable.

I'll give it a try (ignore the notifications until I ping you, I'll have to test with commits on github again)

@syphar syphar force-pushed the build-test branch 9 times, most recently from 834542b to 256603d Compare March 8, 2021 18:32
@syphar
Copy link
Member Author

syphar commented Mar 8, 2021

@jyn514 short summary about the status quo:

  • using plain ubuntu:20.04 didn't work because it doesn't have build-essentials. So I created a separate Dockerfile and used that to create the container to be used.
  • Also I change the test to use the dummy crate for testing.
    both these changes brought the time for the slow tests down from 9 minutes to around 4-5, while adding ~30 seconds for building the small image (caching the docker layers with a separate action makes it actually slower because of the other builds from docker-compose).

Next improvements could be getting rid of the 2 min cargo-sweep install, but that I would look at separately.

@pietroalbini
Copy link
Member

I don't have a chance to look into this more, but there are a couple of suggestions I have here:

  • You should initialize Rustwide with fast_init(true), which will speed up initialization.
  • I'm pretty sure you can setup caching for the small Docker image using GitHub Actions.

@syphar
Copy link
Member Author

syphar commented Mar 9, 2021

I don't have a chance to look into this more, but there are a couple of suggestions I have here:

  • You should initialize Rustwide with fast_init(true), which will speed up initialization.

I'll try that too, thanks @pietroalbini !

  • I'm pretty sure you can setup caching for the small Docker image using GitHub Actions.

I tried that (with this action), and likely due to the other (s3, db) images the build actually became slower than without the caching. (I might have misunderstood what you meant, or don't know enough about docker to find a loophole here).

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

I think we can address anything else in follow-ups :)

The time for this went down from 10 to 6 minutes after the smaller docker image and fast_init(true) - I'm not super happy it takes that long, but it's definitely better than not having tests at all.

@jyn514 jyn514 merged commit 65e946c into rust-lang:master Mar 10, 2021
@syphar syphar deleted the build-test branch March 10, 2021 07:38
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.

3 participants