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

Use 8.0 era dependencies for non net9.0 TFMs #5470

Merged
merged 12 commits into from
Oct 19, 2024

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Oct 3, 2024

Fixes #5462

cc: @eerhardt

This change makes it such that packages produced from this repository will depend on 8.0 era packages when consuming them from a project that targets net8.0 (and net462), and on 9.0 era packages when consuming them from a project that targets net9.0.

This is in favor of allowing customers that can only consume LTS versions of the rest of .NET to not be lifted when referencing packages from dotnet/extensions.

FYI: @ericstj @stephentoub @sebastienros @DamianEdwards @RussKie

Microsoft Reviewers: Open in CodeFlow

eng/packages/General.props Outdated Show resolved Hide resolved
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

I have a feeling we should go back to the original R9 organisation - i.e., have eng/packages/General.props contain only the general packages, and then have eng/packages/net80.props and eng/packages/net90.props that would contain TFM-specific packages.

@joperezr
Copy link
Member Author

joperezr commented Oct 4, 2024

I have a feeling we should go back to the original R9 organisation - i.e., have eng/packages/General.props contain only the general packages, and then have eng/packages/net80.props and eng/packages/net90.props that would contain TFM-specific packages.

Fair enough, I'm not sure how that will work for projects that are not multi-targeting as TFM is not always set at the right time, but I can try doing this and see how it behaves.

@RussKie
Copy link
Member

RussKie commented Oct 8, 2024

I have a feeling we should go back to the original R9 organisation - i.e., have eng/packages/General.props contain only the general packages, and then have eng/packages/net80.props and eng/packages/net90.props that would contain TFM-specific packages.

Fair enough, I'm not sure how that will work for projects that are not multi-targeting as TFM is not always set at the right time, but I can try doing this and see how it behaves.

Similar to https://github.com/dotnet/r9/tree/main/eng/Packages.

eng/Versions.props Outdated Show resolved Hide resolved
@stephentoub
Copy link
Member

@joperezr, what's the schedule for getting this in? We're trying to work through what to do for the various M.E.AI packages.

eng/Versions.props Outdated Show resolved Hide resolved
@joperezr
Copy link
Member Author

@joperezr, what's the schedule for getting this in? We're trying to work through what to do for the various M.E.AI packages.

I'm working on this now, trying to get this in today, or early next week at the latest.

@stephentoub
Copy link
Member

@joperezr, what's the schedule for getting this in? We're trying to work through what to do for the various M.E.AI packages.

I'm working on this now, trying to get this in today, or early next week at the latest.

Thanks. Since you created this, we added the M.E.AI packages. We should be able to have the M.E.AI.Abstractions library on the same plan as what you're creating here, and with a few tweaks, most of the others. The M.E.AI one, though, will need to be exempt for now.

joperezr and others added 2 commits October 18, 2024 12:42
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@joperezr joperezr requested review from a team as code owners October 18, 2024 20:08
@joperezr joperezr requested a review from a team as a code owner October 18, 2024 22:27
@joperezr
Copy link
Member Author

Ok, I think this is ready to go and I've resolved all conversations. Marking as auto-merge so that this can go in with the next sign-off.

@joperezr joperezr enabled auto-merge (squash) October 18, 2024 22:43
@joperezr
Copy link
Member Author

@stephentoub looks like the correctness warnings we are getting now are around nullability likely caused by the downgrade of S.T.J and missing annotations. Assuming that in this case the right thing is to suppress the warnings?

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

I'm not sure splitting the PackageVersions into 2 files helps that much, since the conditions across the 2 files need to line up. But I don't feel strongly it needs to be changed back.

LGTM. We need to make this change for 9.0.0.

@stephentoub
Copy link
Member

Not sure why this triggers nullability warnings, but go ahead and revert the azure inference change and I'll fix it in a subsequent pr.

@joperezr joperezr merged commit eafdf6e into dotnet:main Oct 19, 2024
6 checks passed
@joperezr
Copy link
Member Author

Thanks a lot @stephentoub and @eerhardt for helping pushing this through.

@RussKie
Copy link
Member

RussKie commented Oct 21, 2024

@stephentoub this change has upset the CI build:
image

##[error].dotnet\sdk\9.0.100-rtm.24479.2\Microsoft.Common.CurrentVersion.targets(2413,5): error MSB3277: (NETCORE_ENGINEERING_TELEMETRY=Build) Found conflicts between different versions of "System.Text.Encodings.Web" that could not be resolved.
There was a conflict between "System.Text.Encodings.Web, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" and "System.Text.Encodings.Web, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51".
    "System.Text.Encodings.Web, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" was chosen because it was primary and "System.Text.Encodings.Web, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" was not.
    References which depend on "System.Text.Encodings.Web, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" [D:\a\_work\1\s\.packages\system.text.encodings.web\6.0.0\lib\net6.0\System.Text.Encodings.Web.dll].
        D:\a\_work\1\s\.packages\system.text.encodings.web\6.0.0\lib\net6.0\System.Text.Encodings.Web.dll
          Project file item includes which caused reference "D:\a\_work\1\s\.packages\system.text.encodings.web\6.0.0\lib\net6.0\System.Text.Encodings.Web.dll".
            D:\a\_work\1\s\.packages\system.text.encodings.web\6.0.0\lib\net6.0\System.Text.Encodings.Web.dll
    References which depend on or have been unified to "System.Text.Encodings.Web, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" [].
        D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.AzureAIInference\Debug\net9.0\Microsoft.Extensions.AI.AzureAIInference.dll
          Project file item includes which caused reference "D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.AzureAIInference\Debug\net9.0\Microsoft.Extensions.AI.AzureAIInference.dll".
            D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.AzureAIInference\Debug\net9.0\Microsoft.Extensions.AI.AzureAIInference.dll
        D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.Ollama\Debug\net9.0\Microsoft.Extensions.AI.Ollama.dll
          Project file item includes which caused reference "D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.Ollama\Debug\net9.0\Microsoft.Extensions.AI.Ollama.dll".
            D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.Ollama\Debug\net9.0\Microsoft.Extensions.AI.Ollama.dll
        D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.OpenAI\Debug\net9.0\Microsoft.Extensions.AI.OpenAI.dll
          Project file item includes which caused reference "D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.OpenAI\Debug\net9.0\Microsoft.Extensions.AI.OpenAI.dll".
            D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI.OpenAI\Debug\net9.0\Microsoft.Extensions.AI.OpenAI.dll
        D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI\Debug\net9.0\Microsoft.Extensions.AI.dll
          Project file item includes which caused reference "D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI\Debug\net9.0\Microsoft.Extensions.AI.dll".
            D:\a\_work\1\s\artifacts\bin\Microsoft.Extensions.AI\Debug\net9.0\Microsoft.Extensions.AI.dll

@joperezr
Copy link
Member Author

Well, I agree it was related, it was just against the source index portion of the build. I'm looking into this and will disable that if required.

@stephentoub
Copy link
Member

stephentoub commented Oct 22, 2024

Why didn't it fail CI as part of this PR?

Regardless, putting this back might fix it:
https://github.com/dotnet/extensions/pull/5470/files#diff-9bab1a900200d314d150175c2ee3e46a80f58a134315991e817f41ada7b2f7e9L30-L31

@RussKie
Copy link
Member

RussKie commented Oct 22, 2024

There are steps that can only be run on the internal infra.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using v9.0 packages on .NET 8 shouldn't lift Microsoft.Extensions libraries out of the shared framework
5 participants