-
Notifications
You must be signed in to change notification settings - Fork 752
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
[main] Update dependencies from dotnet/aspnetcore #4061
Merged
dotnet-maestro
merged 6 commits into
main
from
darc-main-fa9b88de-1ffe-4006-9ec1-0f3815443c10
Jun 12, 2023
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7fc808d
Update dependencies from https://github.com/dotnet/aspnetcore build 2…
dotnet-maestro[bot] 6ddc7f2
Upgrading SDK to fix missing method exception
joperezr bfee67a
Update dependencies from https://github.com/dotnet/aspnetcore build 2…
dotnet-maestro[bot] 7d5dd59
Update dependencies from https://github.com/dotnet/aspnetcore build 2…
dotnet-maestro[bot] f1f5f5c
Adding dotnet7 nuget feed since latest SDK requires that for netcorea…
joperezr 0031bad
Fixing type conflict due to missaligned versions and fix small new an…
joperezr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding this? We shouldn't have any dependencies on .NET 7 feed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitek-karas could you help us understand why we have .NET 7 dependency for .NET Core 3.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that this file gets generated here: https://github.com/dotnet/installer/blob/f8275a0edc0a90b593f2e3f73c4b9b3e908e3e22/src/redist/targets/GenerateBundledVersions.targets#L989-L991
Which uses this version:
https://github.com/dotnet/installer/blob/f8275a0edc0a90b593f2e3f73c4b9b3e908e3e22/eng/Versions.props#L27-L29
That is why we ended up having to add the dependency to the .net 7 feed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbomer would know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change now causes downstream repos to stop building unless those add a dependency on dotnet7 feed (which is a very undesirable thing):
https://dev.azure.com/dnceng-public/public/_build/results?buildId=307960&view=logs&j=0bc77094-9fcd-5c38-f6e4-27d2ae131589&t=a78ba58e-354d-5443-f28e-fcee6d8f5be6&l=17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the file @joperezr pointed to says that when targeting netcoreapp3.1, we need to use the net7.0 version of this package (targeting net7.0 has the same requirement). There's normally some process to keep the 7.0 versions required by the 8.0 SDK in sync - so that the 8.0 SDK depends on 7.0 servicing versions released at a similar cadence.
What seems to have gone wrong here is that the 7.0 packages didn't get published. I checked that the 7.0.304 SDK uses the 7.0.100-1.23211.1 version of ILLink.Tasks, but it hasn't been published for use by the 8.0 SDK. I will follow up with the folks responsible for publishing these packages offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sbomer. Should the packages be published to dotnet8 (or perhaps dotnet-public) feed, or is there an expectation that consumers will be required to pull from dotnet7 feed?
We can't pull from non-managed feeds (like nuget.org).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcpopMSFT what's the normal location for downlevel packages that the SDK depends on when targeting downlevel TFMs? Do those go to the dotnet-public feed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may of course be wrong here so I'll wait for @marcpopMSFT to answer the above question, but IMO any packages that are produced by .NET and may be required by the .NET 8 SDK and is not already in the dotnet-public feed (meaning it's not yet in NuGet), then I believe it should be in the dotnet8 feed. It doesn't "feel right" to have to depend on the dotnet7 feed when using the latest .NET 8 SDK.