Deduplicate libraries on hash instead of filename. #32317
Conversation
|
hfive ain’t workin |
|
Thanks for the PR @taralx! Some of the test changes here are a little worrisome to me, does this break existing tests? If not, could this just add a new standalone test (or append to an existing one?) I'd also want to think a little bit on this as well, I feel like we've intentionally not done this in the past but I can't really see any reason not to do this when I think about it... |
|
Yes, this breaks existing tests that assume that two libraries with different paths but the same name and identical content will not be deduplicated. |
| @@ -21,7 +21,7 @@ all: $(TMPDIR)/libnative.a | |||
| mkdir -p $(TMPDIR)/e1 | |||
| mkdir -p $(TMPDIR)/e2 | |||
| $(RUSTC) e.rs -o $(TMPDIR)/e1/libe.rlib | |||
| $(RUSTC) e.rs -o $(TMPDIR)/e2/libe.rlib | |||
| $(RUSTC) e.rs -o $(TMPDIR)/e2/libe-alt.rlib | |||
There was a problem hiding this comment.
Changed because two libraries with the same name and content but different paths will not conflict anymore.
There was a problem hiding this comment.
Hm I'm a little confused here, don't both these libraries have the same SVH? I don't follow why changing the filename causes them to suddenly be "different"
There was a problem hiding this comment.
same SVH, different "hash" i.e. the part of the filename between lib and .rlib.
|
@taralx could this be changed to not modify existing tests where possible? If we want to add tests for new functionality then new tests should probably be added rather than tweaking old ones |
|
@alexcrichton I've made the absolutely minimal changes to existing tests, as noted above in line notes. The problem is that various tests were making assumptions about our behavior with multiple identical crates, which is not a good way to test things. Where possible, I've changed it so that the crates are either (a) not identical or (b) differently named. |
|
The other alternative, if you prefer it, is to rip out the fragile tests and make new ones that subsume them and more clearly cover the space. |
|
I'd rather keep the tests continue testing the same properties if possible, but can you also add some tests specifically for this PR itself? |
|
So I'll grant that the current situation is kind of confusing, because we have two levels of duplicate checking:
What is the desired ruleset? |
|
Hm there may be a disconnect about "hash" here that we've been talking about, and also perhaps some discrepancies in the implementation. The "hash" in the filename, e.g. Inside the libraries, however, there's a hash called the SVH (strict version hash) which is indeed a hash of the contents of the libraries. We should be deduplicating based on the SVH, not the filename. Does that make sense? |
|
Okay, I think I know what's going on. I think that once upon a time, we used to filter on filename first, then only read metadata if we needed to disambiguate further. Now, we read metadata and ignore filename entirely. I'm going to push this all the way to metadata only with SVH-based dedup, ignoring filenames completely (except for .rlib suffix). |
|
So it turns out grouping by name avoids matching an rlib with a different dylib (e.g. liba.rlib with libarena.dylib when searching for "a"). Still trying to find a solution. |
|
@alexcrichton Ok! Have another look, hopefully the test diffs are more understandable. |
|
I have one more little change to remove the last calls to fs::canonicalize coming as soon as I check tests. |
| @@ -13,11 +13,11 @@ all: | |||
| mv $(call DYLIB,foo) $(TMPDIR)/other | |||
| $(RUSTC) foo.rs --crate-type=dylib | |||
| $(RUSTC) bar.rs -L $(TMPDIR)/other 2>&1 | \ | |||
| grep "multiple dylib candidates" | |||
| grep "assertion failed" && exit 1 || exit 0 | |||
There was a problem hiding this comment.
I think these tests should pass, right? With your changes the SVH is the same so they should get deduplicated so the compilation should succeed?
I'm a little wary to have a test that the compiler doesn't print "assertion failed" as that's a very brittle test to easily break without actually detecting a regression, but if these compiles are successful maybe that part can be tested?
There was a problem hiding this comment.
No, the dylib one doesn't pass -- it prints other strange errors. The issue in question was "this causes the compiler to issue assertion failures". We could extend it to "these should work", but then we'd need to fix the other issue with dylibs.
There was a problem hiding this comment.
What are the "strange errors" that get printed here?
"The compiler doesn't assert" is such a broad test case it's essentially not testing anything unfortunately.
There was a problem hiding this comment.
Ok to take care of those you can just compile all dylibs here with -C prefer-dynamic
|
Looks good to me, thanks! Can you also squash the commits? |
|
h5 has previously said not to do that preemptively. Squash and rebase, or just squash? |
|
Ah yeah that was mainly intended for just review, once it's ready to go we prefer to squash + rebase before landing. |
|
Done. Assuming tests pass, this one's good to go. |
|
@bors: r+ 393b86a8071980f3f89205a44653a35bd7a269b1 Thanks! |
|
⌛ Testing commit 393b86a with merge e6286af... |
|
💔 Test failed - auto-win-msvc-64-opt-rustbuild |
|
That's a weird failure. Does this happen often? |
|
Looks like an ICE: definitely seems related to this patch though? |
|
Maybe I need to --enable-debug |
|
Ah yeah without that I think assertions may be disabled by default? |
|
No repro with --enable-debug. |
|
I think the incantation may be |
|
--enable-debug --enable-debug-assertions no repro :( |
|
☔ The latest upstream changes (presumably #32767) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hm ok, retry with a rebase? |
|
Okay, let's try it. |
|
@bors: r+ 0a1067e031caa96a200cd10c2bcd8da901fbba18 |
|
⌛ Testing commit 0a1067e with merge 4316553... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit 0a1067e with merge d29905a... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
Seems strangely restricted to windows platforms, which build differently in rustbuild. Any idea on how I can repro this for debugging, or what might be causing it? |
|
Is it doing a parallel compile, or did the error show up at a different place this time? |
|
Crates are indeed compiling in parallel, and looks like this is legit? |
|
Yeah, definitely legit, but I can't imagine what's causing it. It may be necessary to add more diagnostics to the build. |
|
Is there a way to run the windows build manually somehow? I don't have a box with the necessary compiler on it myself. Maybe a cloud instance? |
|
Nah unfortunately we don't have a great infrastructure for doing so :( |
|
☔ The latest upstream changes (presumably #32828) made this pull request unmergeable. Please resolve the merge conflicts. |
|
So it turns out that creader was also calling fs::canonicalize during existing_match under the assumption that all paths are canonical. Instead of removing it, I just backed out that change, leaving everything using canonical paths. This bit us on windows because of the \ vs / thing. |
Deduplicate libraries on hash instead of filename. Removes the need for canonicalization to prevent #12459. (Now with passing tests!) Canonicalization breaks certain environments where the libraries are symlinks to files that don't end in .rlib (e.g. /remote/cas/$HASH).
Removes the need for canonicalization to prevent #12459.
(Now with passing tests!)
Canonicalization breaks certain environments where the libraries are symlinks to files that don't end in .rlib (e.g. /remote/cas/$HASH).