-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Switched build to Nuke #2154
Switched build to Nuke #2154
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.
Looking good! I've got a few nits, a few questions, and a few things that need to be fixed before we can merge this.
.travis.yml
Outdated
@@ -13,7 +13,7 @@ dotnet: 2.1.200 | |||
script: | |||
- sudo apt-get update | |||
- sudo apt-get install castxml | |||
- ./build.sh --target "Travis" --configuration "Release" | |||
- ./build.sh --target "CiTravis" --configuration "Release" |
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.
Any particular reason we're keeping our Travis config since we're not running our builds on Travis any more?
appveyor.yml
Outdated
@@ -14,7 +14,7 @@ init: | |||
before_build: | |||
- git submodule update --init | |||
build_script: | |||
- ps: .\build.ps1 -Target "AppVeyor" -Configuration "$env:configuration" | |||
- ps: .\build.ps1 --target "CiAppVeyor" --configuration "$env:configuration" |
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.
Do we need/want to still keep our Appveyor builds now that our Azure DevOps build system has stabilized?
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 was porting the script, so I've kept it's functionality the same. It could be removed if we dont beed appveyor anymore
nukebuild/Build.cs
Outdated
|
||
Target Clean => _ => _.Executes(() => | ||
{ | ||
var data = Parameters; |
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 don't know how Nuke works, but is there a way we could pass in the Parameters
object into the target as a parameter or state?
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.
into the target as a parameter or state
State is defined by the Build
object itself.
.SetConfiguration(data.Configuration) | ||
.SetVerbosity(MSBuildVerbosity.Minimal) | ||
.AddProperty("PackageVersion", Parameters.Version) | ||
.AddProperty("iOSRoslynPathHackRequired", "true") |
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.
Do you have any idea why we still need this? Building from the command line doesn't seem to require this hack.
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.
Honestly, no idea, but even if it's not beeded, it should be removed in a separate PR
.SetVerbosity(MSBuildVerbosity.Minimal) | ||
.AddProperty("PackageVersion", Parameters.Version) | ||
.AddProperty("iOSRoslynPathHackRequired", "true") | ||
.SetToolsVersion(MSBuildToolsVersion._15_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.
Do we need to set the tools version?
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.
Cake script was specifying it
nukebuild/BuildParameters.cs
Outdated
IsPullRequest = TeamServices.Instance.PullRequestId.HasValue; | ||
IsMainRepo = StringComparer.OrdinalIgnoreCase.Equals(MainRepo, TeamServices.Instance.RepositoryUri); | ||
|
||
// TODO??? |
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.
What's this TODO for?
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 have found a way to get the tag from Azure DevOps. The original script was ignoring it though.
nukebuild/BuildParameters.cs
Outdated
public BuildParameters(Build b) | ||
{ | ||
var buildSystem = Host; | ||
|
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.
nit: remove extra whitespace.
nukebuild/BuildParameters.cs
Outdated
|
||
public BuildParameters(Build b) | ||
{ | ||
var buildSystem = Host; |
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.
nit: rename uses of buildSystem
to use Host
and remove the buildSystem
variable.
@@ -0,0 +1,24 @@ | |||
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> |
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.
Move this file into the folder with the rest of our .DotSettings
files.
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 think those are project-local settings.
@@ -97,17 +96,17 @@ jobs: | |||
vmImage: 'vs2017-win2016' | |||
steps: | |||
- task: CmdLine@2 |
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.
Change this to use the DotNetTool
step instead of command-line?
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't find the docs on DotNetTool
. BTW Nuke script name is nuke
, not dotnet-nuke
, so dotnet nuke
doesn't work.
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.
LGTM! Remove the appveyor badge from the README and delete the appveyor project and we're all good to merge!
Why Nuke?
dotnet run
I think that's enough to switch from Cake.