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

Building ILC causes double writes in the runtime build #89763

Closed
ViktorHofer opened this issue Aug 1, 2023 · 4 comments · Fixed by #98342
Closed

Building ILC causes double writes in the runtime build #89763

ViktorHofer opened this issue Aug 1, 2023 · 4 comments · Fixed by #98342

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 1, 2023

Repro: build.cmd/sh clr+libs /bl

Inspect the binary log and find the following double write:

image

This is problematic as it makes the build slower as it needs to be and might break the incremental build feature in msbuild. After fixing this issue, we should consider adding CI protection to guard against double writes by doing something similar to https://www.meziantou.net/detecting-double-writes-in-msbuild-using-the-binlog.htm. AFAIK the roslyn team already does that.

cc @MichalStrehovsky

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 1, 2023
@ghost
Copy link

ghost commented Aug 1, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Repro: build.cmd/sh clr+libs /bl

Inspect the binary log and find the following double write:

image

This is problematic as it makes the build slower as it needs to be and might break the incremental build feature in msbuild. After fixing this issue, we should consider adding CI protection to guard against double writes by doing something similar to https://www.meziantou.net/detecting-double-writes-in-msbuild-using-the-binlog.htm. AFAIK the roslyn team already does that.

Author: ViktorHofer
Assignees: -
Labels:

area-Infrastructure-coreclr, untriaged

Milestone: -

@MichalStrehovsky
Copy link
Member

This is related to/same as #79575. The SDK dumps an apphost there that we then need to delete:

<Target Name="CopyNativeBinary" AfterTargets="Publish">
<!-- replace apphost with binary we generated during native compilation -->
<Delete Files="$(PublishDir)\$(TargetName)$(NativeBinaryExt)" />
<Copy SourceFiles="$(NativeOutputPath)$(TargetName)$(NativeBinaryExt)" DestinationFolder="$(PublishDir)" />
<!-- dotnet CLI produces managed debug symbols, which we will delete and copy native symbols instead -->
<Delete Files="$(PublishDir)\$(TargetName).pdb" />
<PropertyGroup>
<_symbolExt Condition="'$(OS)' == 'Windows_NT'">$(NativeSymbolExt)</_symbolExt>
<_symbolExt Condition="'$(OS)' != 'Windows_NT'">$(NativeBinaryExt)$(NativeSymbolExt)</_symbolExt>
</PropertyGroup>
<!-- replace native symbol file if it exists -->
<Delete Files="$(PublishDir)\$(TargetName)$(_symbolExt)" />
<Copy SourceFiles="$(NativeOutputPath)$(TargetName)$(_symbolExt)" DestinationFolder="$(PublishDir)"
Condition="Exists('$(NativeOutputPath)$(TargetName)$(_symbolExt)') and '$(CopyOutputSymbolsToPublishDirectory)' == 'true'" />
</Target>

This code originates from times when we couldn't put customizations into SDK because native AOT was an experimental project. It's no longer so.

@MichalStrehovsky MichalStrehovsky added this to the 9.0.0 milestone Aug 1, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 1, 2023
@ViktorHofer
Copy link
Member Author

I see. Do you know if there's something that we can do in runtime meanwhile to avoid the double write?

@MichalStrehovsky
Copy link
Member

I don't like that we do that in the product itself either. I don't think there's anything we can do from here besides patching the SDK targets in the nuget cache after they restore.

The SDK gets really mad if UseAppHost is false on something it considers self-contained. I was trying to fix this in dotnet/sdk#33229 that you're Cc'd on but it's currently in limbo because I'm sure it will break something else and I'm not mentally capable to deal with the worms from that can at this time. I'll be taking some time off soon so maybe after that.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 13, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants