Warn when rustc output conflicts with existing directories by varkor · Pull Request #47203 · rust-lang/rust

varkor · 2018-01-05T00:31:55Z

When the compiled executable would conflict with a directory, display a
rustc error instead of a verbose and potentially-confusing linker
error. This is a usability improvement, and doesn’t actually change
behaviour with regards to compilation success. This addresses the
concern in #35887. Fixes#13098.

rust-highfive · 2018-01-05T00:32:06Z

r? @pnkfelix

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

kennytm · 2018-01-05T09:09:42Z

OK so why the existing directory part ends with a single quote?

An inadvertent stylistic experiment... Fixed.

kennytm · 2018-01-05T14:33:16Z

The run-make/compiler-lookup-paths-2 test is still failing.

pnkfelix · 2018-01-05T15:51:34Z

This code was able to limp along without a comment before since it was singly-purposed. But if you're going to generalize it with a higher-order function, you should add documentation stating what it does. (E.g. something like "Runs f on every potential output file path, returning this first for which f returns non-none, or None if f is none for all paths.")

pnkfelix · 2018-01-05T16:04:04Z

As @kennytm notes, the run-make/compiler-lookup-paths-2 is failing.

From what I can tell, this is happening because your code is (reasonably?) assuming that the paths enumerated by the contains_path (aka check_output after your changes) represent the exact set of paths that we will try to write to in this compilation attempt.

However, from what I can tell, when the OutputType is OutputType::Exe, we may apply target-dependent transformations to find the actual file path. See rustc_trans_utils::link::out_filename for more details here.

pnkfelix · 2018-01-05T16:06:59Z

However you decide to resolve this, it would probably be good to ensure that driver::write_out_deps and your routine share code as appropriate to enumerate the output filenames, perhaps via similar generalization as you did when you generalized contains_path to check_output. (I again ask that you add documentation when doing such generalization.)

varkor · 2018-01-07T01:14:07Z

Thanks for the pointers @pnkfelix — they were helpful in getting on the right track. I've only got a line or so of documentation for the two new functions at the moment — let me know if you think it's still too sparse! This uncovered (the same) bug in the "don't overwrite the input file" check too, so it was helpful to run into it here.

pnkfelix · 2018-01-08T11:21:11Z

I'd personally use if let Some(ref input_path) = *input_path { ... } here, especially since the None branch is empty.

(oh but I guess the pre-existing code that you're refactoring used match. Fine to leave it as is to ease reading the diff here, I guess.)

It turns out the diffing algorithm doesn't catch on anyway, so I'll change it (considering the change is currently breaking on x86_64-pc-windows-msvc).

pnkfelix · 2018-01-08T11:23:09Z

@bors r+

bors · 2018-01-08T11:23:11Z

📌 Commit eba03fa has been approved by pnkfelix

bors · 2018-01-08T12:48:48Z

⌛ Testing commit eba03fa06e2550a62135c8f4d094fa3fb1ab3724 with merge 0dd43d7a6794e3f5aeb9d4559cbea953e48ede21...

bors · 2018-01-08T14:32:21Z

💔 Test failed - status-appveyor

kennytm · 2018-01-08T15:58:20Z

What a rare sight.

@bors retry #42693

bors · 2018-01-09T02:28:39Z

⌛ Testing commit eba03fa06e2550a62135c8f4d094fa3fb1ab3724 with merge 72c51266f24476d376160243aed05d1f36fae8cc...

bors · 2018-01-09T04:22:10Z

💔 Test failed - status-appveyor

kennytm · 2018-01-09T07:29:52Z

Likely legit error on x86_64-pc-windows-msvc. The output is empty in the new tests run-make\output-filename-conflicts-with-directory and run-make\output-filename-overwrites-input.

varkor · 2018-01-09T11:41:38Z

I guess generated_output_paths is not returning the actual output files on x86_64-pc-windows-msvc. It's going to be a little hard to test without access to a machine, but I'll try looking into it.

The failing run-make tests shouldn't be the issue, because run-make\output-filename-overwrites-input was passing before this change.

kennytm · 2018-01-17T11:11:01Z

Hi @varkor,

I guess generated_output_paths is not returning the actual output files on x86_64-pc-windows-msvc. It's going to be a little hard to test without access to a machine, but I'll try looking into it.

Have you checked the problem, or is it ready to be r+'ed again?

varkor · 2018-01-29T18:13:47Z

@pnkfelix: I think the latest changes should fix the issues on Windows! Do you think you could have a quick look over?

emilyalbini · 2018-02-05T10:36:01Z

@pnkfelix and @rust-lang/compiler, ping from triage! This PR needs a review.

estebank · 2018-02-05T16:37:29Z

@bors r+

bors · 2018-02-05T16:37:29Z

📌 Commit e92bdb9 has been approved by estebank

bors · 2018-02-06T02:51:08Z

⌛ Testing commit e92bdb9 with merge dd7be09...

…y, r=estebank Warn when rustc output conflicts with existing directories When the compiled executable would conflict with a directory, display a rustc error instead of a verbose and potentially-confusing linker error. This is a usability improvement, and doesn’t actually change behaviour with regards to compilation success. This addresses the concern in #35887. Fixes#13098.

bors · 2018-02-06T06:51:07Z

💥 Test timed out

kennytm · 2018-02-06T08:57:08Z

@bors retry

The macs were not scheduled to start.

bors · 2018-02-06T09:51:11Z

⌛ Testing commit e92bdb9 with merge ca7d839...

…y, r=estebank Warn when rustc output conflicts with existing directories When the compiled executable would conflict with a directory, display a rustc error instead of a verbose and potentially-confusing linker error. This is a usability improvement, and doesn’t actually change behaviour with regards to compilation success. This addresses the concern in #35887. Fixes#13098.

bors · 2018-02-06T12:36:51Z

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

rust-highfive assigned pnkfelix Jan 5, 2018

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2018

kennytm reviewed Jan 5, 2018
View reviewed changes

pnkfelix reviewed Jan 5, 2018
View reviewed changes

pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2018

pnkfelix reviewed Jan 8, 2018
View reviewed changes

pnkfelix 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 9, 2018

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2018

varkor added 6 commits January 29, 2018 11:05

Fix tidy error
dc274e6

Fix quotation mark
3d55974

Use correct output file paths for error checking
45e962e

Fix tidy error
ba76092

Minor refactoring
d97da7d

Warn when -C extra-filename flag is used with -o
9a2f02d

varkor force-pushed the output-filename-conflicts-with-directory branch from a23fb80 to 9a2f02dCompare January 29, 2018 12:51

Specify output filenames for compatibility with Windows
e92bdb9

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2018

kennytm 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 Feb 5, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 6, 2018

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 Feb 6, 2018

bors merged commit e92bdb9 into rust-lang:masterFeb 6, 2018

bors mentioned this pull request Feb 6, 2018
Do not run the default panic hook inside procedural macros. #47933
Merged

varkor deleted the output-filename-conflicts-with-directory branch February 6, 2018 12:43

Uh oh!

Uh oh!

Conversation

varkor commented Jan 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 5, 2018

Uh oh!

kennytmJan 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varkorJan 5, 2018

Choose a reason for hiding this comment

Uh oh!

kennytm commented Jan 5, 2018

Uh oh!

pnkfelixJan 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pnkfelix commented Jan 5, 2018

Uh oh!

pnkfelix commented Jan 5, 2018

Uh oh!

varkor commented Jan 7, 2018

Uh oh!

pnkfelixJan 8, 2018

Choose a reason for hiding this comment

Uh oh!

pnkfelixJan 8, 2018

Choose a reason for hiding this comment

Uh oh!

varkorJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

pnkfelix commented Jan 8, 2018

Uh oh!

bors commented Jan 8, 2018

Uh oh!

bors commented Jan 8, 2018

Uh oh!

bors commented Jan 8, 2018

Uh oh!

kennytm commented Jan 8, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

bors commented Jan 9, 2018

Uh oh!

kennytm commented Jan 9, 2018

Uh oh!

varkor commented Jan 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennytm commented Jan 17, 2018

Uh oh!

varkor commented Jan 29, 2018

Uh oh!

emilyalbini commented Feb 5, 2018

Uh oh!

estebank commented Feb 5, 2018

Uh oh!

bors commented Feb 5, 2018

Uh oh!

bors commented Feb 6, 2018

Uh oh!

bors commented Feb 6, 2018

Uh oh!

kennytm commented Feb 6, 2018

Uh oh!

bors commented Feb 6, 2018

Uh oh!

bors commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

varkor commented Jan 5, 2018•
edited
Loading

kennytmJan 5, 2018•
edited
Loading

pnkfelixJan 5, 2018•
edited
Loading

varkor commented Jan 9, 2018•
edited
Loading