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

Bump bootstrap to 1.52 beta #83530

Merged
merged 3 commits into from
Apr 5, 2021
Merged

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Mar 26, 2021

This includes the standard bump, but also a workaround for new cargo behavior around clearing out the doc directory when the rustdoc version changes.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member Author

Hm, that seems unexpected. I can't trivially reproduce locally; trying a full test run now...

@Mark-Simulacrum
Copy link
Member Author

I can reproduce in the docker container locally.

Core does get documented:

Documenting stage2 std (x86_64-unknown-linux-gnu)
 Documenting core v0.0.0 (/checkout/library/core)
    Finished release [optimized] target(s) in 10.78s

But it doesn't look like it exists in the doc/core directory or the stage2-std directory afterwards, which is downright odd. std and alloc both do. It seems like I can reproduce with ./x.py doc --stage 2 library/core locally, though, which seems promising to reproducing the bug. I don't really understand how the beta compiler bump could've affected this though (I guess the next step is to try to reproduce on master).

@Mark-Simulacrum
Copy link
Member Author

@jyn514 would you by chance have a chance to take a look at this? I am not sure that there's a clear explanation for why core isn't getting copied over, and I unfortunately don't have a lot of time to investigate - no worries if you can't though, I can try to find some time.

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.

Off the top of my head nothing looks suspicious. Maybe there was a rustdoc regression in 1.52? Is stage2 normally documented using stage0 or stage 1?

library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member Author

Failures here are likely caused by rust-lang/cargo#8640 which is deleting the target/$host/doc directory during the cargo doc -p core run, causing rustbuild to delete the directory before the alloc run (as it considers it stale, missing the .stamp file as added by rustbuild before running Cargo).

I'm still trying to figure out a solution to this... it may be that we need to document core twice or something like that as an easy stopgap.

@Mark-Simulacrum
Copy link
Member Author

cc @rust-lang/cargo would be good to get a heads up on future deletions/modifications to how we cache state, though I don't know that I'd call this a 'bug' in cargo (we don't really imply stability I guess). May be good to add to the release notes for 1.52 though.

@jyn514
Copy link
Member

jyn514 commented Apr 4, 2021

An alternate solution to rust-lang/cargo#8640 is for cargo to pass --resource-suffix with the toolchain version (or rustdoc could do that automatically if you think it's useful). Then cargo won't have to delete the doc directory.

1.52 Cargo adds rust-lang/cargo#8640 which means that cargo will try to purge
the doc directory caches for us. In theory this may mean that we can jettison
the clear_if_dirty for rustdoc versioning entirely, but for now just workaround
the effects of this change in a less principled but more local way.
@Mark-Simulacrum
Copy link
Member Author

r? @jyn514 since you did a bit of review previously, hopefully CI will pass now

@rust-highfive rust-highfive assigned jyn514 and unassigned kennytm Apr 4, 2021
@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Apr 4, 2021
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.

r=me with CI passing

// output directory will be deleted by us (as cargo will purge the stamp
// file during the first slot's run), and core is relatively fast to
// build so works OK to fill this 'dummy' slot.
let krates = ["core", "core", "alloc", "std", "proc_macro", "test"];
Copy link
Member

@jyn514 jyn514 Apr 4, 2021

Choose a reason for hiding this comment

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

Core is unfortunately pretty chunky until #83278 lands :/ but this seems ok as a workaround. I do think it's worth it for cargo to stop deleting the doc directory though (#83530 (comment)), I'll open an issue if someone from the team doesn't comment soon.

$ x.py doc --stage 1 library/core
Documenting stage1 std (x86_64-unknown-linux-gnu)
Documenting core v0.0.0 (/home/joshua/rustc2/library/core)
Finished release [optimized] target(s) in 8.74s

@Mark-Simulacrum
Copy link
Member Author

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 4, 2021

📌 Commit f06efd2 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2021
@Dylan-DPC-zz
Copy link

@bors p=6

@bors
Copy link
Contributor

bors commented Apr 4, 2021

⌛ Testing commit f06efd2 with merge 35aa636...

@bors
Copy link
Contributor

bors commented Apr 5, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 35aa636 to master...

@ehuss
Copy link
Contributor

ehuss commented Apr 5, 2021

missing the .stamp file as added by rustbuild before running Cargo

Hm, I'm sorry this has caused you trouble. I'm having trouble reproducing the error, can you provide any insight on how to reproduce it? I tried a few things, the most direct was checking out the latest version of master (39eee17), removing the change in f06efd2, and running ./x.py test src/tools/linkchecker. Any ideas on other steps that are needed?

Can you maybe outline what went wrong?

would be good to get a heads up on future deletions/modifications to how we cache state

I can try harder to anticipate what is going to cause problems with rustbuild. I usually try to consider that, and sometimes run test builds when I'm uncertain, but I didn't do that in this case. In cases where I know bootstrap will need to change, I do try to notify you. I'm not sure if it is feasible to alert on every change to the target directory. Almost every release has some changes to the fingerprints or target structure or build behavior. The vast majority shouldn't affect rustbuild.

If you want, I can maybe try to add some semi-automated test to my cargo updates that will run tests with the latest master-cargo? I'd probably want to run them in docker, is there a particular docker image that would be a good representative image? Maybe x86_64-gnu? I'm fine with running one, but I probably don't want to run a bunch of them. I'm not sure if it would help to also include a dist image?

@Mark-Simulacrum Mark-Simulacrum deleted the bootstrap-bump branch April 5, 2021 17:07
@Mark-Simulacrum
Copy link
Member Author

There may be a faster way, but for me rm -rf build/x86_64-unknown-linux-gnu/{doc,stage2-std} && ./x.py test --stage 2 src/tools/linkchecker worked to reproduce the bug. It may need to be run twice (to avoid accidentally succeeding due to lack of need to rebuild deps or something).

The problem, as I understand it, is that rustbuild currently tries to basically do the exact same thing cargo does, but rustbuild (obviously) does it before it runs cargo. Specifically, the sequence of events is:

  • rustbuild runs clear_if_dirty(build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc, stage2/bin/rustdoc)
    • this sees that the directory (first argument) does not exist, so it creates the directory and writes a .stamp file to it
  • cargo runs to build core, sees that build/x86_64-unknown-linux-gnu/stage2-std/.rustdoc_fingerprint.json does not exist, so it deletes x86_64-unknown-linux-gnu/doc, deleting rustbuild's stamp file
  • cargo builds core and exits successfully
  • rustbuild runs clear_if_dirty(build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc, stage2/bin/rustdoc)
    • the directory exists now, and contains core, but there's no stamp file (as cargo deleted it), so rustbuild deletes the directory and creates the stamp file (again)
  • cargo runs, and sees that the rustdoc fingerprint is up to date
    • cargo doesn't delete doc directory as such, builds alloc, test, std, proc_macro docs
  • rustbuild copies the build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc directory to build/x86_64-unknown-linux-gnu/doc, and then runs linkcheck, which sees that core doesn't exist (as rustbuild, helpfully, deleted it)

Hopefully that helps with understanding things!

Yeah, it's no big deal when this happens, realistically it's quite rare that anything actually breaks. Getting some smoke tests running with nightly cargo has been on my 'would love this' list for a while, just not something I've gotten around to personally. I'd personally say that if you wanted to test - and I want to be clear that by no means do I think this is necessary, and is likely too high a burden - then any of the docker test builders would likely be good candidates. I personally usually have a bunch of problems getting those to run nicely (e.g., IPv6 tests in std regularly fail due to problems with my network configuration locally), but if they work OK then that seems reasonable. dist builders are likely not particularly interesting with respect to cargo updates, as they don't do more interesting builds than a test build (and do less of them).

I think the most practical thing may be to just deal with it when I encounter it, and I might be a bit more eager to ping asking if there were any particular changes in the future (as it so happens I think I recall glancing at the cargo PR but I also didn't connect it to breakage).

@ehuss
Copy link
Contributor

ehuss commented Apr 5, 2021

I see, thanks! I'm able to reproduce it. I probably had a pre-existing fingerprint which prevented it from being cleared.

I'll try to think of some way to avoid this problem. My concern is that this could affect other users doing similar unusual things. I'm not sure if the risk is enough to revert it or not.

IPv6 tests in std regularly fail due to problems with my network configuration locally

I regularly run docker images locally. For this particular issue, running enable-docker-ipv6.sh once should suffice, and I think is relatively safe to do. This is documented at https://rustc-dev-guide.rust-lang.org/tests/intro.html#testing-with-docker-images. I'm curious if that maybe still doesn't work?

@Mark-Simulacrum
Copy link
Member Author

Hm, yeah, I'll look into that. It might be a good fix.

bors added a commit to rust-lang/cargo that referenced this pull request Apr 6, 2021
beta: revert #8640 - rustdoc versioning checks

#8640 has been causing some problems with rustbuild (see rust-lang/rust#83914 and rust-lang/rust#83530 (comment)).  In order to give more time to figure out a solution, this reverts the change on beta.  Will need to take some time to figure out what to do on master.

Also cherry picked the following to get tests passing:
* #9316 — Fix semver docs for 1.51.
* #9307 — tests: Tolerate "exit status" in error messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants