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

Deps.json should include project references that aren't present in project.assets.json #28892

Closed
wants to merge 11 commits into from
Closed

Conversation

wrall
Copy link
Contributor

@wrall wrall commented Nov 3, 2022

Addresses #28891.

Although most projects are included in project.assets.json after a restore takes place, there are some (rare) scenarios when this is not the case. When that happens, project references end up being skipped and excluded from deps.json.

One can work around this by adding a binary , that is inconvenient and can lead to issues when one forgets. Instead, it would be good to check whether the project reference is included in project.assets.json before skipping it when generating the list of dependencies.

Also added a functional test (.NET 6.0 project referencing a .NET Framework project) that failed previously and succeeds now.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch for doing this!

I think the test that disables project restore is failing because then the Microsoft.NETFramework.ReferenceAssemblies PackageReference isn't restored and the targeting pack isn't installed. I'd suggest changing the projects to target .NET Framework 4.8, since those reference assemblies are likely to be installed by Visual Studio. I think you'll also need to switch to WindowsOnlyFact too.

Right now the PR is targeting main, which means it will go into the .NET 8 SDK release. You could retarget the PR to release/7.0.2xx if you would like the fix to go in the next feature release of the SDK (which I think corresponds to Visual Studio 17.5).

Also, I'd suggest switching to using the TestProject APIs that we have for creating projects for tests. That way it's more obvious in the test itself what the tested projects are, rather than having that information in a bunch of separate files you have to find and buried in a bunch of boilerplate for non SDK-style projects. Here's what your tests could look like rewritten like that:

// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
//

using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.NET.TestFramework;
using Microsoft.NET.TestFramework.Assertions;
using Microsoft.NET.TestFramework.Commands;
using Xunit;
using FluentAssertions;
using System.Xml.Linq;
using System.Linq;
using System;
using Xunit.Abstractions;
using Microsoft.Extensions.DependencyModel;
using Microsoft.NET.TestFramework.ProjectConstruction;

namespace Microsoft.NET.Build.Tests
{
    public class GivenThatWeWantToBuildAnAppWithTransitiveNonSdkProjectRefs : SdkTest
    {
        public GivenThatWeWantToBuildAnAppWithTransitiveNonSdkProjectRefs(ITestOutputHelper log) : base(log)
        {
        }

        TestProject CreateTestProject()
        {
            string targetFrameworkVersion = "v4.8";

            var auxLibraryProject = new TestProject("AuxLibrary")
            {
                IsSdkProject = false,
                TargetFrameworkVersion = targetFrameworkVersion
            };
            auxLibraryProject.SourceFiles["Helper.cs"] = """
                using System;

                namespace AuxLibrary
                {
                    public static class Helper
                    {
                        public static void WriteMessage()
                        {
                            Console.WriteLine("This string came from AuxLibrary!");
                        }
                    }
                }
                """;

            var mainLibraryProject = new TestProject("MainLibrary")
            {
                IsSdkProject = false,
                TargetFrameworkVersion = targetFrameworkVersion
            };
            mainLibraryProject.ReferencedProjects.Add(auxLibraryProject);
            mainLibraryProject.SourceFiles["Helper.cs"] = """
                using System;

                namespace MainLibrary
                {
                    public static class Helper
                    {
                        public static void WriteMessage()
                        {
                            Console.WriteLine("This string came from MainLibrary!");
                            AuxLibrary.Helper.WriteMessage();
                        }
                    }
                }
                """;

            var testAppProject = new TestProject("TestApp")
            {
                IsExe = true,
                TargetFrameworks = ToolsetInfo.CurrentTargetFramework
            };
            testAppProject.AdditionalProperties["ProduceReferenceAssembly"] = "false";
            testAppProject.ReferencedProjects.Add(mainLibraryProject);
            testAppProject.SourceFiles["Program.cs"] = """
                using System;

                namespace TestApp
                {
                    public class Program
                    {
                        public static void Main(string[] args)
                        {
                            Console.WriteLine("TestApp --depends on--> MainLibrary --depends on--> AuxLibrary");
                            MainLibrary.Helper.WriteMessage();
                        }
                    }
                }
                """;

            return testAppProject;
        }

        [WindowsOnlyFact]
        public void It_builds_the_project_successfully()
        {
            // NOTE the project dependencies in AppWithTransitiveNonSdkProjectRefs:
            // TestApp --depends on--> MainLibrary --depends on--> AuxLibrary (non-SDK)
            // (TestApp transitively depends on AuxLibrary)
            var testAsset = _testAssetsManager.CreateTestProject(CreateTestProject());

            VerifyAppBuilds(testAsset);
        }

        [WindowsOnlyFact]
        public void It_builds_deps_correctly_when_projects_do_not_get_restored()
        {
            // NOTE the project dependencies in AppWithTransitiveProjectRefs:
            // TestApp --depends on--> MainLibrary --depends on--> AuxLibrary
            // (TestApp transitively depends on AuxLibrary)
            var testAsset = _testAssetsManager.CreateTestProject(CreateTestProject())
                .WithProjectChanges(
                    (projectName, project) =>
                    {
                        if (StringComparer.OrdinalIgnoreCase.Equals(Path.GetFileNameWithoutExtension(projectName), "AuxLibrary") ||
                            StringComparer.OrdinalIgnoreCase.Equals(Path.GetFileNameWithoutExtension(projectName), "MainLibrary"))
                        {
                            var ns = project.Root.Name.Namespace;

                            // indicate that project restore is not supported for these projects:
                            var target = new XElement(ns + "Target",
                                new XAttribute("Name", "_IsProjectRestoreSupported"),
                                new XAttribute("Returns", "@(_ValidProjectsForRestore)"));

                            project.Root.Add(target);
                        }
                    });

            string outputDirectory = VerifyAppBuilds(testAsset);

            using (var depsJsonFileStream = File.OpenRead(Path.Combine(outputDirectory, "TestApp.deps.json")))
            {
                var dependencyContext = new DependencyContextJsonReader().Read(depsJsonFileStream);

                var projectNames = dependencyContext.RuntimeLibraries.Select(library => library.Name).ToList();
                projectNames.Should().BeEquivalentTo(new[] { "TestApp", "AuxLibrary", "MainLibrary" });
            }
        }

        private string VerifyAppBuilds(TestAsset testAsset)
        {
            var buildCommand = new BuildCommand(testAsset, "TestApp");
            var outputDirectory = buildCommand.GetOutputDirectory(ToolsetInfo.CurrentTargetFramework);

            buildCommand
                .Execute()
                .Should()
                .Pass();

            outputDirectory.Should().OnlyHaveFiles(new[] {
                "TestApp.dll",
                "TestApp.pdb",
                $"TestApp{EnvironmentInfo.ExecutableExtension}",
                "TestApp.deps.json",
                "TestApp.runtimeconfig.json",
                "MainLibrary.dll",
                "MainLibrary.pdb",
                "AuxLibrary.dll",
                "AuxLibrary.pdb",
            });

            new DotnetCommand(Log, Path.Combine(outputDirectory.FullName, "TestApp.dll"))
                .Execute()
                .Should()
                .Pass()
                .And
                .HaveStdOutContaining("This string came from MainLibrary!")
                .And
                .HaveStdOutContaining("This string came from AuxLibrary!");

            return outputDirectory.FullName;
        }
    }
}

@wrall
Copy link
Contributor Author

wrall commented Nov 4, 2022

Thanks a bunch for doing this!

I think the test that disables project restore is failing because then the Microsoft.NETFramework.ReferenceAssemblies PackageReference isn't restored and the targeting pack isn't installed. I'd suggest changing the projects to target .NET Framework 4.8, since those reference assemblies are likely to be installed by Visual Studio. I think you'll also need to switch to WindowsOnlyFact too.

Right now the PR is targeting main, which means it will go into the .NET 8 SDK release. You could retarget the PR to release/7.0.2xx if you would like the fix to go in the next feature release of the SDK (which I think corresponds to Visual Studio 17.5).

Also, I'd suggest switching to using the TestProject APIs that we have for creating projects for tests. That way it's more obvious in the test itself what the tested projects are, rather than having that information in a bunch of separate files you have to find and buried in a bunch of boilerplate for non SDK-style projects. Here's what your tests could look like rewritten like that:

I've updated the test as you recommended. Also, I think that I've fixed the project name not matching assembly name issue, but your expertise in helping to confirm this would be much appreciated!

Should we start by checking into main and then port it to release/7.0.2xx from there? Ultimately, it would be nice to have it in the .NET 6 SDK if possible.

@marcpopMSFT
Copy link
Member

Thanks for the contribution. 7.0.2xx is fully open for any change that isn't a breaking change and we have automated codeflow that takes changes to main. So you're welcome to make in main and backport or just target 7.0.2xx and let automated codeflow take it.

As for servicing, which version of the SDK are you using currently and expect to be using in the future? Even if you're targeting 6.0, you can use the 7.0 sdks for that. I'm a little hesitant to include this in servicing because it is a behavior change (ie files will be showing up in the deps.json that weren't there before). We usually end up breaking someone when we take behavior changes. Is it possible to make this something folks could opt into or at least opt-out with a project property? That would lower the risk.

@wrall wrall changed the base branch from main to release/7.0.2xx November 9, 2022 03:17
@wrall wrall changed the base branch from release/7.0.2xx to main November 9, 2022 03:18
@wrall wrall changed the base branch from main to release/7.0.2xx November 9, 2022 03:18
@wrall wrall changed the base branch from release/7.0.2xx to main November 9, 2022 03:18
@wrall
Copy link
Contributor Author

wrall commented Nov 9, 2022

Thanks for the contribution. 7.0.2xx is fully open for any change that isn't a breaking change and we have automated codeflow that takes changes to main. So you're welcome to make in main and backport or just target 7.0.2xx and let automated codeflow take it.

As for servicing, which version of the SDK are you using currently and expect to be using in the future? Even if you're targeting 6.0, you can use the 7.0 sdks for that. I'm a little hesitant to include this in servicing because it is a behavior change (ie files will be showing up in the deps.json that weren't there before). We usually end up breaking someone when we take behavior changes. Is it possible to make this something folks could opt into or at least opt-out with a project property? That would lower the risk.

Everything goes crazy when I try to rebase through the UI - I'll rebase and submit a new PR.

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

Successfully merging this pull request may close these issues.

3 participants