-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add SDK support for generating deps.json files for .NET CLI Tools #1128
Add SDK support for generating deps.json files for .NET CLI Tools #1128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests?
@eerhardt I was planning for tests to go in the CLI repo. |
So when we are fixing bugs or adding new features in this area, we won't be able to tell if we broke something until it gets merged into CLI? |
Condition=" '$(AdditionalImport)' != '' And Exists($(AdditionalImport))" /> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>netcoreapp$(BundledNETCoreAppTargetFrameworkVersion)</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This this always going to hold true? What if the project.assets.json file was restored with netcoreapp1.0
? Won't GenerateBuildDependencyFile
raise an error because there isn't a netcoreapp2.0
target in the lock file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe DotnetCliToolTargetFramework
should be passed in to this project? Or the TargetFramework
should be passed in?
In reply to: 111981249 [](ancestors = 111981249)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets passed in now, based on the target framework from the assets file.
@@ -86,6 +91,10 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
Inputs="$(ProjectAssetsFile)" | |||
Outputs="$(ProjectDepsFilePath)"> | |||
|
|||
<PropertyGroup> | |||
<IncludeMainProjectInDepsFile Condition=" '$(IncludeMainProjectInDepsFile)' == '' ">true</IncludeMainProjectInDepsFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating these property defaulting in two places, can we default it in a common spot?
} | ||
else | ||
{ | ||
fullProjectPath = Path.GetFullPath(projectPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic concerns me. I don't think this will work correctly if the project includes P2P references.
Maybe alternatively, we always create a mainProjectInfo
, and instead have a boolean on it - IncludeInDependencyContext
or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, a test would be nice here - have a project that sets <IncludeMainProjectInDepsFile>false</IncludeMainProjectInDepsFile>
and has a P2P reference.
I think this kind of scenario could be used in situations like the dotnet/cli uses. See https://github.com/dotnet/cli/blob/master/src/redist/redist.csproj#L62-L64 where we have to go and strip the "main project" from the deps file.
In reply to: 111982912 [](ancestors = 111982912)
I've created the corresponding PR for the CLI changes: dotnet/cli#6356
@eerhardt You're right, we should probably have some test coverage for this in the SDK repo too. I'm going to first work on adding tests for the end-to-end scenario in the CLI, then I'll come back and look at what kind of tests for this to add to the SDK. |
@MattGertz for approval. This is the other PR I mentioned to you earlier today. |
public static ICommand AddTestEnvironmentVariables(ICommand command) | ||
{ | ||
// Set NUGET_PACKAGES environment variable to match value from build.ps1 | ||
command = command.EnvironmentVariable("NUGET_PACKAGES", Path.Combine(RepoInfo.RepoRoot, "packages")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NuGetCachePath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that wouldn't hurt. This code was written before that variable existed, and I'm moving it to a different location here.
.And.HaveStdOutContaining("Successfully loaded runtime graph"); | ||
} | ||
|
||
// This method duplicates a lot of logic from the CLI in order to test generating deps files for tools in the SDK repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we need to go this far?
Could we get away with just testing the GenerateDeps.proj file without bringing a bunch of "tools" stuff. Just get a project.assets.json file, and invoke the GenerateDeps.proj file on it....
thoughts?
IsExe = true | ||
}; | ||
|
||
toolProject.PackageReferences.Add(new TestPackageReference("Microsoft.Extensions.DependencyModel", "2.0.0-preview1-002022", null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get this version from our DependencyVersions.props file? That way we are constantly updating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the point is to test conflict resolution, I think we want a fixed version so that we don't roll forward to a version of the library that doesn't need it (because it targets .NET Core / .NET Standard 2.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then let's use a released version, like 1.0.0
or 1.1.0
, etc. Not some random daily build.
…ET Core bundled in the CLI Also include GenerateDeps.proj in tasks project
…I to use to get the path to it in the SDK
44120b4
to
dc9139b
Compare
…122.5 (dotnet#1128) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19572.5
Fixes dotnet#1132 Fixes dotnet#1128 Fixes dotnet#1127 Fixes dotnet#1126 Fixes dotnet#1125 Fixes dotnet#1124 Fixes dotnet#1115
Fixes dotnet#1132 Fixes dotnet#1128 Fixes dotnet#1127 Fixes dotnet#1126 Fixes dotnet#1125 Fixes dotnet#1124 Fixes dotnet#1115
This is an SDK-side change which will be part of fixing https://github.com/dotnet/cli/issues/6087. There will be a corresponding CLI change which will take advantage of it. We should probably wait to merge this change until I have the CLI change ready, but I think this change can be reviewed as is.
The idea is that when the CLI is going to run a tool, it will check to see if there is a deps.json file next to the project.assets.json under the NuGet .tools folder. If not, it will generate the deps file by building the new GenerateDeps.proj file which is inside the CLI, passing in necessary information such as the path to the assets file.
This will allow the same logic to be used to generate the .deps.json files for tools as is used when apps are built. It will include the conflict resolution logic, which will fix #6087.