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/9.0-staging] ILC: Allow OOB reference to upgrade framework assembly #110058

Open
wants to merge 2 commits into
base: release/9.0-staging
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 21, 2024

Backport of #109988 to release/9.0-staging

/cc @sbomer

Customer Impact

Fixes a customer reported issue where the ILC build logic was making it impossible to upgrade FX assemblies via OOB references.

Regression

  • Yes
  • No

The inability to upgrade framework assemblies via OOB reference in ILC is not a regression, but changes in the particular OOB package the customer tried made this look like a regression when they upgraded to the latest version of this package.

Testing

Tested locally in a few different configurations:

  • With the repro from Possible System.Diagnostics.DiagnosticSource trimming issue #109872, using IlcBuildTasksPath to point to a local build of the task. Validated ILC picks the 9.0.0 version of the referenced assembly.
  • Using PackageReference to Microsoft.DotNet.ILCompiler version 9.0.0 in a net8.0 app (validated it doesn't break the ability to upgrade via manual PackageReference)
  • Using PackageReference to Microsoft.DotNet.ILCompiler version 8.0.1 in a net8.0 app (validated it produces the expected error when trying to use System.Private.CoreLib from the coreclr runtime pack).

No automated tests were added because we don't have a good place for this kind of integration test (see discussion in #109988).

Risk

Low. The impact is limited to native AOT scenarios, and is targeted specifically at the scenario where a PackageReference introduces a conflict with a framework assembly. The new error only shows up in scenarios that were already broken, but turns an unpredictable failure mode (failure during ILC with an unhelpful error message about some implementation details) into a predictable one.

Copy link
Contributor

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

@sbomer sbomer added the Servicing-consider Issue for next servicing release review label Nov 21, 2024
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. we will take for consideration in 9.0.x

@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Nov 22, 2024
@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 2, 2024
@jeffschwMSFT jeffschwMSFT modified the milestones: 9.0.x, 9.0.1 Dec 2, 2024
{
if (GetFileVersion(itemSpec).CompareTo(GetFileVersion(frameworkItem.ItemSpec)) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Conflict resolution also checks assembly version first, but I don't think our current packages have any case where we might ship an older assembly version with newer file version (EG: newer package from lower servicing band). We used to - which is why that check in conflict resolution was required - but a single file version check should be good enough now since I think we give every assembly the same major.minor file and assembly version. Does that sound right @ViktorHofer?

Copy link
Member

Choose a reason for hiding this comment

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

The only exception that I know of in dotnet/runtime is System.Runtime.Serialization.Formatters

OOB 9.0.0 package assembly:

image

inbox 9.0.0 assembly:

image

There might be other packages, not sure. The above don't probably doesn't matter because only the inbox assembly has the difference, not the packaged-up assembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants