Remove dep-info files as targets in themselves

[acfoltzer]

If you ask rustc to --emit dep-info, the resulting dependency file contains a rule for producing the dependency file itself. This differs from the output of gcc -MD or clang -MD, which only includes dependency rules for the object files produced.

Tools like Ninja often consume and delete dependency files as soon as they’re produced for performance reasons, particularly on Windows. In the case of rustc output, though, the recently-deleted dependency file is cached by Ninja as a target, and therefore triggers a rebuild every time.

This very small patch removes the dep-info file from the list of output filenames, so it matches the behavior of gcc and clang.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eddyb]

r? @alexcrichton

[kennytm]

Thanks for the PR! We’ll periodically check in on it to make sure that @alexcrichton or someone else from the team reviews it soon.

[alexcrichton]

Hm so my main worry here would be in the area of breaking changes. This is technically a breaking change because there may be systems (I'm not immediately aware of any myself though) which could be relying on this behavior.

The main consumer (I think) of these files, Cargo, would be ok with this change and I believe wouldn't need an update. The original project to add this feature (Servo) has since moved to Cargo and I believe no longer needs it. That being said I wasn't aware of direct ninja/rustc integration so there may be other integrations elsewhere that aren't using Cargo but are relying on this behavior.

Could you detail a bit more about why this causes problems with Ninja? Does Ninja assume that all files listed as targets are outputs? Could a Ninja-local patch perhaps be applied? Could this be opt-in instead of a breaking change?

[acfoltzer]

Could you detail a bit more about why this causes problems with Ninja? Does Ninja assume that all files listed as targets are outputs?

Yes, it does; the relevant section of the Ninja user guide is here.

Could a Ninja-local patch perhaps be applied?

I'm not familiar enough with the range of Ninja use cases to say definitively. My reading of the manual is that it expects gcc's behavior, but I'm not sure how strictly.

Could this be opt-in instead of a breaking change?

Probably, though I would need a bit more guidance to know what the appropriate means would be for users to opt in. --emit dep-info-gcc maybe, or -C dep-info-style=gcc? The current patch required comparatively fewer UI decisions 😉

[acfoltzer]

Also,

That being said I wasn't aware of direct ninja/rustc integration so there may be other integrations elsewhere that aren't using Cargo but are relying on this behavior.

My motivation for this patch is to improve the support for defining rustc targets with the Meson build system, which uses Ninja as a backend. I'm not aware of any other such uses, but this issue is currently preventing the Meson support from working properly (see my incorrect attempt at a Meson PR here)

[carols10cents]

ping @alexcrichton , what do you think?

[alexcrichton]

@acfoltzer ah sorry for the delay, fell off my radar. Thanks for the background info though! Would it be possible for the integration you're working with to perhaps postprocess the dep-info file to remove this output? I'm still pretty worried about causing a breaking change for projects, as I think a project using a Makefile could be broken by a change like this?

[acfoltzer]

@alexcrichton It would certainly be possible, but I would much rather see rustc just have consistent behavior (at least as an option) with gcc and clang. I'm sympathetic to the breaking change concern, though; do you have thoughts on the option styles I proposed above?

[alexcrichton]

@acfoltzer oh yeah I think either of those would be fine as well, we'd probably start it out as an unstable feature like -Z dep-info-omit-d-output or something like that and then we could stabilize it in due time.

[acfoltzer]

@alexcrichton sounds great, 8c09d29 implements -Z dep-info-d-target. I welcome suggestions for how/whether to add a test; I looked through the existing suite and didn't see an obvious spot for something like this.

[alexcrichton]

@bors: r+

Looks great, thanks!

[bors]

📌 Commit 8c09d29 has been approved by alexcrichton

[bors]

⌛ Testing commit 8c09d29 with merge a1fd7bd...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry

o_O what happened

[bors]

⌛ Testing commit 8c09d29 with merge 0232694...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry

Took more than 30 minutes to compile librustc on the macs.

[bors]

⌛ Testing commit 8c09d29 with merge a6c95cd85ab8f4861d1a31887e821f69b3d7d3a2...

[acfoltzer]

@kennytm is there anything to do on my end to smooth this out?

[bors]

💔 Test failed - status-appveyor

[alexcrichton]

@bors: retry

• Spurious appveyor 32-bit test timeouts #46903

[bors]

⌛ Testing commit 8c09d29 with merge 3e49ada...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 3e49ada to master...