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

NativeAOT builds aren't updated when rebuilding with different MSBuild options #88725

Closed
Sergio0694 opened this issue Jul 12, 2023 · 2 comments · Fixed by #89911
Closed

NativeAOT builds aren't updated when rebuilding with different MSBuild options #88725

Sergio0694 opened this issue Jul 12, 2023 · 2 comments · Fixed by #89911

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Jul 12, 2023

Description

Note: this issue is a follow up from a conversation with @MichalStrehovsky, @tannergooding, @SingleAccretion and others on the C# Discord. I'm aware the culprit here is likely just MSBuild, but opening this just for tracking 🙂

While working on Sergio0694/ComputeSharp#542, I noticed that rebuilding my CLI sample with NAOT using different MSBuild options was just... Not doing anything. And the resulting binary was exactly the same. I have configured a bunch of MSBuild options to build my sample with or without some more size saving options, and my assumptions was that rebuilding after setting up those new environment variables would do "the right thing", but it seems it doesn't 😅

Reproduction Steps

$env:COMPUTESHARP_SWAPCHAIN_CLI_PUBLISH_AOT='true';
dotnet publish samples\ComputeSharp.SwapChain.Cli\ComputeSharp.SwapChain.Cli.csproj -c Release -f net7.0 -r win-x64
  • Verify the resulting binary is ~2.3 MB, and has an icon
  • Run these commands:
$env:COMPUTESHARP_SWAPCHAIN_CLI_PUBLISH_AOT='true';
$env:COMPUTESHARP_SWAPCHAIN_CLI_SIZE_OPTIMIZED='true';
$env:COMPUTESHARP_SWAPCHAIN_CLI_DISABLE_REFLECTION='true';
dotnet publish samples\ComputeSharp.SwapChain.Cli\ComputeSharp.SwapChain.Cli.csproj -c Release -f net7.0 -r win-x64
  • Verify the binary is exactly the same (but it should be ~1.1 MB, and with no icon instead)

Expected behavior

The build should be retriggered with the right properties. The resulting binary should be the same you would get if you tried building with the same exact commands but from a freshly cloned repo instead, not affected by previous builds.

Actual behavior

The resulting binary doesn't change.

Known Workarounds

You can run git clean -fdx before the second build. That works, but isn't ideal.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 12, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 12, 2023
@ghost
Copy link

ghost commented Jul 12, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Note: this issue is a follow up from a conversation with @MichalStrehovsky, @tannergooding, @SingleAccretion and others on the C# Discord. I'm aware the culprit here is likely just MSBuild, but opening this just for tracking 🙂

While working on Sergio0694/ComputeSharp#542, I noticed that rebuilding my CLI sample with NAOT using different MSBuild options was just... Not doing anything. And the resulting binary was exactly the same. I have configured a bunch of MSBuild options to build my sample with or without some more size saving options, and my assumptions was that rebuilding after setting up those new environment variables would do "the right thing", but it seems it doesn't 😅

Reproduction Steps

$env:COMPUTESHARP_SWAPCHAIN_CLI_PUBLISH_AOT='true';
dotnet publish samples\ComputeSharp.SwapChain.Cli\ComputeSharp.SwapChain.Cli.csproj -c Release -f net7.0 -r win-x64
  • Verify the resulting binary is ~2.3 MB, and has an icon
  • Run these commands:
$env:COMPUTESHARP_SWAPCHAIN_CLI_PUBLISH_AOT='true';
$env:COMPUTESHARP_SWAPCHAIN_CLI_SIZE_OPTIMIZED='true';
$env:COMPUTESHARP_SWAPCHAIN_CLI_DISABLE_REFLECTION='true';
dotnet publish samples\ComputeSharp.SwapChain.Cli\ComputeSharp.SwapChain.Cli.csproj -c Release -f net7.0 -r win-x64
  • Verify the binary is exactly the same (but it should be ~1.1 MB, and with no icon instead)

Expected behavior

The build should be retriggered with the right properties. The resulting binary should be the same you would get if you tried building with the same exact commands but from a freshly cloned repo instead, not affected by previous builds.

Actual behavior

The resulting binary doesn't change.

Known Workarounds

You can run git clean -fdx before the second build. That works, but isn't ideal.

Author: Sergio0694
Assignees: -
Labels:

untriaged, area-NativeAOT-coreclr, needs-area-label

Milestone: -

@teo-tsirpanis teo-tsirpanis removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 12, 2023
@tannergooding
Copy link
Member

There are multiple options available for helping ensure that property inputs impact the rerunability of targets.

What a lot of the SDK does is similar to the following handling for GenerateAssemblyInfo: https://github.com/dotnet/sdk/blob/ce54eaa3bd5a380985825a8c789a5f7d31301884/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets

You can see that there is the main target GenerateAssemblyInfo which has the high level condition and is otherwise empty. It simply inserts itself into the right "BeforeTargets" and adds the relevant DependsOnTargets, one of which is CoreGenerateAssemblyInfo.

That target itself has the inputs/outputs where one of the inputs is GeneratedAssemblyInfoInputsCacheFile which is created by the CreateGeneratedAssemblyInfoInputsCacheFile target.

The CreateGeneratedAssemblyInfoInputsCacheFile target is going to effectively "always" run and it does the minimal steps of basically hashing all the input properties into a file, but with WriteOnlyWhenDifferent=true so the timestamp doesn't change when inputs don't change.

There are of course other options as well, but having the input properties somehow written to a file that is an "input" to the main target is the basic principle behind which it typically operates.

MichalStrehovsky added a commit that referenced this issue Aug 3, 2023
Incremental build wouldn't consider things like `OptimizationPreference` property changing as a thing that should trigger a rebuild. I feel like this is more a MSBuild bug, but it has been like this for a long time.

Turns out we can use `WriteIlcRspFileForCompilation` target as a sentinel:

* Run the target always. We only actually write out the file if it's different (`WriteOnlyWhenDifferent` is already set to `true`).
* ILC execution already specifies the RSP as one of its inputs. So if the RSP is more recent than the output, it will trigger a build.

Fixes #88725.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 3, 2023
jkotas pushed a commit that referenced this issue Aug 3, 2023
Incremental build wouldn't consider things like `OptimizationPreference` property changing as a thing that should trigger a rebuild. I feel like this is more a MSBuild bug, but it has been like this for a long time.

Turns out we can use `WriteIlcRspFileForCompilation` target as a sentinel:

* Run the target always. We only actually write out the file if it's different (`WriteOnlyWhenDifferent` is already set to `true`).
* ILC execution already specifies the RSP as one of its inputs. So if the RSP is more recent than the output, it will trigger a build.

Fixes #88725.
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Aug 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2023
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.

4 participants