Skip to content

Latest targets file breaks MSBuild incremental build.#211

Merged
Scottj1s merged 3 commits into
microsoft:masterfrom
jlaanstra:jlaans/always-building
Mar 11, 2019
Merged

Latest targets file breaks MSBuild incremental build.#211
Scottj1s merged 3 commits into
microsoft:masterfrom
jlaanstra:jlaans/always-building

Conversation

@jlaanstra

Copy link
Copy Markdown
Contributor

This PR removes the Delete tasks meant to cleanup rsp files and instead puts them in the $(IntDir). The Delete of midl.rsp causes MSBuild to always think that files are missing breaking incremental build.
Additionally this also help diagnosing issues if the rsp files would ever contain invalid input.

How verified.

After making these changes, incremental builds work again.

@jlaanstra jlaanstra requested a review from Scottj1s as a code owner March 8, 2019 05:32
@jlaanstra jlaanstra changed the title Latest targets file breaks MSBuild tincremental build. Latest targets file breaks MSBuild incremental build. Mar 8, 2019
@kennykerr

Copy link
Copy Markdown
Contributor

You're on fire. Thanks for all the improvements! 👍

@jlaanstra

Copy link
Copy Markdown
Contributor Author

At the same time I'm confused by the MSBuild behavior here, as it thinks midlrt.rsp is a build output before making this change. Maybe @rainersigwald can explain why that happens?

@Scottj1s

Scottj1s commented Mar 8, 2019

Copy link
Copy Markdown
Member

Thanks Johan, we'll probably need another solution, as deleting was necessary for razzle builds.

@rainersigwald

Copy link
Copy Markdown
Member

Can you share a binlog of a case where the incremental build was decided incorrectly?

@Scottj1s Scottj1s left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think razzle should be happy with this. Can you verify?

@Scottj1s

Scottj1s commented Mar 8, 2019

Copy link
Copy Markdown
Member

I'll approve for now, and re-evaluate razzle when migration is complete

@jlaanstra

Copy link
Copy Markdown
Contributor Author

@Scottj1s if you can provide me with a directory to build and location of the targets in razzle I can give it a shot.

@jlaanstra

Copy link
Copy Markdown
Contributor Author

@rainersigwald I'll send you the binlog offline.

@rainersigwald

Copy link
Copy Markdown
Member

Thanks for the logs!

midl.exe is run under FileTracker, which observes the file read and write operations that happen inside the tools process. Since you explicitly pass a .rsp file (via AdditionalOptions on the Midl item), midl.exe reads that file. It then gets marked as an input (written to x64\Release\SharedUtils.tlog\midl.read.1.tlog) of the MIDL task.

On the second execution, two things happen:

  1. GetCppWinRTPlatformWinMDReferences runs (always, because it doesn't specify inputs/outputs), which unconditionally overwrites the .rsp file.
  2. The MIDL task runs, detects that the .rsp is newer than the output .winmd and runs.

I don't think the problem was the delete, but the unconditional overwrite, which is still there. How did you validate that this helped?

I'd suggest using the WriteOnlyWhenDifferent argument to WriteLinesToFile when writing the .rsp files, in addition to avoiding deleting them.

@jlaanstra

Copy link
Copy Markdown
Contributor Author

@rainersigwald With this change the incremental build works in VS. That's how I validated this.

@jlaanstra

Copy link
Copy Markdown
Contributor Author

@rainersigwald let me get you two new files where these changes are applied.

@jlaanstra

Copy link
Copy Markdown
Contributor Author

WrtiteOnlyWhenDifferent makes quite a bit of difference when running MSBuild directly!

@rainersigwald

Copy link
Copy Markdown
Member

Confusingly, builds in Visual Studio have two levels of incrementality:

  1. The project system for each project has a "fast up-to-date check" which decides whether to invoke MSBuild at all.
  2. Then MSBuild has incremental behavior at the target level, and some tasks (like those that use FileTracker) have it at the task level.

It sounds like your initial fix was enough to convince the C++ project system that the project was up to date, so it didn't invoke MSBuild, but the binlog showed that when MSBuild was invoked (as in a command-line build) the MIDL task was still running (and then causing a cascade: everything that depended on the .winmd would have to rebuild because the .winmd changed . . .). Using WriteOnlyWhenDifferent stops that.

One thing I forgot to mention that might be relevant since I see the word "Razzle" above: WriteOnlyWhenDifferent was new to MSBuild 15.0. If this project needs to build in an environment that's still on 14 (or lower 😱) it'll fail.

@jlaanstra

jlaanstra commented Mar 8, 2019

Copy link
Copy Markdown
Contributor Author

@Scottj1s can confirm, but I think Windows would have MSBuild 15.0 as C++/WinRT is generally quite up-to-date with regards to tooling requirements.

I think we can make this even more efficient by separating the generation of the rsp files from the exec tasks, so exec will be skipped when the rsp file didn't change. I'll try that separately from this change.

Nevermind, the commandline would be the same if the content of one of the files changed in which case we would still have to rerun the command. This only works for the midl case as it processes each file individually.

@jlaanstra

Copy link
Copy Markdown
Contributor Author

I am seeing at least one usage of WriteOnlyWhenDifferent in the razzle tooling, so it seems like it should be available.

@jlaanstra

Copy link
Copy Markdown
Contributor Author

/cc @jevansaks for incremental build improvements that this might result in.

@Scottj1s Scottj1s left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Rainer and Johan for the improvements.

@Scottj1s Scottj1s merged commit abd61e7 into microsoft:master Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants