rustbuild: Use copies instead of hard links #39518

alexcrichton · 2017-02-04T01:34:50Z

The original motivation for hard links was to speed up the various stages of
rustbuild, but in the end this is causing problems on Windows (#39504).

This commit tweaks the build system to use copies instead of hard links
unconditionally to ensure that the files accessed by Windows are always
disjoint.

Locally this added .3s to a noop build, so it shouldn't be too much of a
regression hopefully!

Closes #39504

alexcrichton · 2017-02-04T01:34:55Z

r? @brson

rust-highfive · 2017-02-04T01:35:01Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

nagisa · 2017-02-04T01:54:14Z

I always said we should be using CoW copies when they are supported :)

nikomatsakis · 2017-02-07T06:00:30Z

@alexcrichton I think we copied this "hard-link-or-copy" pattern in incremental compilation (cc @michaelwoerister, still true?) -- should we revert that code? Also, maybe in the test runner?

michaelwoerister · 2017-02-07T07:18:22Z

We are using hard-links when possible for incremental compilation and I haven't seen any reports of problems with that lately. The usage pattern in incr. comp. is rather simple. CoW would be ideal, but it's not available on most file systems.

alexcrichton · 2017-02-07T07:24:49Z

Oh so to be clear I think this is a relatively obscure bug, having to do with the interaction between Cargo, rustbuild, and the compiler. The compiler's only considering the interaction with itself with incremental compilation, so I'd be confident that the strategy for the compiler is still going to work quite well.

I would have solved this problem very differently if it were just a problem within Cargo itself, but seeing how it spanned across multiple systems this was the best solution I could think of.

brson · 2017-02-09T23:52:17Z

@bors r+

bors · 2017-02-09T23:52:18Z

📌 Commit 32e6c9f has been approved by brson

bors · 2017-02-10T00:41:27Z

⌛ Testing commit 32e6c9f with merge 9658b3b...

bors · 2017-02-10T01:24:33Z

💔 Test failed - status-appveyor

nagisa · 2017-02-11T12:17:49Z

CoW would be ideal, but it's not available on most file systems.

Since copy is happening anyway, it is worthwhile to just try CoW copy first.

alexcrichton · 2017-03-03T20:04:26Z

Ok a new hypothesis for what's going on here.

In one process:
- Cargo is running rustc --test for librand
- The compiler has -L path/do/deps
- The libtest.rlib file is found in the sysroot
- This crate depends on alloc_system (transitively)
- The compiler searches for alloc_system
- The compiler finds two candidates, one in 'deps' one in 'sysroot'
- They have the same SVH, the compiler pikcs the first, the one in 'deps'
- The compiler mmaps the rlib to learn of the metadata
In another process
- Cargo is running normally, learns that alloc_system is fresh
- Upon learning alloc_system is fresh Cargo runs the freshness logic
- This logic will re-link an rlib with hashes to a location of an rlib without hashes
- This involes deleting a previous hard link if it's there

This means that we're deleting a hard link where the compiler in another process has an open file handle to a different path pointing to the hard link. Windows then rejects this.

Some questions I've run into:

https://github.com/rust-lang/cargo/pull/3102/commits first introduced the "link files from hash location to non-hash location" logic. That commit was in 1.15.0. Why are we only seeing this bug now? Turns out we didn't trigger this logic until Lift up workspace rlibs while building cargo#3478. PR Lift up workspace rlibs while building cargo#3478 is only on 1.16.0, not 1.15.0
Why did this PR not fix the issue? The copy-instead-of-hard-link only appplies from the cargo dir to the sysroot. I believe the problematic hard links are within Cargo's dirs itself, not across the rustbuild locations (but those I believe are also susceptible to a similar problem)

So... what now? I'm not sure.

alexcrichton · 2017-03-03T20:05:33Z

Oh right so I should also mention that I can't think of a way to trigger this in a "normal" cargo project. The problem is the circular dependency sorta back through the sysroot. Typically the compiler never finds sysroot crates in the deps/ dir so the only mmap'd files are ones that Cargo's already finished with or ones in the sysroot.

So it's not a heinous bug for everyone, just for rustbuild ...

alexcrichton · 2017-03-03T20:05:55Z

Oh and of course I can't reproduce this locally, as with all other "flaky" failures nowadays.

alexcrichton · 2017-03-03T20:32:08Z

Ok I'm not even sure if that's true. I can't reproduce an error by deleting a file when the compiler has it open. I have no idea what's going on.

brson · 2017-03-04T00:28:48Z

We could try verifying rust-lang/cargo#3478 is the one that causes it. That bors commit is 4b351eadce9330fa2d17219b48bbcdc8cef255e7 and the preceding bors commit, that should pass, is d12cc03cf3b144bc7f13707f40d29edb79dc259d. Here's a patch for the latter, though if we tried to test it against bors and it passed, then we would merge the commit we don't want.

If that's the offending commit then we could revert it in cargo.

Can we adjust the flags so that we're not asking rustc to arbitrarily search the deps folder, by being more specific about --extern and not passing -L?

Since this is believed to be a concurrency bug, can we verify that by running this build serially?

Can we trigger this windows build outside of a PR so we don't accidentally land something bogus? Perhaps add an intentional fail late in the build and disable all the other configurations for a while. It seems like we're going to have to throw experiments at the bots.

brson · 2017-03-04T00:32:48Z

I pushed a patch to my branch that I think tells the linux builders to fail immediately, the idea being to run experiments without wasting CPU or accidentally landing.

brson · 2017-03-04T02:04:05Z

@bors r+

bors · 2017-03-04T02:04:06Z

📌 Commit 4b9ecba has been approved by brson

brson · 2017-03-04T02:04:16Z

@bors r+ p=1

bors · 2017-03-04T02:04:17Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: More through normalization, Feb/Mar 2017 edition #40163

bors · 2017-03-04T02:04:17Z

📌 Commit 4b9ecba has been approved by brson

brson · 2017-03-04T02:04:52Z

@alexcrichton I went ahead and tried a build using the commit you think is immediately before the one that fails, just for the sake of doing something. Feel free to kill it if you have a better use of cpu.

alexcrichton · 2017-03-10T05:16:24Z

I've opened a homu issue for that failure.

bors · 2017-03-10T06:37:08Z

⌛ Testing commit 6f43149 with merge 2485800...

bors · 2017-03-10T07:06:07Z

💔 Test failed - status-travis

arielb1 · 2017-03-10T11:41:59Z

ccache error:

caused by: error reading compile response from server

caused by: IoError(Error { repr: Custom(Custom { kind: Other, error: StringError("unexpected EOF") }) })

Wedged sccached timing out CI builds #39003

@bors retry

bors · 2017-03-10T11:42:13Z

⌛ Testing commit 6f43149 with merge d538fcf...

bors · 2017-03-10T13:06:06Z

💔 Test failed - status-travis

arielb1 · 2017-03-10T13:15:50Z

OSX linker segfaulting on Travis #38878

@bors retry

bors · 2017-03-10T13:22:16Z

⌛ Testing commit 6f43149 with merge f573db4...

rustbuild: Use copies instead of hard links The original motivation for hard links was to speed up the various stages of rustbuild, but in the end this is causing problems on Windows (#39504). This commit tweaks the build system to use copies instead of hard links unconditionally to ensure that the files accessed by Windows are always disjoint. Locally this added .3s to a noop build, so it shouldn't be too much of a regression hopefully! Closes #39504

rustbuild: Use copies instead of hard links The original motivation for hard links was to speed up the various stages of rustbuild, but in the end this is causing problems on Windows (rust-lang#39504). This commit tweaks the build system to use copies instead of hard links unconditionally to ensure that the files accessed by Windows are always disjoint. Locally this added .3s to a noop build, so it shouldn't be too much of a regression hopefully! Closes rust-lang#39504

bors · 2017-03-10T15:34:13Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing f573db4 to master...

This is targeted at removing the need for a workaround in rust-lang/rust#39518, allowing the main rust build system to move back to hard links which should be much more efficient.

Use `same-file` to avoid unnecessary hard links This is targeted at removing the need for a workaround in rust-lang/rust#39518, allowing the main rust build system to move back to hard links which should be much more efficient.

The `copy` function historically in rustbuild used hard links to speed up the copy operations that it does. This logic was backed out, however, in rust-lang#39518 due to a bug that only showed up on Windows, described in rust-lang#39504. The cause described in rust-lang#39504 happened because Cargo, on a fresh build, would overwrite the previous artifacts with new hard links that Cargo itself manages. This behavior in Cargo was fixed in rust-lang/cargo#4390 where it no longer should overwrite files on fresh builds, opportunistically leaving the filesystem intact and not touching it. Hopefully this can help speed up local builds by doing fewer copies all over the place!

rustbuild: Switch back to using hard links The `copy` function historically in rustbuild used hard links to speed up the copy operations that it does. This logic was backed out, however, in #39518 due to a bug that only showed up on Windows, described in #39504. The cause described in #39504 happened because Cargo, on a fresh build, would overwrite the previous artifacts with new hard links that Cargo itself manages. This behavior in Cargo was fixed in rust-lang/cargo#4390 where it no longer should overwrite files on fresh builds, opportunistically leaving the filesystem intact and not touching it. Hopefully this can help speed up local builds by doing fewer copies all over the place!

rust-highfive assigned nikomatsakis Feb 4, 2017

rust-highfive assigned brson and unassigned nikomatsakis Feb 4, 2017

alexcrichton added this to the 1.16.0 milestone Feb 11, 2017

alexcrichton force-pushed the update-cargo branch from 32e6c9f to cf0fe01 Compare February 16, 2017 22:20

alexcrichton force-pushed the update-cargo branch from cf0fe01 to 1ecc019 Compare March 2, 2017 22:22

alexcrichton mentioned this pull request Mar 10, 2017

Travis/AppVeyor may miss a webhook servo/homu#97

Open

This was referenced Mar 10, 2017

fix #40294 obligation cause.body_id is not always a NodeExpr #40404

Merged

syntax: add ast::ItemKind::MacroDef, simplify hygiene info #40220

Merged

alexcrichton mentioned this pull request Mar 10, 2017

Rollup of 36 pull requests #40418

Closed

bors merged commit 6f43149 into rust-lang:master Mar 10, 2017

alexcrichton deleted the update-cargo branch March 10, 2017 19:08

alexcrichton mentioned this pull request Aug 10, 2017

Use same-file to avoid unnecessary hard links rust-lang/cargo#4390

Merged

alexcrichton mentioned this pull request Sep 2, 2017

rustbuild: Switch back to using hard links #44260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustbuild: Use copies instead of hard links #39518

rustbuild: Use copies instead of hard links #39518

alexcrichton commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

rust-highfive commented Feb 4, 2017

nagisa commented Feb 4, 2017

nikomatsakis commented Feb 7, 2017

michaelwoerister commented Feb 7, 2017

alexcrichton commented Feb 7, 2017

brson commented Feb 9, 2017

bors commented Feb 9, 2017

bors commented Feb 10, 2017

bors commented Feb 10, 2017

nagisa commented Feb 11, 2017

alexcrichton commented Mar 3, 2017

alexcrichton commented Mar 3, 2017

alexcrichton commented Mar 3, 2017

alexcrichton commented Mar 3, 2017

brson commented Mar 4, 2017

brson commented Mar 4, 2017

brson commented Mar 4, 2017

bors commented Mar 4, 2017

brson commented Mar 4, 2017

bors commented Mar 4, 2017

bors commented Mar 4, 2017

brson commented Mar 4, 2017

alexcrichton commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

arielb1 commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

arielb1 commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

rustbuild: Use copies instead of hard links #39518

rustbuild: Use copies instead of hard links #39518

Conversation

alexcrichton commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

rust-highfive commented Feb 4, 2017

nagisa commented Feb 4, 2017

nikomatsakis commented Feb 7, 2017

michaelwoerister commented Feb 7, 2017

alexcrichton commented Feb 7, 2017

brson commented Feb 9, 2017

bors commented Feb 9, 2017

bors commented Feb 10, 2017

bors commented Feb 10, 2017

nagisa commented Feb 11, 2017

alexcrichton commented Mar 3, 2017

alexcrichton commented Mar 3, 2017

alexcrichton commented Mar 3, 2017

alexcrichton commented Mar 3, 2017

brson commented Mar 4, 2017

brson commented Mar 4, 2017

brson commented Mar 4, 2017

bors commented Mar 4, 2017

brson commented Mar 4, 2017

bors commented Mar 4, 2017

bors commented Mar 4, 2017

brson commented Mar 4, 2017

alexcrichton commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

arielb1 commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017

arielb1 commented Mar 10, 2017

bors commented Mar 10, 2017

bors commented Mar 10, 2017