Skip to content

Add .version file to shared framework zip #21587

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

Merged
merged 3 commits into from
May 13, 2020
Merged

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented May 7, 2020

Manual port of #21548

Attempts to resolve #21177 by including a .version file in our shared framework zip, and a versions.txt file in our shared framework .nupkg. This more closely mimics what happens in microsoft.netcore.app

Testing confirms that the shared framework zips now contain .version in shared\Microsoft.AspNetCore.App\3.1.4, while the .nupkg still includes a versions.txt file.

@wtgodbe wtgodbe requested review from mthalman and a team May 7, 2020 19:08
@ghost ghost added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label May 7, 2020
@wtgodbe
Copy link
Member Author

wtgodbe commented May 12, 2020

Expected file to exist at /private/tmp/helix/working/A9E30922/w/AC52095B/e/.dotnet32361/shared/Microsoft.AspNetCore.App/5.0.0-preview.5.20253.7/.version but it did not

It looks like in Helix, ASPNET_RUNTIME_PATH is set to the location of AspNetCore in the SDK, so ItContainsVersionFile and other similar tests aren't actually testing the shared framework we just built, they're testing the SDK we're using. @HaoK should we just disable these in Helix? How would we do that?

@HaoK
Copy link
Member

HaoK commented May 12, 2020

Yeah looks like I might have messed up the conversion to C# of the install aspnet ref package code. I can take a look at fixing those, feel free to just add [SkipOnHelix("aspnet ref busted")] for the failing tests, and I'll open a PR to work on reenabling them with the TestRunner fix

@HaoK
Copy link
Member

HaoK commented May 12, 2020

@wtgodbe Actually I spoke too soon, looks like these tests are looking at the shared fx not the ref pack, and I checked out the artifacts on the build: https://dev.azure.com/dnceng/public/_build/results?buildId=635302&view=artifacts&type=publishedArtifacts

in Runtime.win-x64.5.0.0-ci, I still see Microsoft.AspNetCore.App.vesrions.txt there, I don't see a .versions file, so I think its copying the built fx correctly, we just aren't producing the .version still

Found AspNetRuntime: Microsoft.AspNetCore.App.Runtime.win-x64.5.0.0-ci.nupkg, extracting *.txt,json,dll to /private/tmp/helix/working/A9E30922/w/AC52095B/e/.dotnet32361/shared/Microsoft.AspNetCore.App/5.0.0-preview.5.20253.7

Displaying directory contents for /private/tmp/helix/working/A9E30922/w/AC52095B/e/.dotnet32361/shared/Microsoft.AspNetCore.App/5.0.0-preview.5.20253.7:
Microsoft.AspNetCore.ResponseCaching.dll
Microsoft.AspNetCore.ResponseCompression.dll
Microsoft.AspNetCore.Rewrite.dll
Microsoft.AspNetCore.Cryptography.Internal.dll
Microsoft.AspNetCore.Connections.Abstractions.dll
Microsoft.Win32.SystemEvents.dll
Microsoft.AspNetCore.StaticFiles.dll
Microsoft.AspNetCore.Mvc.DataAnnotations.dll
Microsoft.Extensions.Localization.dll
Microsoft.Extensions.WebEncoders.dll
Microsoft.AspNetCore.Cors.dll
Microsoft.AspNetCore.Authorization.Policy.dll
Microsoft.Extensions.ObjectPool.dll
Microsoft.AspNetCore.DataProtection.dll
System.Security.Permissions.dll
Microsoft.AspNetCore.Cryptography.KeyDerivation.dll
Microsoft.AspNetCore.Localization.Routing.dll
Microsoft.AspNetCore.Identity.dll
Microsoft.Extensions.Identity.Core.dll
Microsoft.Net.Http.Headers.dll
Microsoft.AspNetCore.Session.dll
Microsoft.Extensions.Hosting.Abstractions.dll
Microsoft.Extensions.Logging.dll
Microsoft.AspNetCore.Http.Features.dll
Microsoft.AspNetCore.Components.Server.dll
Microsoft.AspNetCore.Http.Abstractions.dll
Microsoft.AspNetCore.CookiePolicy.dll
Microsoft.Extensions.Caching.Abstractions.dll
Microsoft.AspNetCore.Antiforgery.dll
Microsoft.Extensions.Logging.EventSource.dll
Microsoft.Extensions.Options.ConfigurationExtensions.dll
Microsoft.AspNetCore.Mvc.RazorPages.dll
Microsoft.AspNetCore.Http.Connections.Common.dll
Microsoft.Extensions.Logging.Abstractions.dll
Microsoft.AspNetCore.Razor.Runtime.dll
System.Security.Cryptography.Xml.dll
Microsoft.AspNetCore.Mvc.TagHelpers.dll
Microsoft.Extensions.Identity.Stores.dll
Microsoft.AspNetCore.Components.Web.dll
Microsoft.Extensions.Logging.Configuration.dll
Microsoft.Extensions.Configuration.UserSecrets.dll
Microsoft.AspNetCore.HttpsPolicy.dll
Microsoft.AspNetCore.App.versions.txt

@wtgodbe
Copy link
Member Author

wtgodbe commented May 12, 2020

@HaoK we are producing the .version file now, but it goes in the shared framework .zip, while the versions.txt file goes in the .nupkg (which is what dotnet/runtime does as well). Does the test check different locations locally vs. in Helix?

@HaoK
Copy link
Member

HaoK commented May 12, 2020

Ah interesting, yeah the helix test is working against the nupkg, and I'd guess the local tests are running against the shared framework, so for helix maybe the right thing to do is continue looking for the versions.txt? We could also considering sending both the zip and nupkg to helix and have explicit tests for both?

@wtgodbe
Copy link
Member Author

wtgodbe commented May 12, 2020

@HaoK where does that infra live? If it's easy enough to implement either suggestion then I can do that as part of this PR, otherwise I'd rather disable the .version test on Helix for now and file a follow-up issue to fix the tests, as without this there's a broken scenario in the SDK

@@ -133,7 +133,7 @@ public void ItContainsValidDepsJson()
[Fact]
public void ItContainsVersionFile()
{
var versionFile = Path.Combine(_sharedFxRoot, "Microsoft.AspNetCore.App.versions.txt");
var versionFile = Path.Combine(_sharedFxRoot, ".version");
Copy link
Member

Choose a reason for hiding this comment

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

Tactical fix for now could be to just have a helix guard around the expected file I guess? You can do a string.IsNullOrEmpty(Environment.GetEnvironmentVariable("ASPNET_RUNTIME_PATH")) check since this wouldn't be set locally to determine which file to look for

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed 582fe0d

@wtgodbe wtgodbe merged commit 7c58b8b into master May 13, 2020
@wtgodbe wtgodbe deleted the wtgodbe/fixupVFileMast branch May 13, 2020 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent version file compared to other frameworks
4 participants