Remove ViewCompilation from publish manifest #167
Conversation
|
|
||
| <PropertyGroup> | ||
| <TargetManifestFiles Condition="'$(OS)' == 'Windows_NT'">$(TargetManifestFiles);@(WindowsAspNetCoreTargetManifestFiles)</TargetManifestFiles> | ||
| <TargetManifestFiles Condition="'$(OS)' != 'Windows_NT'">$(TargetManifestFiles);@(NonWindowsAspNetCoreTargetManifestFiles)</TargetManifestFiles> |
There was a problem hiding this comment.
Wouldn't this mean we can no longer publish Linux apps from Windows and vice-versa?
JunTaoLuo
changed the title
JunTaoLuo
force-pushed
the
johluo/platform-specific-manifests
branch
from
677f152 to
9ebd42e
Compare
build/repo.targets
Outdated
| @@ -208,6 +211,11 @@ | |||
| <SetEnvironmentVariable Variable="PATH" Value="$(PATH);$(DiaSymReaderDirectory)" /> | |||
| </Target> | |||
|
|
|||
| <Target Name="_RemoveViewCompilationFromManifest"> | |||
| <Exec Command="powershell.exe -command "Get-Content $(ManifestFileWithViewCompilation) | Where-Object {$_ -inotmatch ''} | Set-Content $(ManifestFileWithViewCompilation)"" Condition="'$(OS)' == 'Windows_NT'"/> | |||
There was a problem hiding this comment.
Only need to remove on macOS and linux.
JunTaoLuo
force-pushed
the
johluo/platform-specific-manifests
branch
from
09e6b14 to
67738fe
Compare
JunTaoLuo
requested review from
pranavkm,
natemcmaster,
ryanbrandenburg and
mikeharder
|
Why don't we just remove this? https://github.com/aspnet/MetaPackages/blob/rel/2.0.0-preview2/build/common.props#L133 |
JunTaoLuo
force-pushed
the
johluo/platform-specific-manifests
branch
from
67738fe to
89a2dad
Compare
| @@ -166,6 +166,9 @@ | |||
| <!--Drop a nuspec file in artifacts for packing zip files into a nupkg--> | |||
| <Copy SourceFiles="$(RepositoryRoot)build\Build.RS.nuspec" DestinationFolder="$(ArtifactsDir)" Condition="'$(OSPlatform)'=='Linux'" /> | |||
| <WriteLinesToFile File="$(ArtifactsDir)version.txt" Lines="$(VersionPrefix)-$(VersionSuffix)" Overwrite="true" Condition="'$(OSPlatform)'=='Linux'" /> | |||
|
|
|||
| <!-- Preview 2 Workaround: remove Microsoft.AspNetCore.Mvc.Razor.ViewCompilation from publish manifest --> | |||
| <Exec Command="sed -i -e '/microsoft.aspnetcore.mvc.razor.viewcompilation/d' $(ArtifactsDir)%(PackageStoreManifestFiles.TimestampDestinationFile)" Condition="'$(OS)' != 'Windows_NT'" /> | |||
There was a problem hiding this comment.
Can't we just remove the package reference? Why mess with the build output?
There was a problem hiding this comment.
I thought this may be pulled in transitively and we want to change as few things as possible.
There was a problem hiding this comment.
Nope, it's a standalone top-level package. Should be safe to just remove the package reference.
There was a problem hiding this comment.
Actually wouldn't we need to make it a FullMetaPackagePackageNoRuntimeStoreReference? We want it to be a transitive dependency I assume? But not in the runtime store.
There was a problem hiding this comment.
But we'd need to remove the PrivateAssets=All metadata
There was a problem hiding this comment.
Spoke to @JunTaoLuo offline. Seems like this is the only way right now to get a reference in the package, but not have the package in the store.
There was a problem hiding this comment.
To clarify, we are not removing the package from the store, we just never use the one in the store by forcing ViewCompilation to be bin deployed.
There was a problem hiding this comment.
I feel the alternative which is to remove it from the store completely is doable, but I'd want to test it more since it's slightly more riskier. Figured better to unblock the build with this now.
We are running into an issue where the store generated on windows and non-windows are different. The windows stores (x86 and x64) do not contain the M.A.Mvc.Razor.ViewCompilation while the non windows store (macOS and linux) do not. Our assumption that the stores on all platforms are the same is now broken so our approach of trimming by all manifests no longer works. We need to react accordingly in the M.A.All metapackage target and the proposal is to remove the ViewCompilation entry from the manifest so it will never be trimmed.
This would resolve aspnet/MvcPrecompilation#146, note that after preview2 we plan to remove ViewCompilation from the runtime store so this will not be an issue for the next release.
I also ported the fix from #143, this helps us resolve #161 without having to manually update the target file in the M.A.All package.