Skip to content

Fix linker incrementalism #18234

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

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Jan 9, 2020

The Input/Output values previously specified on _BlazorCopyFilesToOutputDirectory could lead to corrupted builds, because this kind of incrementalism skips processing files where the output is newer than the input. The incrementalism we really want here is skipping items where the output is the same as the input (so we do copy if it's either older or newer).

As an example of how it was broken before:

  • Build with linker enabled
  • Now set <BlazorLinkOnBuild>false</BlazorLinkOnBuild>
  • Now, during the next build, there will be an item with input=<somenupkgdir>/mscorlib.dll, output=bin/.../mscorlib.dll.
    • Since there's already a file at bin/.../mscorlib.dll, and you just generated it using the linker in step 1, the output file will be newer than the input, hence it will skip copying this
  • You now have a corrupted build, in which some of the dlls have been linked and some have not. It will fail at runtime if you start using any APIs that were linked out when you did the build in step 1.

It could even break when you upgrade to new packages, even if you're always using the linker. Example:

  • Make a build with some version of packages and linker enabled
  • Now change to different dependency versions
  • Now, during the next build, there will be an item with input=<somenupkgdir>/SomeDependency.dll, output=bin/.../SomeDependency.dll.
    • Since you just generated bin/.../SomeDependency.dll on your previous build a few minutes ago, its timestamp could be newer than the file from your NuGet package cache
  • Now you have a corrupted build because it's randomly using some older versions of package assemblies than you have declared

I think the fix is to copy whenever the file is different, regardless of whether it's newer or older. This is already handled by the <CopyTask>, since we specify SkipUnchangedFiles by default. So we just have to remove the input/output declarations from _BlazorCopyFilesToOutputDirectory and let <CopyTask> take care of the incrementalism.

@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly labels Jan 9, 2020
@@ -257,6 +255,8 @@
<Output TaskParameter="Dependencies" ItemName="_BlazorResolvedRuntimeDependencies" />
</ResolveBlazorRuntimeDependencies>

<WriteLinesToFile File="$(_BlazorApplicationAssembliesCacheFile)" Lines="@(_BlazorResolvedRuntimeDependencies)" Overwrite="true" />
Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Jan 9, 2020

Choose a reason for hiding this comment

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

@pranavkm This is an entirely separate issue I noticed while looking at this. Previously, we never wrote the unlinked.output file to disk, which I guess is an unintentional omission, since some other code does try to read that file. Adding this line means we do write it to disk.

However, it's unclear to me that this achieves anything. The "input" items for _ResolveBlazorRuntimeDependencies include @(IntermediateAssembly), which is treated as changed on every build, even if you didn't modify your sources. As such, _ResolveBlazorRuntimeDependencies is never skipped, and hence we regenerate unlinked.output and @(_BlazorResolvedRuntimeDependencies) on every build.

So, what's the intention for unlinked.output? If we know the info will be recreated on every build, does that make it pointless to write it to disk?

Copy link
Contributor

Choose a reason for hiding this comment

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

include @(IntermediateAssembly), which is treated as changed on every build, even if you didn't modify your sources.

That seems really suspect. I'll have a look at it, but the rest of the change here looks great.

@SteveSandersonMS SteveSandersonMS merged commit 2e2d062 into blazor-wasm Jan 10, 2020
@SteveSandersonMS SteveSandersonMS deleted the stevesa/fix-linker-incrementalism branch January 10, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants