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

RID-based restore should not be necessary for PSCD apps #34198

Closed
richlander opened this issue Jul 24, 2023 · 7 comments
Closed

RID-based restore should not be necessary for PSCD apps #34198

richlander opened this issue Jul 24, 2023 · 7 comments
Assignees

Comments

@richlander
Copy link
Member

This is my experience.

root@015a54c22b5a:/app# cat *.csproj
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishSelfContained>true</PublishSelfContained>
  </PropertyGroup>

</Project>
root@015a54c22b5a:/app# dotnet restore
  Determining projects to restore...
  All projects are up-to-date for restore.
root@015a54c22b5a:/app# dotnet publish --no-restore
MSBuild version 17.7.0+5785ed5c2 for .NET
/usr/share/dotnet/sdk/8.0.100-preview.6.23330.14/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(314,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [/app/app.csproj]
/usr/share/dotnet/sdk/8.0.100-preview.6.23330.14/Sdks/Microsoft.NET.Sdk/targets/Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1047: Assets file '/app/obj/project.assets.json' doesn't have a target for 'net8.0/linux-x64'. Ensure that restore has run and that you have included 'net8.0' in the TargetFrameworks for your project. You may also need to include 'linux-x64' in your project's RuntimeIdentifiers. [/app/app.csproj]
root@015a54c22b5a:/app# dotnet restore -r linux-x64
  Determining projects to restore...
  Restored /app/app.csproj (in 2.16 sec).
root@015a54c22b5a:/app# dotnet publish --no-restore
MSBuild version 17.7.0+5785ed5c2 for .NET
/usr/share/dotnet/sdk/8.0.100-preview.6.23330.14/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(314,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [/app/app.csproj]
  app -> /app/bin/Release/net8.0/linux-x64/app.dll
  app -> /app/bin/Release/net8.0/linux-x64/publish/

I assume this would be fixed if RID-specific was the default. It's very odd that apps with PublishSelfContained are portable for build.

@agocke @nagilson

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Jul 24, 2023
@richlander richlander changed the title RID-based restore should not be necessary RID-based restore should not be necessary for PSCD apps Jul 24, 2023
@nagilson
Copy link
Member

I believe the problem is the following. The app is not RID specific by default. But SelfContained infers a RID with the change we made last year. But PublishSelfContained only sets SelfContained if you are publishing. So: the restore was portable but our publish with no-restore was RID specific, and it failed because it did not restore the necessary assets.

If we make publish self contained give a rid when you restore, then it would break build in the same fashion. I think this ultimately comes back to this: to fix this we would need to make all apps rid specific by default.

@nagilson nagilson removed the untriaged Request triage from a team member label Jul 25, 2023
@richlander
Copy link
Member Author

Let's do a thought exercise ...

Today, publish configuration influences build analyzers, via Publish* properties. Let's assume that we did the same with this. We can could make PublishSelfContained apps RID-specific. Why isn't that possible?

@nagilson
Copy link
Member

We could absolutely make PublishSelfContained apps always RID Specific by default. And you're right, some of the publish properties already affect build. I don't really like the UX of it in this case though: I set PublishSelfContained, and suddenly my build output type has changed. It is unintuitive that the property would make my build RID Specific as well, IMO. I am unsure if allowing no-restore on a publish with PublishSelfContained is worth that cost/confusion.

@richlander
Copy link
Member Author

richlander commented Jul 26, 2023

I set PublishSelfContained, and suddenly my build output type has changed.

This is not the primary scenario to be concerned about. For example, native AOT templates come like this. Also, folks typically apply this property and then all the folks working on that project expect we'll deliver the optimal behavior from that point on.

no-restore is our guidance for building containers. Containers are very popular.

Let's do a though exercise using this example Dockerfile. It's not self-contained, but let's pretend it was.

https://github.com/dotnet/dotnet-docker/blob/33deb9b9df2abef0526ba97936d4c5462917b152/samples/dotnetapp/Dockerfile

Imagine you are regularly updating .cs files and rebuilding your container image. That means that result of line 11 will be cached and lines 11 and and 12 will be regularly re-executed. That's described as the dev scenario although CI might not be much different.

Important UX question:

  • Will the runtime pack be cached with line 9? or
  • Will the runtime pack be repeatedly downloaded as part of line 12?

@nagilson
Copy link
Member

Do you mean if it was PublishSelfContained and the restore did not have UCR? If the restore on line 8 deletes any potential cached runtime pack downloaded by line 12, yeah, that is pretty bad... I think UCR would fix the problem on restore though. It is certainly additional complexity that kind of requires you to know how the SDK works, though.

@richlander
Copy link
Member Author

richlander commented Jul 27, 2023

Right. No --ucr.

It is the opposite.

Here is a workflow:

  1. Build container image.
  2. Edit .cs file.
  3. Build container image.
  4. Edit .csproj file.
  5. Build container image.
  6. Edit .cs file
  7. Build container image.

When you rebuild the container image, Docker is able to use an incremental build system, relying on a cache. Each instruction in the Dockerfile is a layer.

  • When you edit the .csproj file, you start rebuilding from line 7.
  • When you edit a .cs file, you start re-building from line 11.

That has implications on when the runtime pack needs to be downloaded. If restore could download the runtime pack, then it would only be redownloaded when the .csproj was modified.

My meta point is that forcing folks to specify a RID for restore with an app type that is RID specific is bad.

I think UCR would fix the problem on restore though. It is certainly additional complexity that kind of requires you to know how the SDK works

It is the opposite. Users should not need to specify a RID or think about any of this. The SDK should build features to work well with popular systems.

@richlander
Copy link
Member Author

Creating a new issue.

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

No branches or pull requests

2 participants