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

[release/8.0.4xx] Fix defaulting of which Runtime(s) to containerize that led to always publishing multiple containers #46067

Open
wants to merge 5 commits into
base: release/8.0.4xx
Choose a base branch
from

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Jan 16, 2025

Fixes #46053 and part of #46015.

Description and impact

There were two bugs in the multi-architecture container publishing that led to some projects always publishing for all container architectures:

  • one bug was a typo in a property that let users override the RuntimeIdentifiers, which is often used to narrow the set of RIDs to containerize - for example to remove win-x64 from the set of RIDs to containerize
  • the other bug was in how the container targets detected if a project was intended to be published for all RIDs or just a single RID by the user

The net effect of these changes is to make most multi-RID projects publish for all RIDs regardless of the -r CLI parameter, and to also make the documented workarounds ineffective - using the container-specific RID properties.

These scenarios were hit almost immediately after the release, with customers logging issues, tagging on social media, and via Discord.

Regression

Yes - the multi-arch containers work introduced this.

Risk

Low/Medium - the changes are targeted and in all cases more safe than before - they defer to user inputs in a safer way, they make complex boolean conditionals more simple to understand and more easily visible in the binlog, and we have added significant additional test coverage.

Testing

New automated test cases have been added that verify

  • single-RID overrides are honored (-r, etc)
  • multi-RID-ness is detected correctly

Manual testing on sample projects confirms this. Providing the same targets changes to customers (cc @wasabii) has validated that pre-January-release behaviors are back.

Technical Details

Here's a simple app that demonstrates the various problems:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <RootNamespace>multi_arch_build_bug</RootNamespace>
    <ImplicitUsings>enable</ImplicitUsings>
    <RuntimeIdentifiers>linux-x64;linux-arm64;win-x64</RuntimeIdentifiers>
    <ContainerRuntimeIdentifiers>linux-x64;linux-arm64</ContainerRuntimeIdentifiers>
    <Nullable>enable</Nullable>
    <EnableSdkContainerSupport>true</EnableSdkContainerSupport>
  </PropertyGroup>

</Project>

In January's releases of 8.0.4xx and 9.0.1xx, running dotnet publish -t:PublishContainer -r linux-64 for this project will cause both ContainerRuntimeIdentifiers RIDs to be built. If a user tries to workaround this by running dotnet publish -t:PublishContainer -p ContainerRuntimeIdentifier=linux-x64, the workaround will not take effect and both RIDs will also be built.

After these changes, the following scenarios all now work:

RIDs RID CRIDs CRID result
linux-x64;linux-arm64 - - - linux-x64;linux-arm64 multi-arch container
linux-x64;linux-arm64 linux-x64 - - linux-x64 single-arch container
linux-x64;linux-arm64;win-x64 - linux-x64;linux-arm64 - linux-x64;linux-arm64 multi-arch container
linux-x64;linux-arm64 - - linux-x64 linux-x64 single-arch container
linux-x64;linux-arm64;win-x64 - linux-x64;linux-arm64 linux-x64 linux-x64 single-arch container
linux-x64;linux-arm64;win-x64 linux-x64 linux-x64;linux-arm64 - linux-x64 single-arch container

@baronfel baronfel requested a review from a team January 16, 2025 22:21
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jan 16, 2025
@baronfel baronfel added Area-Containers Related to dotnet SDK containers functionality Servicing-consider and removed Area-Infrastructure untriaged Request triage from a team member labels Jan 16, 2025
@baronfel baronfel changed the title Fix typo in condition check for container publishing that led to inadvertent multi-arch publishing [release/8.0.4xx] Fix typo in condition check for container publishing that led to inadvertent multi-arch publishing Jan 16, 2025
* using Container-specific properties to target a single RID
* using normal RID properties to target a single RID
@baronfel
Copy link
Member Author

I've added two tests for user-facing scenarios that should work but currently done. Need to send more code changes to get them green.

…fter we've checked all possible user-provided values
@baronfel
Copy link
Member Author

The build failure is the known-failing build that the IL folks say isn't a real thing.

Copy link
Member

@surayya-MS surayya-MS left a comment

Choose a reason for hiding this comment

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

Looks great!

@baronfel baronfel changed the title [release/8.0.4xx] Fix typo in condition check for container publishing that led to inadvertent multi-arch publishing [release/8.0.4xx] Fix defaulting of which Runtime(s) to containerize that led to always publishing multiple containers Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality Servicing-consider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants