The Wayback Machine - https://web.archive.org/web/20220203211752/https://github.com/dotnet/aspnetcore/pull/39965
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

Update KnownRuntimePack as part of updating KnownFrameworkReference #39965

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Feb 3, 2022

No description provided.

@pranavkm pranavkm requested a review from as a code owner Feb 3, 2022
@wtgodbe wtgodbe mentioned this pull request Feb 3, 2022
@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Feb 3, 2022

https://dev.azure.com/dnceng/public/_build/results?buildId=1590349&view=results tests this workaround against the commits from #39853 pre-SDK update

Copy link
Member

@dougbu dougbu left a comment

It's not clear what you intend @pranavkm. As-is, the change does nothing because the %(LatestRuntimeFrameworkVersion) metadata is updated earlier in exactly the same way.

https://github.com/dotnet/aspnetcore/pull/39965/files#diff-9fb67cbc0a7ba3b793455ec41e7f36e1d019d5152674d669551810b308510d37R44-R45

@pranavkm
Copy link
Contributor Author

@pranavkm pranavkm commented Feb 3, 2022

@dougbu the targets currently update KnownFrameworkReference but not KnownRuntimePacks. In a Blazor WASM app, the latter is used to resolve the rid-specific runtime pack (Microsoft.NETCore.App.Mono.browser-wasm). Screenshot of the build logs before this change:

image

dougbu
dougbu approved these changes Feb 3, 2022
Copy link
Member

@dougbu dougbu left a comment

Ah, I understand now. Do we need to do this w/ the @(KnownAppHostPack) item for the default Core TFM too

@pranavkm
Copy link
Contributor Author

@pranavkm pranavkm commented Feb 3, 2022

Further details - the version for KnownRuntimePacks currently comes from the value the SDK bakes in. With a blazor app, it currently compiles using the version of the runtime specified by the targeting park that we override, but runs against the version from the SDK. This is problematic in that we're not testing blazor against new runtime ingestions. Additionally, it also prevents runtime insertions when the targeting pack and the runtime pack don't align. #39853 was a recent example of this.

@pranavkm
Copy link
Contributor Author

@pranavkm pranavkm commented Feb 3, 2022

Do we need to do this w/ the @(KnownAppHostPack)

I suppose we should - it's also got the SDK-specified version. That said, I don't know how that package gets used in the build.

@dougbu
Copy link
Member

@dougbu dougbu commented Feb 3, 2022

I don't know how that package gets used in the build.

I'm not positive either. I suspect your new commit will primarily affect tests that execute dotnet {something}.

@wtgodbe anything we should be concerned about For example, are there cases where the versions won't align Versions for the app host, runtime pack, and framework reference all match in the couple of SDKs I checked. Just wondering if I'm missing anything in how dotnet/runtime pushes new packages during servicing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants