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.

Sign up for GitHub

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

Jump to bottom

GH1533: Updated DotNetCoreBuild inline with Tooling v1.0 #1550

Merged
merged 1 commit into from
Apr 15, 2017
Merged

GH1533: Updated DotNetCoreBuild inline with Tooling v1.0 #1550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Cake.Common.Tests/Unit/Tools/DotNetCore/Build/DotNetCoreBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using Cake.Common.Tests.Fixtures.Tools.DotNetCore.Build;
using Cake.Common.Tools.DotNetCore;
using Cake.Common.Tools.DotNetCore.Build;
using Cake.Testing;
using Xunit;
Expand Down Expand Up @@ -98,12 +99,13 @@ public void Should_Add_Additional_Arguments()
fixture.Settings.Configuration = "Release";
fixture.Settings.VersionSuffix = "rc1";
fixture.Project = "./src/*";
fixture.Settings.Verbosity = DotNetCoreVerbosity.Minimal;

// When
var result = fixture.Run();

// Then
Assert.Equal("build \"./src/*\" --runtime runtime1 --framework net451 --configuration Release --version-suffix rc1", result.Args);
Assert.Equal("build \"./src/*\" --runtime runtime1 --framework net451 --configuration Release --version-suffix rc1 --verbosity Minimal", result.Args);
}

[Fact]
Expand Down Expand Up @@ -143,8 +145,6 @@ public void Should_Add_Build_Arguments()
{
// Given
var fixture = new DotNetCoreBuilderFixture();
fixture.Settings.BuildBasePath = "./temp/";
fixture.Settings.BuildProfile = true;
fixture.Settings.NoIncremental = true;
fixture.Settings.NoDependencies = true;
fixture.Project = "./src/*";
Expand All @@ -153,7 +153,7 @@ public void Should_Add_Build_Arguments()
var result = fixture.Run();

// Then
Assert.Equal("build \"./src/*\" --build-base-path \"/Working/temp\" --build-profile --no-incremental --no-dependencies", result.Args);
Assert.Equal("build \"./src/*\" --no-incremental --no-dependencies", result.Args);
}

[Fact]
Expand Down
12 changes: 1 addition & 11 deletions src/Cake.Common/Tools/DotNetCore/Build/DotNetCoreBuildSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ namespace Cake.Common.Tools.DotNetCore.Build
/// </summary>
public sealed class DotNetCoreBuildSettings : DotNetCoreSettings
{
/// <summary>
/// Gets or sets the directory in which to place temporary outputs.
/// </summary>
public DirectoryPath BuildBasePath { get; set; }

/// <summary>
/// Gets or sets the output directory.
/// </summary>
Expand All @@ -41,11 +36,6 @@ public sealed class DotNetCoreBuildSettings : DotNetCoreSettings
/// </summary>
public string VersionSuffix { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

We should add a string Version { get; set; } too, equivalent to

 ArgumentCustomization = arg=>arg.Append(string.Concat("/p:Version=", semVersion)),

We should also add an

public IDictionary<string, IList<string>> Properties => _properties;

To support MSBuild properties without ArgumentCustomization, similar too MSBuildSettings.Properties

We should also add an

public ISet<string> Targets { get; }

Running dotnet msbuild -h looks like it support most MSBuild so future refactoring could be to see what we have common between the .NET Core CLI and MSBuild alias.

Copy link
Member

@devlead devlead Apr 3, 2017 •
edited

Choose a reason for hiding this comment

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

Maybe version could be something common, know dotnet pack accepts & should have it too (haven't tested all yet but probably publish too).

Copy link
Member

Choose a reason for hiding this comment

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

@devlead Should we really add Version now? Isn't it better to wait until the API settled a little bit? I would rather see MSBuild properties first.

If we add version we should add support for AssemblyVersion and FileVersion as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning to add VersionPrefix after all the current tasks are done, along with Properties. I don't think we should use Version, I think it gets overridden in some situations unexpectedly and doesn't work with VersionSuffix.

@patriksvensson if my memory is correct, when I was testing using VersionPrefix with the build command acted as AssemblyVersion


/// <summary>
/// Gets or sets a value indicating whether to print the incremental safety checks that prevent incremental compilation.
/// </summary>
public bool BuildProfile { get; set; }

/// <summary>
/// Gets or sets a value indicating whether to mark the build as unsafe for incrementality.
/// This turns off incremental compilation and forces a clean rebuild of the project dependency graph.
Expand All @@ -57,4 +47,4 @@ public sealed class DotNetCoreBuildSettings : DotNetCoreSettings
/// </summary>
public bool NoDependencies { get; set; }
}
}
}
15 changes: 1 addition & 14 deletions src/Cake.Common/Tools/DotNetCore/Build/DotNetCoreBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,6 @@ private ProcessArgumentBuilder GetArguments(string project, DotNetCoreBuildSetti
builder.AppendQuoted(settings.OutputDirectory.MakeAbsolute(_environment).FullPath);
}

// Temporary output directory
if (settings.BuildBasePath != null)
{
builder.Append("--build-base-path");
builder.AppendQuoted(settings.BuildBasePath.MakeAbsolute(_environment).FullPath);
}

// Runtime
if (!string.IsNullOrEmpty(settings.Runtime))
{
Expand Down Expand Up @@ -105,12 +98,6 @@ private ProcessArgumentBuilder GetArguments(string project, DotNetCoreBuildSetti
builder.Append(settings.VersionSuffix);
}

// Build Profile
if (settings.BuildProfile)
{
builder.Append("--build-profile");
}

// No Incremental
if (settings.NoIncremental)
{
Expand All @@ -126,4 +113,4 @@ private ProcessArgumentBuilder GetArguments(string project, DotNetCoreBuildSetti
return builder;
}
}
}
}