Update nuke build to include unsigned and non-merged client nuget targets for local builds #782
Conversation
| @@ -1,4 +1,4 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
I changed the file encoding to UTF8 with no BOM so that when we do inline edits of the file and then revert it, it doesn't leave a pending change in git.
build/Build.cs
Outdated
| @@ -189,6 +189,33 @@ class Build : NukeBuild | |||
| }); | |||
| }); | |||
|
|
|||
| Target PackMergedClientNugetForMacOs => _ => _ | |||
| .OnlyWhenStatic(() => IsLocalBuild) | |||
There was a problem hiding this comment.
Is the indenting a bit funny here or is it just github displaying it?
I'd expect this, in keeping with the rest of the file
Target PackMergedClientNugetForMacOs => _ => _
.OnlyWhenStatic(() => IsLocalBuild) // note tabbed in by 4 spaces here
.OnlyWhenStatic(() => OperatingSystem.IsMacOS())There was a problem hiding this comment.
Yeah I think my Rider is not doing this right. Let me fix that up.
There was a problem hiding this comment.
I think that's fixed now.
build/Build.cs
Outdated
| @@ -189,6 +189,33 @@ class Build : NukeBuild | |||
| }); | |||
| }); | |||
|
|
|||
| Target PackMergedClientNugetForMacOs => _ => _ | |||
| .OnlyWhenStatic(() => IsLocalBuild) | |||
| .OnlyWhenStatic(() => OperatingSystem.IsMacOS()) | |||
There was a problem hiding this comment.
You could merge .OnlyWhenStatic(() => IsLocalBuild && !OperatingSystem.IsWindows())
There was a problem hiding this comment.
As per your comments below, I've remove the operating system check altogether 👍
build/Build.cs
Outdated
| @@ -189,6 +189,33 @@ class Build : NukeBuild | |||
| }); | |||
| }); | |||
|
|
|||
| Target PackMergedClientNugetForMacOs => _ => _ | |||
There was a problem hiding this comment.
You've happened to hit the issue on macOS, but neither the problem nor solution are macOS specific.
Other developers on linux will have the same problem, and if a windows dev is in your situation they may be able to run ILRepack, but the Azure code signing keys most likely won't be accessible and they also may want an unsigned nuget package.
Suggestion: Remove the 'macOS' and call this something more generic like PackUnsignedClient
There was a problem hiding this comment.
Also: This target is called PackMergedClient but it doesn't seem to be doing any ILMerging?
There was a problem hiding this comment.
I've updated the naming of targets to make it clearer and I've removed the MacOS specific naming to make it more generically creating "Unsigned" nuget packages 👍
| @@ -0,0 +1,27 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
As above, this is not mac specific - perhaps call it "Local" or "Unsigned" instead of "MacOs.nuspec"
| <projectUrl>https://github.com/OctopusDeploy/OctopusClients</projectUrl> | ||
| <icon>images\icon.png</icon> | ||
| <description>Octopus Deploy is an automated release management tool for modern developers and DevOps teams. | ||
| This package contains the ILmerged client library for the HTTP API in Octopus.</description> |
There was a problem hiding this comment.
Description is copied from the release nuspec which is probably wrong. This one doesn't contain an ILMerged client library for example.
Suggestion: Put a developer-specific description in here e.g. "This package contains an unsigned, non-ILMerged, locally built client library for the HTTP API in Octopus"
build/Build.cs
Outdated
| }); | ||
|
|
||
| void PackNormalClientNugetPackage() |
There was a problem hiding this comment.
Should this be it's own Nuke target rather than a function, so other targets can DependsOn it?
There was a problem hiding this comment.
I don't think we can in this case because of the order of execution. For the Signed Client we need to sign the binaries before packing which means the packing target would need to depend on the signing target but then we can't use the packing target for anything else because you can't have a conditional dependency which is a shame.
| @@ -115,63 +115,63 @@ class Build : NukeBuild | |||
| Target Merge => _ => _ | |||
| .DependsOn(Compile) | |||
| .Executes(() => | |||
| { | |||
| foreach (var target in new[] { "net462", "net48", "netstandard2.0" }) | |||
There was a problem hiding this comment.
This whole target has not changed - It's just less indented to match other formatting... I can revert it if it's annoying.
scme0
changed the title
The Nuke build fails in a few places for MacOS which meant I had to manually comment things out to get it to play ball.
The two problematic areas are:
I've added a build target to create unsigned and non-merged Client Nuget packages for a Local builds so they can be used for testing purposes locally.