Meaningful error on runtime identifier mismatch in project.assets.json

[BogdanYarotsky]

@nagilson
Resolves #30199

Since the error appeared in the ResolvePackageAssets task of Microsoft.PackageDependencyResolution.targets I have added RID validation before it. CheckRuntimeIdentifier task handles the situations when there is a mismatch between the runtime identifier of the project and project.assets.json targets. More descriptive build error is now shown when dotnet clean is invoked.

Here is a screenshot of the updated error message (and current workflow):

[BogdanYarotsky]

@dotnet-policy-service agree

[nagilson]

Thank you so much for your contribution, I will take a look at this within the next few business days :) I saw the test failures and was going to ask you to what you just did first, but looks like you're on top of it. I'm excited to take a look and we all appreciate your work 🔥

[BogdanYarotsky]

thanks @nagilson!
I hope that I took the steps in the right direction :)

[nagilson]

A few suggestions are made below. One more test also still needs investigation. Thank you for your hard work and following up. Please let me know if there's any questions :)

Once this is responded to, I can take another dive into it.

[nagilson]

We typically put assets like these in the src/Assets folders. I would load them there instead of using a hard-coded string, this way it is easier to reproduce having an actual file on disk ;)

[BogdanYarotsky]

Hi @nagilson,

I initially used the GivenAResolvePackageAssetsTask test file as a reference since it contained some hard-coded asset JSON within the test class itself.

I attempted to move these JSONs to src/Assets and use _testAssetsManager.CopyTestAsset(). However, I encountered an issue where the obj and bin folders weren't copied over for some reason; they appear to be omitted by _testAssetsManager.

From my understanding, the alternative approach would be to set up a clean project within src/Assets and modify it as needed in each individual test case (e.g., restore, add runtime identifier, run task).

One challenge I'm facing is to find a place for this integration test. Microsoft.NET.Build.Tasks.UnitTests doesn't seem like the most suitable project as it focuses mainly on tasks, making it too narrow for this use case. On the other hand, incorporating this directly into the dotnet CLI tests feels too broad, and I'm uncertain about how to validate error codes in that context.

Do you have any suggestions on where this test might best fit? I've retained the hard-coded JSON for now until I find a better spot for it.

Thank you for your time and guidance 😃

[nagilson]

I see, thanks for your thorough explanation. You're right in that it'd probably be easiest to make one test project and modify it there. \src\Tests\Microsoft.NET.Build.Tests\GlobalPropertyFlowTests.cs has a great example of our library code we have to edit project files to have a RuntimeIdentifier or whatever else.

For example, you could use this code:

            _testProject = new TestProject()
            {
                TargetFrameworks = ToolsetInfo.CurrentTargetFramework,
            };
            
            var testAsset = _testAssetsManager.CreateTestProject(_testProject);

Then run the dotnetBuild command, then set the property of the project afterward and try to run restore? I think that should work.

Such as

          var properties = _testProject.GetPropertyValues(testAsset.TestRoot, targetFramework: ToolsetInfo.CurrentTargetFramework);

              properties["RuntimeIdentifier"] = "foo";
          

I think you have found the best fit place for this test :) Since that's where the task is, it makes sense that its tests would be held in a mirror location in the test folder structure. I wouldn't put the test project in the root folder called test, I would put it in the TestProjects folder in the Assets folder if you still need it, but likely you don't. Another thing that we'd avoid is including unnecessary information in the runtime assets json file and include only the things necessary for the test (if the data is needed to make it run, then void it out to empty string etc.)

[nagilson]

Overall the change looks great, thank you for sticking through! 😄 Just some nits about the tests to match our existing codebase a bit better.

@ocalles May you also see if you agree with this change, since you seem to know this area very well? Thanks. :) If not, that's cool too.

[nagilson]

Suggested change

	Condition="'$(DesignTimeBuild)' != 'true' And Exists('$(ProjectAssetsFile)') And '$(RuntimeIdentifier)' != '' And '$(DOTNET_CLI_DISABLE_RUNTIME_ASSETS_RESTORE_CHECK)'!='true'"
	Condition="'$(DesignTimeBuild)' != 'true' And Exists('$(ProjectAssetsFile)') And '$(RuntimeIdentifier)' != '' And '$(DOTNET_CLI_DISABLE_RUNTIME_ASSETS_RESTORE_CHECK)' != 'true'"

[nagilson]

nit: whitespace

[nagilson]

Typically I would request not using var but all of the code here already does it, so you're matching the style and that's cool :)

[nagilson]

Impressive catch on not running the check during design time builds! :)

[nagilson]

I see, thanks for your thorough explanation. You're right in that it'd probably be easiest to make one test project and modify it there. \src\Tests\Microsoft.NET.Build.Tests\GlobalPropertyFlowTests.cs has a great example of our library code we have to edit project files to have a RuntimeIdentifier or whatever else.

For example, you could use this code:

            _testProject = new TestProject()
            {
                TargetFrameworks = ToolsetInfo.CurrentTargetFramework,
            };
            
            var testAsset = _testAssetsManager.CreateTestProject(_testProject);

Then run the dotnetBuild command, then set the property of the project afterward and try to run restore? I think that should work.

Such as

          var properties = _testProject.GetPropertyValues(testAsset.TestRoot, targetFramework: ToolsetInfo.CurrentTargetFramework);

              properties["RuntimeIdentifier"] = "foo";
          

I think you have found the best fit place for this test :) Since that's where the task is, it makes sense that its tests would be held in a mirror location in the test folder structure. I wouldn't put the test project in the root folder called test, I would put it in the TestProjects folder in the Assets folder if you still need it, but likely you don't. Another thing that we'd avoid is including unnecessary information in the runtime assets json file and include only the things necessary for the test (if the data is needed to make it run, then void it out to empty string etc.)

[nagilson]

@dotnet/dotnet-cli Heya folks, I've reviewed this PR pretty thoroughly from the community contributor @BogdanYarotsky, but it's always good to have a 2nd look if anyone has time. If not, that's cool too; I think it looks pretty good.

[dsplaisted]

I'm not clear on how this changes the user experience. It seems like it's moving an error out of ResolvePackageAssets into a new CheckRuntimeIdentifier task and target. But isn't the error and error message pretty similar in either case?

On a technical level, this bypasses the caching that ResolvePackageAssets does. The assets file can get big for large projects, so there can be some overhead with reading it. The ResolvePackageAssets caches its output in an optimized format to avoid reading the assets file if its not necessary. If the CheckRuntimeIdentifier task has to load the assets file anyway, we lose the benefit of that optimization.

[BogdanYarotsky]

@dsplaisted thank you for the feedback. I agree that it doesn't change much but rather moves error handling from one place another. As I understood from the open issue the current error message didn't give the clear instructions to the user who reported this: #29859 (comment)

Since I'm not that familiar with the codebase I thought to extend it instead of modyifing existing functionality. But since ResolvePackageAssets is built with performance in mind - I think that changing the build error message when target RID not found - would be a simpler/better approach. (It's also tested which will reduce total amount of changes). What do you think about this solution?

[dsplaisted]

What do you think about this solution?

That sounds fine. The current error message is generated in LockFileExtensions.GetTargetAndThrowIfNotFound, which is mostly used by ResolvePackageAssets but also in a few other places. I think that would be the place to make the change, either by changing the existing error messages, or adding some logic to add a new error message (for example, if the TargetFramework was found in the assets file but the RuntimeIdentifier wasn't).

Thanks!

[nagilson]

adding some logic to add a new error message (for example, if the TargetFramework was found in the assets file but the RuntimeIdentifier wasn't).

I agree with this suggestion, I'm not sure if the error had changed since writing the issue, but it does seem to be fairly good. It could be improved, however, with Danie's suggestion to be more specific (either if the TFM is missing or the RID.) I'm sorry to have you spend time and have to change the approach, that is on me for not writing up the issue clearly.

[BogdanYarotsky]

@nagilson don't worry, it's all good :) Thank you and @dsplaisted for coming back with the feedback!
I'll give it another shot a bit later with this info in mind.

[BogdanYarotsky]

Hi @nagilson, sorry for long time without a response. I have re-examined all the information from the original issue and comments in this PR. After spending some time with it I think that there is no technical problem here. The existing error message clearly says that restore needs to run before invoking dotnet clean and runtime identifier must be set in csproj. Doing so fixes the issue. Even the original poster of #29859 said that adding RID helped. And this was exactly what an error NETSDK1112 suggested to him:
The runtime pack for Microsoft.NETCore.App.Runtime.win-x64 was not downloaded. Try running a NuGet restore with the RuntimeIdentifier 'win-x64'. [C:\sc\ws1\YOV Services\Account\Account.EF\Account.EF.csproj]

The only problem that I see here is that adding a runtime identifier means that dotnet clean will never clean the artifacts from builds before a runtime identifier was added. So your proposal to add dotnet clean --all could be used for this. But it's a whole different thing to tackle.

In any case, I'll close this PR as it's way too behind the current main.
Thank you for your time!