Spurious script rebuilds with shared target directory with build-cef #9794
Labels
Comments
larsbergstrom
added
A-content/script
|
Interesting, in another run (I've been switching between a normal and CEF build), this time from normal -> CEF, I got: Maybe some error in path normalization has crept in here? They're building from the same directory, but with different relative paths. |
|
And here' s a run of |
|
Maybe related to rust-lang/cargo#2383 ? |
|
This is probably related to how Cargo stores the "package id" for path dependencies. Right now it incorrectly changes the package id depending on which path dependency is the root package being built. That explains the differences in paths you're seeing here. I've reopened rust-lang/cargo#497 which I believe is the underlying issue here. |
|
Thanks for looking into it, @alexcrichton! This would save a ton of time on our build machines. I'm also open to restructuring our various Cargo.toml files, if that's the "correct" fix (or the one that puts us on a better path to only-one-Cargo-toml). |
|
One way you may be able to get around this is to always have the same "root" package and then use |
|
@larsbergstrom it looks like the first error is legit: Namely, That's not the entire story here, however, as I think there's something more subtle at play here. Haven't yet tracked it down yet. |
|
Aha, found it! I couldn't reproduce because it turns out this was a regression introduced by rust-lang/cargo#2279. This also confirms to me that my suspected fix is indeed the route I would like to take to fix it. Time to clean up the commit and send a PR. Thanks for the cc @larsbergstrom! |
|
Got rid of |
|
@alexcrichton Awesome, thanks a ton for looking into this! It'll really help decrease our CI cycle times. |
Do you mean the part where we only build specific parts of the graph? Because that's an ugly work-around, I don't see why geckolib and Servo should be driven by the same top-level Cargo.toml file. :( |
|
@nox I was assuming Alex meant it was:
|
|
Oh, disregard me then. |
bors
added a commit
to rust-lang/cargo
that referenced
this issue
Mar 4, 2016
All crates being compiled by Cargo are identified by a unique `PackageId` instance. This ID incorporates information such as the name, version, and source from where the crate came from. Package ids are allowed to have path sources to depend on local crates on the filesystem. The package id itself encodes the path of where the crate came from. Historically, however, the "path source" from where these packages are learned had some interesting logic. Specifically one specific source would be able to return many packages within. In other words, a path source would recursively walk crate definitions and the filesystem attempting to find crates. Each crate returned from a source has the same source id, so consequently all packages from one source path would have the same source path id. This in turn leads to confusing an surprising behavior, for example: * When crates are compiled the status message indicates the path of the crate root, not the crate being compiled * When viewed from two different locations (e.g. two different crate roots) the same package would have two different source ids because the id is based on the root location. This hash mismatch has been [papered over](#1697) in the past to try to fix some spurious recompiles, but it unfortunately [leaked back in](#2279). This is clearly indicative of the "hack" being inappropriate so instead these commits fix the root of the problem. --- In short, these commits ensure that the package id for a package defined locally has a path that points precisely at that package. This was a relatively invasive change and had ramifications on a few specific portions which now require a bit of extra code to support. The fundamental change here was to change `PathSource` to be non-recursive by default in terms of what packages it thinks it contains. There are still two recursive use cases, git repositories and path overrides, which are used for backwards compatibility. This meant, however, that the packaging step for crate no longer has knowledge of other crates in a repository to filter out files from. Some specific logic was added to assist in discovering a git repository as well as filtering out sibling packages. Another ramification of this patch, however, is that special care needs to be taken when decoding a lockfile. We now need all path dependencies in the lockfile to point precisely at where the path dependency came from, and this information is not encoded in the lock file. The decoding support was altered to do a simple probe of the filesystem to recursively walk path dependencies to ensure that we can match up packages in a lock file to where they're found on the filesystem. Overall, however, this commit closes #1697 and also addresses servo/servo#9794 where this issue was originally reported.
Merged
bors-servo
pushed a commit
that referenced
this issue
Mar 4, 2016
Update cargo Confirmed locally that it fixes #9794 (thanks, @alexcrichton!). r? @nox @metajack <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9880) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Mar 4, 2016
Update cargo Confirmed locally that it fixes #9794 (thanks, @alexcrichton!). r? @nox @metajack <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9880) <!-- Reviewable:end -->
bors-servo
pushed a commit
that referenced
this issue
Mar 5, 2016
Update cargo Confirmed locally that it fixes #9794 (thanks, @alexcrichton!). r? @nox @metajack <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9880) <!-- Reviewable:end -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As seen in: http://build.servo.org/builders/mac-rel-wpt/builds/1966, despite sharing a target directory, runing
./mach build-cef -rafter a./mach build -r, it still takes 14 minutes and 43 seconds. This time is because the script crate believes that it has been changed and needs to re-run the build. Here's a sample fingerprint output:Does not seem to be the same as #7936.
I'm not really sure what this file is (yet), but am looking into it.
CC: @jdm @Ms2ger @alexcrichton @metajack
The text was updated successfully, but these errors were encountered: