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

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

Merged

Conversation

JonCubed
Copy link
Contributor

@JonCubed JonCubed commented Mar 29, 2017

  • Removed settings that are no longer valid, if you prefer for them to be marked as obsolete instead let me know.
  • Updated unit tests

See #1533 for more info

@dnfclas
Copy link

dnfclas commented Mar 29, 2017

@JonCubed,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@JonCubed JonCubed changed the title Updated DotNetCoreBuild inline with Tooling v1.0 #1533 Updated DotNetCoreBuild inline with Tooling v1.0 Mar 29, 2017
@devlead devlead modified the milestone: v0.x.0 Apr 2, 2017
@devlead devlead changed the base branch from develop to feature/dotnetV10tooling April 2, 2017 13:54
@devlead devlead changed the title Updated DotNetCoreBuild inline with Tooling v1.0 GH1533: Updated DotNetCoreBuild inline with Tooling v1.0 Apr 2, 2017
@devlead devlead self-requested a review April 2, 2017 13:57
@@ -42,11 +37,6 @@ public sealed class DotNetCoreBuildSettings : DotNetCoreSettings
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

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

@JonCubed JonCubed force-pushed the dotnetcorebuild branch 2 times, most recently from 33f8846 to f9b3ee2 Compare April 3, 2017 20:40
@devlead devlead merged commit 465d3ca into cake-build:feature/dotnetV10tooling Apr 15, 2017
@devlead
Copy link
Member

devlead commented Apr 15, 2017

@JonCubed your changes have been merged, thanks for your contribution 👍

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

Successfully merging this pull request may close these issues.

None yet

5 participants