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

Do not include the shared framework in the packages #24816

Merged
merged 4 commits into from
Aug 25, 2020

Conversation

pranavkm
Copy link
Contributor

DevServer and dotnet-watch include binaries from the ASP.NET Core shared framework
as part of the package. This change compiles these projects against the most recently built
version of the shared framework which ensures build and publish work as normals. Individual
projects from the runtime can be referenced to pick up new runtime features when necessary

DevServer and dotnet-watch include binaries from the ASP.NET Core shared framework
as part of the package. This change compiles these projects against the most recently built
version of the shared framework which ensures build and publish work as normals. Individual
projects from the runtime can be referenced to pick up new runtime features when necessary
@pranavkm pranavkm requested review from SteveSandersonMS and a team as code owners August 11, 2020 22:20
@pranavkm pranavkm requested a review from a team August 11, 2020 22:21
@pranavkm pranavkm added this to the 5.0.0-rc1 milestone Aug 11, 2020
@Pilchie Pilchie added area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Aug 12, 2020
@pranavkm
Copy link
Contributor Author

Updated based on our conversation. Like I mentioned offline, I'd like to look at moving dotnet-watch outside the AspNetCore repo for 6.0.It should minimize some of the packaging requirements at that point. We might also be able to do something similar with the Blazor dev-server depending on how much progress we make with the browser debugging work in 6.0.

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Remove the unused .nuspec file, confirm it's used, or make it used and this will be good to go.

<Reference Include="Selenium.Support" />
<Reference Include="Selenium.WebDriver" />
<ProjectReference Include="..\..\..\WebAssembly\DevServer\src\Microsoft.AspNetCore.Components.WebAssembly.DevServer.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: Why compile in these files instead of referencing the project❔ Should the files be moved into a Shared directory❔

Copy link
Member

Choose a reason for hiding this comment

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

I'm also curious about that :)

My guess is it's something nonobvious about having to run it inside docker. Perhaps @pranavkm can clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I completely lost track of this PR. The M.AspNetCore.App reference in the DevServer project is viral, any project that references it also needs to be able to resolve that reference. Compiling the relevant types by source avoids any further hacks.

<file src="$OutputPath$dotnet-watch.pdb" target="tools\$DefaultNetCoreTargetFramework$\any\dotnet-watch.pdb" />
<file src="$BrowserRefreshOutputPath$Microsoft.AspNetCore.Watch.BrowserRefresh.dll" target="tools\$DefaultNetCoreTargetFramework$\any\middleware\Microsoft.AspNetCore.Watch.BrowserRefresh.dll" />
</files>
</package>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file will be used. Suggest removing the PopulateNuspec target and adding a $(NuspecFile) setting in the project file if you want this instead of the auto-generated file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This can be removed.


<ProjectReference
Copy link
Member

Choose a reason for hiding this comment

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

nit: Might be worth a comment about ensuring the latest framework bits exist before resolving the framework ref.

@pranavkm pranavkm changed the base branch from master to release/5.0 August 17, 2020 21:25
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This all looks completely plausible and I'm confident that @pranavkm is the right person to make calls about these changes. Pranav, if you think there's anything uncertain about the validity of these changes then please speak up, otherwise as far as I'm concerned, if it works then let's ship it :)

@mkArtakMSFT mkArtakMSFT merged commit b0530a6 into release/5.0 Aug 25, 2020
@mkArtakMSFT mkArtakMSFT deleted the prkrishn/remove-shared-fx-hacks branch August 25, 2020 17:04
dougbu added a commit that referenced this pull request Sep 30, 2020
- allow sharedFx assemblies in Microsoft.AspNetCore.Components.WebAssembly.DevServer package
- see #26448 about reducing this baggage later in 6.0
dougbu added a commit that referenced this pull request Oct 10, 2020
- allow sharedFx assemblies in Microsoft.AspNetCore.Components.WebAssembly.DevServer package
- see #26448 about reducing this baggage later in 6.0
dougbu added a commit that referenced this pull request Oct 12, 2020
* Update branding to 6.0.0 Alpha1
- hold the TFM back at `net5.0`
- correct `TargetFrameworkVersion` in FrameworkList.xml; don't use `$(AspNetCoreMajorMinorVersion)`
- move `$(DefaultNetCoreTargetFramework)` to eng/Versions.props
  - avoid repeating value in AfterSolutionBuild.targets and Directory.Build.props files
- add new versions to `TemplatePackageInstaller`
- update project template test scripts

* Update / add 6.0 package versions in test expectations
- basically, remove test assumptions that branding == target TFM
- `TestData` will need another update once 6.0.0.0 assembly versions arrive from dotnet/runtime

* Update `Condition`s on `@(SuppressBaselineReference)` items to 6.0

* Undo some of #24816
- allow sharedFx assemblies in Microsoft.AspNetCore.Components.WebAssembly.DevServer package
- see #26448 about reducing this baggage later in 6.0

* Skip IndividualAuth tests on Helix
- #26776
dougbu added a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
* Update branding to 6.0.0 Alpha1
- hold the TFM back at `net5.0`
- correct `TargetFrameworkVersion` in FrameworkList.xml; don't use `$(AspNetCoreMajorMinorVersion)`
- move `$(DefaultNetCoreTargetFramework)` to eng/Versions.props
  - avoid repeating value in AfterSolutionBuild.targets and Directory.Build.props files
- add new versions to `TemplatePackageInstaller`
- update project template test scripts

* Update / add 6.0 package versions in test expectations
- basically, remove test assumptions that branding == target TFM
- `TestData` will need another update once 6.0.0.0 assembly versions arrive from dotnet/runtime

* Update `Condition`s on `@(SuppressBaselineReference)` items to 6.0

* Undo some of dotnet/aspnetcore#24816
- allow sharedFx assemblies in Microsoft.AspNetCore.Components.WebAssembly.DevServer package
- see dotnet/aspnetcore#26448 about reducing this baggage later in 6.0

* Skip IndividualAuth tests on Helix
- dotnet/aspnetcore#26776

Commit migrated from dotnet/aspnetcore@da7fead48635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants