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

Tests which depend on the host don't use a live built version #58109

Closed
ViktorHofer opened this issue Aug 25, 2021 · 13 comments · Fixed by #78354
Closed

Tests which depend on the host don't use a live built version #58109

ViktorHofer opened this issue Aug 25, 2021 · 13 comments · Fixed by #78354

Comments

@ViktorHofer
Copy link
Member

Both the linker tests and the singlefile tests use a prebuilt version of the host instead of the live built one. In general we want to use live built assets where possible. In addition to that, when the repository revs to the next major version and the SDK doesn't yet understand the net7.0 tfm, the prebuilt host isn't discovered without help (KnownAppHostPack AppHostPackVersion update).

TL;DR: Tests which depend on the apphost should have a dependency on the host subset being built first.

@ViktorHofer ViktorHofer added this to the 7.0.0 milestone Aug 25, 2021
@ghost
Copy link

ghost commented Aug 25, 2021

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Both the linker tests and the singlefile tests use a prebuilt version of the host instead of the live built one. In general we want to use live built assets where possible. In addition to that, when the repository revs to the next major version and the SDK doesn't yet understand the net7.0 tfm, the prebuilt host isn't discovered without help (KnownAppHostPack AppHostPackVersion update).

TL;DR: Tests which depend on the apphost should have a dependency on the host subset being built first.

Author: ViktorHofer
Assignees: -
Labels:

area-Infrastructure

Milestone: 7.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 25, 2021
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Aug 25, 2021
@jkoritzinsky
Copy link
Member

cc: @agocke

@agocke
Copy link
Member

agocke commented Aug 25, 2021

TL;DR: Tests which depend on the apphost should have a dependency on the host subset being built first.

Note: singlefilehost is part of the CLR partition. I think the single file tests all use the just-built single file host, but I may be forgetting some

@am11
Copy link
Member

am11 commented Oct 12, 2021

Tests which depend on the apphost should have a dependency on the host subset being built first.

Since cab3a63, corehost is built before libs subset, so I guess this objective is completed more broadly.

Can't wait to see live corehost being dogfooded to those tests (and other usages of dotnet(1) in the repo). :)

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jul 21, 2022

@agocke now that the live built host is part of the testhost folder this item tracks removing the remaining dependencies on a prebuilt host. It would be great if we could tackle this before we rev to .NET 8.

The change would need to be done directly in targetingpack.targets by supplying the path to the host

@am11
Copy link
Member

am11 commented Jul 21, 2022

Host packages were also removed from Versions.props in #71725.
#72371 was closed so bot can open a new PR without host dependencies updates (it hasn't opened a new PR so far).

ps- I think we can also use live ilasm and remove its package as well?

@ViktorHofer
Copy link
Member Author

ps- I think we can also use live ilasm and remove its package as well?

In theory yes, but the tricky part is the Microsoft.NET.IL.Sdk which today is used by all the .ilproj project files. We would need to convert those to not use the Sdk attribute and instead import the .targets file manually.

@hoyosjs
Copy link
Member

hoyosjs commented Jul 22, 2022

ps- I think we can also use live ilasm and remove its package as well?

In theory yes, but the tricky part is the Microsoft.NET.IL.Sdk which today is used by all the .ilproj project files. We would need to convert those to not use the Sdk attribute and instead import the .targets file manually.

There's an extension point in the SDK, where setting ILAsmToolPath is enough to use a custom ILAsm. We use it here:

<ILAsmToolPath Condition="'$(DotNetBuildFromSource)' == 'true' or '$(BuildArchitecture)' == 's390x'">$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'coreclr', '$(TargetOS).$(TargetArchitecture).$(Configuration)'))</ILAsmToolPath>
for s390x and sourcebuild. Don't see why not extend except for having to build the IL tools component.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jul 22, 2022

Right, we definitely want to use such an extension point, but I was referring to that even then we would still use the Microsoft.NET.IL.Sdk prebuilt package which we should also get rid off to avoid the self dependency in the source build graph.

See #71326 for more details.

@am11
Copy link
Member

am11 commented Jul 22, 2022

@ViktorHofer, this is an interesting point. Can Microsoft.NET.HostModel and its tests also opt into the approach you mentioned, so they could dogfood the live runtime and libs? Currently, src/installer/**/*.csproj need to wait for SDK update (which typically means months of waiting) to consume the new APIs added in runtime libraries.

@ViktorHofer
Copy link
Member Author

Yes they can and should. I Was attempting to use the live System.Text.Json at some point but gave up because of an infrastructure issue. We should definitely make this work.

@am11
Copy link
Member

am11 commented Jul 22, 2022

Yup, HostModel and DependencyModel can get rid of Newtonsoft.Json and FluentAssertions dependencies. I wanted to remove some reflection code in installer tests, which is waiting for 7.0.0-preview7+ (to be able to use UnixFileMode APIs added one month ago, so it's still some weeks of waiting before I can use them..) 😁

@hoyosjs hoyosjs modified the milestones: 7.0.0, 8.0.0 Aug 10, 2022
ViktorHofer added a commit to dotnet/sdk that referenced this issue Nov 20, 2022
Contributes to dotnet/runtime#58109

In dotnet/runtime we live build targeting packs, runtime packs and app host packs. For the former two, switches already exist to disable nuget restore:
- EnableTargetingPackDownload
- EnableRuntimePackDownload

The latter one doesn't yet have such a switch and therefore is always restored by NuGet. In the cases where want to use the live built apphost pack, restore fails, i.e. from a runtime build:

`artifacts/bin/trimmingTests/projects/Microsoft.Extensions.DependencyInjection.TrimmingTests/ActivatorUtilitiesTests/osx-x64/project.csproj(0,0): error NU1102: (NETCORE_ENGINEERING_TELEMETRY=Build) Unable to find package Microsoft.NETCore.App.Host.osx-x64 with version (= 8.0.0)`
ViktorHofer added a commit to dotnet/sdk that referenced this issue Nov 21, 2022
* Add switch to disable apphost pack restore

Contributes to dotnet/runtime#58109

In dotnet/runtime we live build targeting packs, runtime packs and app host packs. For the former two, switches already exist to disable nuget restore:
- EnableTargetingPackDownload
- EnableRuntimePackDownload

The latter one doesn't yet have such a switch and therefore is always restored by NuGet. In the cases where want to use the live built apphost pack, restore fails, i.e. from a runtime build:

`artifacts/bin/trimmingTests/projects/Microsoft.Extensions.DependencyInjection.TrimmingTests/ActivatorUtilitiesTests/osx-x64/project.csproj(0,0): error NU1102: (NETCORE_ENGINEERING_TELEMETRY=Build) Unable to find package Microsoft.NETCore.App.Host.osx-x64 with version (= 8.0.0)`
ViktorHofer added a commit to carlossanlop/runtime that referenced this issue Nov 21, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 21, 2022
@ViktorHofer
Copy link
Member Author

The NativeExports.csproj is yet another component that use a prebuilt version of the host instead of the live built version. DNNE is invoked and given the apphost path. We should fix this by defining a dependency to the apphost from NativeExports.csproj. We don't need the singlefilehost, just the apphost. Related: #79144.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
5 participants