code review feedback

src/Microsoft.DotNet.Cli.Utils/AnsiColorExtensions.cs

@@ -1,6 +1,6 @@
 namespace Microsoft.DotNet.Cli.Utils
 {
-    public static class AnsiExtensions
+    public static class AnsiColorExtensions
     {
         public static string Black(this string text)
         {

src/Microsoft.DotNet.Cli.Utils/Command.cs

@@ -63,11 +63,9 @@ namespace Microsoft.DotNet.Cli.Utils
                 // args is empty, we just run "foo.exe"
                 if (!string.IsNullOrEmpty(args))
                 {
-                    args = " " + args;
+                    executable = (executable + " " + args).Replace("\"", "\\\"");
                 }
-                var cmd = executable + args;
+                args = $"/C \"{executable}\"";
-                cmd = cmd.Replace("\"", "\\\"");
-                args = $"/C \"{executable}{args}\"";
                 executable = comSpec;
             }
 

src/Microsoft.DotNet.Cli.Utils/Constants.cs

@@ -11,5 +11,6 @@ namespace Microsoft.DotNet.Cli.Utils
         public static readonly string ExeSuffix = ".exe";
         public static readonly string CoreConsoleName = "coreconsole" + ExeSuffix;
         public static readonly string DefaultConfiguration = "Debug";
+        public static readonly string BinDirectoryName = "bin";
     }
 }

src/Microsoft.DotNet.Cli.Utils/DebugHelper.cs

@@ -20,9 +20,9 @@ namespace Microsoft.DotNet.Cli.Utils
 
         private static void WaitForDebugger()
         {
-            Console.WriteLine("Waiting for debugger to attach, or press Ctrl-C to terminate");
+            Console.WriteLine("Waiting for debugger to attach. Press ENTER to continue");
             Console.WriteLine($"Process ID: {Process.GetCurrentProcess().Id}");
-            while (!Debugger.IsAttached) { }
+            Console.ReadLine();
         }
     }
 }

src/Microsoft.DotNet.Tools.Compiler/Program.cs

@@ -102,8 +102,8 @@ namespace Microsoft.DotNet.Tools.Compiler
             if (string.IsNullOrEmpty(outputPath))
             {
                 outputPath = Path.Combine(
-                    context.Project.ProjectDirectory,
+                    context.ProjectFile.ProjectDirectory,
-                    "bin",
+                    Constants.BinDirectoryName,
                     configuration,
                     context.TargetFramework.GetTwoDigitShortFolderName());
             }
@@ -113,15 +113,15 @@ namespace Microsoft.DotNet.Tools.Compiler
             }
 
             // Get compilation options
-            var compilationOptions = context.Project.GetCompilerOptions(context.TargetFramework, configuration);
+            var compilationOptions = context.ProjectFile.GetCompilerOptions(context.TargetFramework, configuration);
-            var outputName = Path.Combine(outputPath, context.Project.Name + ".dll");
+            var outputName = Path.Combine(outputPath, context.ProjectFile.Name + ".dll");
 
             // Assemble csc args
             var cscArgs = new List<string>()
             {
                 "-nostdlib",
                 "-nologo",
-                $"-out:{outputName}"
+                $"-out:\"{outputName}\""
             };
 
             // Add compilation options to the args
@@ -129,19 +129,19 @@ namespace Microsoft.DotNet.Tools.Compiler
 
             foreach (var dependency in dependencies)
             {
-                cscArgs.AddRange(dependency.CompilationAssemblies.Select(r => $"-r:{r}"));
+                cscArgs.AddRange(dependency.CompilationAssemblies.Select(r => $"-r:\"{r}\""));
                 cscArgs.AddRange(dependency.SourceReferences);
             }
 
             // Add project source files
-            cscArgs.AddRange(context.Project.Files.SourceFiles);
+            cscArgs.AddRange(context.ProjectFile.Files.SourceFiles);
 
             // Write RSP file
-            var rsp = Path.Combine(outputPath, "csc.rsp");
+            var rsp = Path.Combine(outputPath, "dotnet-compile.csc.rsp");
             File.WriteAllLines(rsp, cscArgs);
 
             // Execute CSC!
-            var result = Command.Create("csc", $"-noconfig @{rsp}")
+            var result = Command.Create("csc", $"-noconfig @\"{rsp}\"")
                 .ForwardStdErr()
                 .ForwardStdOut()
                 .RunAsync()
@@ -151,14 +151,14 @@ namespace Microsoft.DotNet.Tools.Compiler
 
         private static void ApplyCompilationOptions(CompilerOptions compilationOptions, List<string> cscArgs)
         {
-            var targetType = (compilationOptions.EmitEntryPoint ?? false) ? "exe" : "library";
+            var targetType = compilationOptions.EmitEntryPoint.GetValueOrDefault() ? "exe" : "library";
             cscArgs.Add($"-target:{targetType}");
-            if (compilationOptions.AllowUnsafe == true)
+            if (compilationOptions.AllowUnsafe.GetValueOrDefault())
             {
                 cscArgs.Add("-unsafe+");
             }
             cscArgs.AddRange(compilationOptions.Defines.Select(d => $"-d:{d}"));
-            if (compilationOptions.Optimize == true)
+            if (compilationOptions.Optimize.GetValueOrDefault())
             {
                 cscArgs.Add("-optimize");
             }
@@ -166,7 +166,7 @@ namespace Microsoft.DotNet.Tools.Compiler
             {
                 cscArgs.Add($"-platform:{compilationOptions.Platform}");
             }
-            if (compilationOptions.WarningsAsErrors == true)
+            if (compilationOptions.WarningsAsErrors.GetValueOrDefault())
             {
                 cscArgs.Add("-warnaserror");
             }

src/Microsoft.DotNet.Tools.Compiler/Reporter.cs

@@ -1,9 +1,4 @@
-using System;
+using System.Runtime.InteropServices;
-using System.Collections.Generic;
-using System.IO;
-using System.Linq;
-using System.Runtime.InteropServices;
-using System.Threading.Tasks;
 using Microsoft.Dnx.Runtime.Common.CommandLine;
 
 namespace Microsoft.DotNet.Cli.Utils

src/Microsoft.DotNet.Tools.Publish/Program.cs

@@ -29,8 +29,14 @@ namespace Microsoft.DotNet.Tools.Publish
 
             app.OnExecute(() =>
             {
-                CheckArg(framework);
+                if(!CheckArg(framework))
-                CheckArg(runtime);
+                {
+                    return 1;
+                }
+                if(!CheckArg(runtime))
+                {
+                    return 1;
+                }
 
                 // Locate the project and get the name and full path
                 var path = project.Value;
@@ -40,7 +46,9 @@ namespace Microsoft.DotNet.Tools.Publish
                 }
 
                 // Load project context and publish it
-                var context = ProjectContext.Create(path, NuGetFramework.Parse(framework.Value()), new[] { runtime.Value() });
+                var fx = NuGetFramework.Parse(framework.Value());
+                var rids = new[] { runtime.Value() };
+                var context = ProjectContext.Create(path, fx, rids);
                 return Publish(context, output.Value(), configuration.Value() ?? Constants.DefaultConfiguration);
             });
 
@@ -55,13 +63,14 @@ namespace Microsoft.DotNet.Tools.Publish
             }
         }
 
-        private static void CheckArg(CommandOption argument)
+        private static bool CheckArg(CommandOption argument)
         {
             if (!argument.HasValue())
             {
-                // TODO: GROOOOOOSS
+                Reporter.Error.WriteLine($"Missing required argument: {argument.LongName.Red().Bold()}");
-                throw new OperationCanceledException($"Missing required argument: {argument.LongName}");
+                return false;
             }
+            return true;
         }
 
         private static int Publish(ProjectContext context, string outputPath, string configuration)
@@ -72,8 +81,8 @@ namespace Microsoft.DotNet.Tools.Publish
             if (string.IsNullOrEmpty(outputPath))
             {
                 outputPath = Path.Combine(
-                    context.Project.ProjectDirectory,
+                    context.ProjectFile.ProjectDirectory,
-                    "bin",
+                    Constants.BinDirectoryName,
                     configuration,
                     context.TargetFramework.GetTwoDigitShortFolderName(),
                     "publish");
@@ -84,14 +93,14 @@ namespace Microsoft.DotNet.Tools.Publish
             }
 
             // Compile the project (and transitively, all it's dependencies)
-            var result = Command.Create("dotnet-compile", $"--framework {context.TargetFramework.DotNetFrameworkName} {context.Project.ProjectDirectory}")
+            var result = Command.Create("dotnet-compile", $"--framework {context.TargetFramework.DotNetFrameworkName} {context.ProjectFile.ProjectDirectory}")
                 .ForwardStdErr()
                 .ForwardStdOut()
                 .RunAsync()
                 .Result;
             if (result.ExitCode != 0)
             {
-                Console.Error.WriteLine("Compilation failed!");
+                Reporter.Error.WriteLine("Compilation failed!".Red().Bold());
                 return result.ExitCode;
             }
 
@@ -111,7 +120,7 @@ namespace Microsoft.DotNet.Tools.Publish
             var coreConsole = Path.Combine(outputPath, Constants.CoreConsoleName);
             if (!File.Exists(coreConsole))
             {
-                Console.Error.WriteLine($"Unable to find {Constants.CoreConsoleName} at {coreConsole}. You must have an explicit dependency on Microsoft.NETCore.ConsoleHost (for now ;))");
+                Reporter.Error.WriteLine($"Unable to find {Constants.CoreConsoleName} at {coreConsole}. You must have an explicit dependency on Microsoft.NETCore.ConsoleHost (for now ;))".Red().Bold());
                 return 1;
             }
 
@@ -119,22 +128,22 @@ namespace Microsoft.DotNet.Tools.Publish
             string overrideCoreConsole = Environment.GetEnvironmentVariable("DOTNET_CORE_CONSOLE_PATH");
             if(!string.IsNullOrEmpty(overrideCoreConsole) && File.Exists(overrideCoreConsole))
             {
-                Console.WriteLine($"Using CoreConsole override: {overrideCoreConsole}");
+                Reporter.Output.WriteLine($"Using CoreConsole override: {overrideCoreConsole}");
                 File.Copy(overrideCoreConsole, coreConsole, overwrite: true);
             }
 
             // Use the 'command' field to generate the name
-            var outputExe = Path.Combine(outputPath, context.Project.Name + ".exe");
+            var outputExe = Path.Combine(outputPath, context.ProjectFile.Name + ".exe");
-            if (File.Exists(outputExe))
+            File.Copy(coreConsole, outputExe, overwrite: true);
+            if (File.Exists(coreConsole))
             {
-                File.Delete(outputExe);
+                File.Delete(coreConsole);
             }
-            File.Move(coreConsole, outputExe);
 
             // Check if the a command name is specified, and rename the necessary files
-            if(context.Project.Commands.Count == 1)
+            if(context.ProjectFile.Commands.Count == 1)
             {
-                var commandName = context.Project.Commands.Single().Key;
+                var commandName = context.ProjectFile.Commands.Single().Key;
 
                 // Move coreconsole and the matching dll
                 var renamedExe = Path.Combine(outputPath, commandName + ".exe");
@@ -148,7 +157,7 @@ namespace Microsoft.DotNet.Tools.Publish
                 outputExe = Path.Combine(outputPath, commandName + ".exe");
             }
 
-            Console.Error.WriteLine($"Published to {outputExe}");
+            Reporter.Output.WriteLine($"Published to {outputExe}".Green().Bold());
             return 0;
         }
 

src/Microsoft.DotNet.Tools.Publish/Reporter.cs

@@ -1,9 +1,4 @@
-using System;
+using System.Runtime.InteropServices;
-using System.Collections.Generic;
-using System.IO;
-using System.Linq;
-using System.Runtime.InteropServices;
-using System.Threading.Tasks;
 using Microsoft.Dnx.Runtime.Common.CommandLine;
 
 namespace Microsoft.DotNet.Cli.Utils

src/Microsoft.Extensions.ProjectModel/Compilation/LibraryExporter.cs

@@ -92,21 +92,6 @@ namespace Microsoft.Extensions.ProjectModel.Compilation
             }
         }
 
-        private void QueueDependencies(Queue<Node> queue, Node node)
-        {
-            // Queue up all the dependencies to be exported
-            foreach (var dependency in node.Library.Dependencies)
-            {
-                var childNode = new Node
-                {
-                    Library = LibraryManager.GetLibrary(dependency.Name),
-                    Parent = node
-                };
-
-                queue.Enqueue(childNode);
-            }
-        }
-
         private LibraryExport GetExport(LibraryDescription library)
         {
             // Don't even try to export unresolved libraries
@@ -184,7 +169,7 @@ namespace Microsoft.Extensions.ProjectModel.Compilation
         {
             return Path.Combine(
                 project.Project.ProjectDirectory,
-                "bin",
+                "bin", // This can't access the Constant in Cli Utils. But the output path stuff is temporary right now anyway
                 _configuration,
                 project.Framework.GetTwoDigitShortFolderName(),
                 project.Project.Name + ".dll");
@@ -246,12 +231,5 @@ namespace Microsoft.Extensions.ProjectModel.Compilation
                 paths.Add(path);
             }
         }
-
-        private class Node
-        {
-            public LibraryDescription Library { get; set; }
-
-            public Node Parent { get; set; }
-        }
     }
 }

src/Microsoft.Extensions.ProjectModel/Graph/LibraryIdentity.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.Extensions.Internal;
 using NuGet;
 using NuGet.Versioning;
 
@@ -33,8 +34,6 @@ namespace Microsoft.Extensions.ProjectModel.Graph
 
         public bool Equals(LibraryIdentity other)
         {
-            if (ReferenceEquals(null, other)) return false;
-            if (ReferenceEquals(this, other)) return true;
             return string.Equals(Name, other.Name) &&
                 Equals(Version, other.Version) &&
                 Equals(Type, other.Type);
@@ -42,20 +41,16 @@ namespace Microsoft.Extensions.ProjectModel.Graph
 
         public override bool Equals(object obj)
         {
-            if (ReferenceEquals(null, obj)) return false;
+            return obj is LibraryIdentity && Equals((LibraryIdentity)obj);
-            if (ReferenceEquals(this, obj)) return true;
-            if (obj.GetType() != this.GetType()) return false;
-            return Equals((LibraryIdentity)obj);
         }
 
         public override int GetHashCode()
         {
-            unchecked
+            var combiner = HashCodeCombiner.Start();
-            {
+            combiner.Add(Name);
-                return ((Name != null ? Name.GetHashCode() : 0) * 397) ^
+            combiner.Add(Version);
-                    (Version != null ? Version.GetHashCode() : 0) ^
+            combiner.Add(Type);
-                    (Type.GetHashCode());
+            return combiner.CombinedHash;
-            }
         }
 
         public static bool operator ==(LibraryIdentity left, LibraryIdentity right)

src/Microsoft.Extensions.ProjectModel/Graph/LockFileReader.cs

@@ -5,7 +5,6 @@ using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
-using System.Runtime.Versioning;
 using Microsoft.Extensions.ProjectModel.Utilities;
 using Newtonsoft.Json;
 using Newtonsoft.Json.Linq;
@@ -20,7 +19,7 @@ namespace Microsoft.Extensions.ProjectModel.Graph
     {
         public static LockFile Read(string path)
         {
-            using(var fs = FileSystemUtility.OpenFileStream(path))
+            using(var fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read))
             {
                 return Read(fs);
             }

src/Microsoft.Extensions.ProjectModel/ProjectContext.cs

@@ -16,20 +16,20 @@ namespace Microsoft.Extensions.ProjectModel
     // NOTE(anurse): Copied from ApplicationHostContext in DNX. This name seemed more appropriate for this :)
     public class ProjectContext
     {
-        public Project Project { get; }
 
         public GlobalSettings GlobalSettings { get; }
 
-        // TODO: Remove this, it's kinda hacky
         public ProjectDescription RootProject { get; }
 
         public NuGetFramework TargetFramework { get; }
 
         public string RuntimeIdentifier { get; }
 
+        public Project ProjectFile => RootProject.Project;
+
         public string RootDirectory => GlobalSettings.DirectoryPath;
 
-        public string ProjectDirectory => Project.ProjectDirectory;
+        public string ProjectDirectory => ProjectFile.ProjectDirectory;
 
         public string PackagesDirectory { get; }
 
@@ -38,7 +38,6 @@ namespace Microsoft.Extensions.ProjectModel
         public LibraryManager LibraryManager { get; }
 
         internal ProjectContext(
-            Project project,
             GlobalSettings globalSettings,
             ProjectDescription rootProject,
             NuGetFramework targetFramework,
@@ -47,7 +46,6 @@ namespace Microsoft.Extensions.ProjectModel
             FrameworkReferenceResolver frameworkResolver,
             LibraryManager libraryManager)
         {
-            Project = project;
             GlobalSettings = globalSettings;
             RootProject = rootProject;
             TargetFramework = targetFramework;

src/Microsoft.Extensions.ProjectModel/ProjectContextBuilder.cs

@@ -95,7 +95,6 @@ namespace Microsoft.Extensions.ProjectModel
             AddLockFileDiagnostics(libraryManager, LockFile != null, validLockFile, lockFileValidationMessage);
 
             return new ProjectContext(
-                Project,
                 GlobalSettings,
                 mainProject,
                 TargetFramework,

src/Microsoft.Extensions.ProjectModel/ProjectReader.cs

@@ -267,18 +267,6 @@ namespace Microsoft.Extensions.ProjectModel
                         projectPath,
                         ((IJsonLineInfo)dependencyKey.Value).LineNumber,
                         ((IJsonLineInfo)dependencyKey.Value).LinePosition));
-
-                    //{
-                    //    LibraryRange = new LibraryRange(dependencyKey, isGacOrFrameworkReference)
-                    //    {
-                    //        VersionRange = dependencyVersionRange,
-                    //        FileName = projectPath,
-                    //        Line = dependencyValue.Line,
-                    //        Column = dependencyValue.Column,
-                    //        Target = target
-                    //    },
-                    //    Type = dependencyTypeValue
-                    //});
                 }
             }
         }

src/Microsoft.Extensions.ProjectModel/Resolution/PackageDependencyProvider.cs

@@ -14,13 +14,11 @@ namespace Microsoft.Extensions.ProjectModel.Resolution
     {
         private readonly string _packagesPath;
 
-        private readonly IEnumerable<VersionFolderPathResolver> _cacheResolvers;
         private readonly VersionFolderPathResolver _packagePathResolver;
 
         public PackageDependencyProvider(string packagesPath)
         {
             _packagesPath = packagesPath;
-            _cacheResolvers = GetCacheResolvers();
             _packagePathResolver = new VersionFolderPathResolver(packagesPath);
         }
 
@@ -41,7 +39,7 @@ namespace Microsoft.Extensions.ProjectModel.Resolution
             var dependencies = new List<LibraryRange>(targetLibrary.Dependencies.Count + targetLibrary.FrameworkAssemblies.Count);
             PopulateDependencies(dependencies, targetLibrary);
 
-            var path = ResolvePackagePath(package);
+            var path = _packagePathResolver.GetInstallPath(package.Name, package.Version);
 
             var packageDescription = new PackageDescription(
                 new LibraryRange(package.Name, new VersionRange(package.Version), LibraryType.Package, LibraryDependencyType.Default),
@@ -74,25 +72,6 @@ namespace Microsoft.Extensions.ProjectModel.Resolution
             }
         }
 
-        private string ResolvePackagePath(LockFilePackageLibrary package)
-        {
-            string expectedHash = package.Sha512;
-
-            foreach (var resolver in _cacheResolvers)
-            {
-                var cacheHashFile = resolver.GetHashPath(package.Name, package.Version);
-
-                // REVIEW: More efficient compare?
-                if (File.Exists(cacheHashFile) &&
-                    File.ReadAllText(cacheHashFile) == expectedHash)
-                {
-                    return resolver.GetInstallPath(package.Name, package.Version);
-                }
-            }
-
-            return _packagePathResolver.GetInstallPath(package.Name, package.Version);
-        }
-
         public static string ResolveRepositoryPath(string rootDirectory, GlobalSettings settings)
         {
             // Order
@@ -136,29 +115,5 @@ namespace Microsoft.Extensions.ProjectModel.Resolution
             return packageCachePathValue.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries)
                                         .Select(path => new VersionFolderPathResolver(path));
         }
-
-        private class AssemblyNameComparer : IEqualityComparer<AssemblyName>
-        {
-            public static IEqualityComparer<AssemblyName> OrdinalIgnoreCase = new AssemblyNameComparer();
-
-            public bool Equals(AssemblyName x, AssemblyName y)
-            {
-                return
-                    string.Equals(x.Name, y.Name, StringComparison.OrdinalIgnoreCase) &&
-                    string.Equals(x.CultureName ?? "", y.CultureName ?? "", StringComparison.OrdinalIgnoreCase);
-            }
-
-            public int GetHashCode(AssemblyName obj)
-            {
-                var hashCode = 0;
-                if (obj.Name != null)
-                {
-                    hashCode ^= obj.Name.ToUpperInvariant().GetHashCode();
-                }
-
-                hashCode ^= (obj.CultureName?.ToUpperInvariant() ?? "").GetHashCode();
-                return hashCode;
-            }
-        }
     }
 }

src/Microsoft.Extensions.ProjectModel/Utilities/FileSystemUtility.cs

@@ -1,39 +0,0 @@
-using System;
-using System.IO;
-using System.Threading;
-using System.Threading.Tasks;
-
-namespace Microsoft.Extensions.ProjectModel.Utilities
-{
-    internal static class FileSystemUtility
-    {
-        internal static FileStream OpenFileStream(string filePath)
-        {
-            // Retry 3 times before re-throw the exception.
-            // It mitigates the race condition when DTH read lock file while VS is restoring projects.
-
-            int retry = 3;
-            while (true)
-            {
-                try
-                {
-                    return new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read);
-                }
-                catch (Exception)
-                {
-                    if (retry > 0)
-                    {
-                        retry--;
-                        Thread.Sleep(1000);
-                    }
-                    else
-                    {
-                        throw;
-                    }
-                }
-            }
-
-        }
-
-    }
-}

src/Microsoft.Extensions.ProjectModel/Utilities/VersionUtility.cs

@@ -8,11 +8,7 @@ namespace Microsoft.Extensions.ProjectModel.Utilities
     {
         public static NuGetVersion GetAssemblyVersion(string path)
         {
-//#if NET451
-//            return new NuGetVersion(AssemblyName.GetAssemblyName(path).Version);
-//#else
             return new NuGetVersion(AssemblyLoadContext.GetAssemblyName(path).Version);
-//#endif
         }
     }
 }