Check for duplicate output filenames. #6308
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
@alexcrichton I didn't fully follow your last comment, because it kinda sounded like you were saying two different things. I think generating an error if the current command would generate duplicates is the right thing to do, because almost certainly that is the wrong behavior. These can be converted to warnings if needed, though. I might follow up with a separate PR sometime later to clean up the |
There was a problem hiding this comment.
This looks great to me! I think my last comment was "let's do what this PR does" and then possibly emit warnings instead of errors for the time being. I think I'd personally still be slightly in favor of doing that, emitting a warning.
Do you think it's feasible to tweak the output here to say that it will soon become a hard error (perhaps with an issue URL as well) and leave it as a warning for now?
| @@ -72,18 +72,6 @@ impl<'a> Unit<'a> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a> Ord for Unit<'a> { | |||
There was a problem hiding this comment.
Out of curiosity, was this done for performance reasons or for correctness reasons?
There was a problem hiding this comment.
Yes, it is only for performance. The old code was rehashing every field for every comparison.
Yea, sounds good! Updated with a corresponding tracking issue. |
|
@bors: r+ 💯 |
|
📌 Commit a10eb01 has been approved by |
|
⌛ Testing commit a10eb01 with merge dc5add8ee9f6527e222be2d95b9c5672c3cca04f... |
|
💔 Test failed - status-appveyor |
|
🤔 forgot to check windows after switching to a warning. Looks like it's a little more sensitive to concurrent writers to the same library. I think the other tests should be safe. |
|
Looks like one of our old friends may be sneaking back... @bors: r+ |
|
📌 Commit 69c6363 has been approved by |
Check for duplicate output filenames. This generates an error if it detects that different units would output the same file name. This can happen in a few situations: - Multiple targets in a workspace use the same name. - `--out-dir` in some cases (like `examples` because it does not include the directory). - Bugs in cargo (like #5524, #5444) This includes a few cleanups/consolidations: - `export_path` added to `OutputFile` so there is one place where the `--out-dir` filename logic is located. - `TargetKind::description()` added for a simple way to get a human-readable name of a target kind. - The `PartialOrd` changes are a slight performance improvement (overall things are now slightly faster even though it is doing more work). Closes #5524, closes #6293.
|
☀️ Test successful - status-appveyor, status-travis |
This generates an
errorwarning if it detects that different units would output the same file name. This can happen in a few situations:--out-dirin some cases (likeexamplesbecause it does not include the directory).This includes a few cleanups/consolidations:
export_pathadded toOutputFileso there is one place where the--out-dirfilename logic is located.TargetKind::description()added for a simple way to get a human-readable name of a target kind.PartialOrdchanges are a slight performance improvement (overall things are now slightly faster even though it is doing more work).Closes #5524, closes #6293.