Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add question flag to "question" the build if it is incremental #8012

Merged
merged 41 commits into from
Apr 4, 2023

Conversation

yuehuang010
Copy link
Contributor

@yuehuang010 yuehuang010 commented Sep 28, 2022

Fixes #7348

This PR adds "question" switch to msbuild.exe that will error out if a target or a task fails incremental check. Targets will fail if both Inputs and Outputs are present and not skip. Tasks changes are individually modified to support the interface IIncrementalTask, which sets the question boolean. Each task will need to be updated this interface take part.

I have started with the following tasks and fixed some of the issues within MSBuild enlistment. And there are more, see the notes below. Tasks updated: ToolTask, Copy, MakeDir, Touch, WriteLinesTofile, RemoveDir, DownloadFile, Move, ZipDirectory, Unzip, GenerateResource, GenerateBindingRedirects.

Using question investigate incremental issues is orders of magnitude easier. Reading the logs is simpler and repros are more consistent. In this PR, it includes a few fixes to the common targets which address some issues.

Example of a Target I/O error.
image

External Packages Incremental Notes dotnet\sdk\7.0.100-rc.1.22431.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets Target Name="_IncrementalCleanPublishDirectory" DependsOnTargets="_GetCurrentAndPriorPublishFileWrites" WriteLinesToFile File="$(IntermediateOutputPath)$(_PublishCleanFile)" Lines="@(_CurrentPublishFileWrites)" Overwrite="true" WriteOnlyWhenDifferent="true"

.nuget\packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.22458.2\tools\VisualStudio.VsixBuild.targets
Some Copy task are missing SkipUnchangedFiles="true"

BootStrapMSBuild.targets, copy is overwriting files, thus it would never be incremental. A small refactor is required.

RAR sometimes touches the cache and cause GenerateBindingRedirects to run.
The FileState .dependencies are missing from the cache so it dirty the cache to fetched again.
See SystemState.GetAssemblyMetadata().

microsoft.sourcelink.github\1.0.0\build\Microsoft.SourceLink.GitHub.targets
Target Name="_InitializeAzureReposGitSourceLinkUrl" Inputs="@(SourceRoot)" Outputs="|%(Identity)|"

@yuehuang010 yuehuang010 changed the title [WIP] Add question flag to "question" the build if it is incremental Add question flag to "question" the build if it is incremental Oct 4, 2022
@rainersigwald
Copy link
Member

I am very skeptical of this approach. In general, incremental build problems are hard to debug because so many things are out-of-date--if there's only one thing that's broken, searching for the incremental build key phrases (is newer than) will quickly point to what's wrong. But usually there are both many things that are expected to run on every build, and an unpleasant cascade of builds from a single source of problems.

Can you give an example of a concrete real-world incremental build problem where this would help investigate? That might help understand the value.

@yuehuang010
Copy link
Contributor Author

yuehuang010 commented Oct 4, 2022

The analogy is maintaining code in hard without tests. Once tests are in place and reliable, then confidence grows and the product is more manageable.

The goal of this feature is to 1) provide a fast way to investigate the issue by stopping on first incremental issue. This will shorten investigation time and more fixing time. 2) is to establish a way to validate incremental (aka testing). At the moment, there is too existing issues for 2 to be in place, but hopefully, this will lay the foundation going forward.

Here is an example of an issue fixed in this PR. With the target "GenerateBindingRedirects" can hit an state where the IO will run indefinitely. Touch the project file without changing anything. This will trigger $(MSBuildAllProjects) on the input. The GenerateBindingRedirects task will run but won't write a new file because the content are same. The output wasn't modified and the target will repeat. While this task won't lead to disk write, it causes "noise" in the (is newer than) search.

src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs Outdated Show resolved Hide resolved
src/Tasks/Copy.cs Outdated Show resolved Hide resolved
src/Tasks/Delegate.cs Outdated Show resolved Hide resolved
src/Tasks/Touch.cs Outdated Show resolved Hide resolved
@yuehuang010
Copy link
Contributor Author

@rainersigwald Ping.

@rainersigwald rainersigwald self-assigned this Nov 4, 2022
@rainersigwald
Copy link
Member

This is on my to-do list, hopefully early this week.

@yuehuang010
Copy link
Contributor Author

Ping.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Left some comments, but I think this generally looks good. It'd also be good to add some documentation on how to use this properly, but that can wait. I can easily imagine people trying to use it on their first build and getting confused by all the errors...then being confused that they still happen if you build again...and again...

src/MSBuild/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs Outdated Show resolved Hide resolved
src/Framework/IIncrementalTask.cs Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Move.cs Show resolved Hide resolved
src/Tasks/Touch.cs Show resolved Hide resolved
src/Tasks/ZipDirectory.cs Show resolved Hide resolved
@yuehuang010
Copy link
Contributor Author

Ping @rainersigwald

@rainersigwald rainersigwald modified the milestones: VS 17.6, VS 17.7 Mar 28, 2023
@rainersigwald rainersigwald enabled auto-merge (squash) April 4, 2023 15:58
@Forgind
Copy link
Member

Forgind commented Apr 4, 2023

@rainersigwald, looks like resource files are unhappy; may need to build + push again

@Forgind Forgind closed this Apr 4, 2023
auto-merge was automatically disabled April 4, 2023 16:26

Pull request was closed

@Forgind
Copy link
Member

Forgind commented Apr 4, 2023

I think I screwed up 😢

@Forgind
Copy link
Member

Forgind commented Apr 4, 2023

I have a meeting but will try to fix afterwards

@Forgind Forgind reopened this Apr 4, 2023
@rainersigwald rainersigwald merged commit 0aa8c5f into dotnet:main Apr 4, 2023
rainersigwald pushed a commit that referenced this pull request Apr 7, 2023
`src/MSBuild/Properties/launchSettings.json` was added to the repo as part of #8012 by mistake.
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.

[Feature Request] Add an option to error out when not incremental
4 participants