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

2148: Add dotnet nuget push command and arguments #2229

Merged

Conversation

Projects
None yet
2 participants
@atlemann
Copy link
Contributor

commented Dec 21, 2018

Add dotnet nuget push command

Fixes: #2148

@atlemann atlemann changed the title [WIP] 2148: Add dotnet nuget push command and arguments 2148: Add dotnet nuget push command and arguments Dec 28, 2018

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2019

Question is if we can somehow re-use existing code. Similar to how we did in msbuild. Basically dotnet msbuild is CLI equivalent to msbuild.exe. The same is true for dotnet nuget and nuget.exe.... Ideally we update them together and not have to fix bugs twice...

@matthid matthid changed the base branch from master to release/next Jan 12, 2019

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Yes, it would be nice to share some code. I can't seem to find what code is shared between msbuild and dotnet msbuild though.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

Fake.DotNet.Cli references Fake.DotNet.MSBuild:

<ProjectReference Include="..\Fake.DotNet.MSBuild\Fake.DotNet.MSBuild.fsproj" />

And some refactoring was done to use a common structure (in order to convert between old and new structure and have a single logic):

type CliArguments =
{ Targets : string list
Properties : (string * string) list
/// corresponds to the msbuild option '/m':
/// - 'None' will omit the option.
/// - 'Some None' will emit '/m'.
/// - 'Some 2' will emit '/m:2'.
MaxCpuCount : int option option
/// Execute a restore before executing the targets (/restore flag)
DoRestore : bool
NoLogo : bool
NodeReuse : bool
ToolsVersion : string option
Verbosity : MSBuildVerbosity option
NoConsoleLogger : bool
WarnAsError: string list option
NoWarn: string list option
/// Fake attaches a binlog-logger in order to report errors and warnings. You can disable this behavior with this flag
DisableInternalBinLog: bool
/// corresponds to the msbuild option '/fl'
FileLoggers : MSBuildFileLoggerConfig list option
/// corresponds to the msbuild option '/bl'
BinaryLoggers : string list option
/// corresponds to the msbuild option '/consoleloggerparameters'
ConsoleLogParameters : MSBuildLogParameter list
/// corresponds to the msbuild option '/l'
Loggers : (MSBuildLoggerConfig) list option
/// corresponds to the msbuild option '/dl'
DistributedLoggers : (MSBuildLoggerConfig * MSBuildLoggerConfig option) list option }
static member Create() : CliArguments =
{ Targets = []
Properties = []
MaxCpuCount = None
DoRestore = false
NoLogo = false
NodeReuse = false
ToolsVersion = None
Verbosity = None
NoConsoleLogger = false
WarnAsError = None
NoWarn = None
DisableInternalBinLog = false
ConsoleLogParameters =
if BuildServer.ansiColorSupport then [ForceConsoleColor]
else []
FileLoggers = None
BinaryLoggers = None
DistributedLoggers = None
Loggers = None }
let internal asCliArguments (x:MSBuildParams) : CliArguments =
{ Targets = x.Targets
Properties =
("RestorePackages", x.RestorePackagesFlag.ToString()) :: x.Properties
MaxCpuCount = x.MaxCpuCount
NoLogo = x.NoLogo
NodeReuse = x.NodeReuse
DoRestore = x.DoRestore
ToolsVersion = x.ToolsVersion
Verbosity = x.Verbosity
NoConsoleLogger = x.NoConsoleLogger
WarnAsError = x.WarnAsError
NoWarn = x.NoWarn
DisableInternalBinLog = x.DisableInternalBinLog
ConsoleLogParameters = x.ConsoleLogParameters
FileLoggers = x.FileLoggers
Loggers = x.Loggers
BinaryLoggers = x.BinaryLoggers
DistributedLoggers = x.DistributedLoggers }
let internal withCliArguments (oldObj:MSBuildParams) (x:CliArguments) =
{ oldObj with
Targets = x.Targets
Properties = x.Properties
MaxCpuCount = x.MaxCpuCount
DoRestore = x.DoRestore
NoLogo = x.NoLogo
NodeReuse = x.NodeReuse
RestorePackagesFlag =
x.Properties
|> Seq.tryFind (fun (p,v) -> p = "RestorePackages")
|> (function
| Some (_, v) -> System.Boolean.Parse v
| None -> false)
ToolsVersion = x.ToolsVersion
Verbosity = x.Verbosity
NoConsoleLogger = x.NoConsoleLogger
WarnAsError = x.WarnAsError
NoWarn = x.NoWarn
Loggers = x.Loggers
DisableInternalBinLog = x.DisableInternalBinLog
ConsoleLogParameters = x.ConsoleLogParameters
FileLoggers = x.FileLoggers
BinaryLoggers = x.BinaryLoggers
DistributedLoggers = x.DistributedLoggers }

I'm not sure if this is viable approach for NuGet. Honestly I'm not all to happy with the NuGet module API...

matthid and others added some commits Feb 15, 2019

Merge branch '2148-add.dotnet.nuget.push.delete' of github.com:atlema…
…nn/FAKE into 2148-add.dotnet.nuget.push.delete
@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

I'm trying to figure out if anything can be shared. The NuGet.exe module combines pack and push in the same arguments. However, NuGet pack doesn't build the project like dotnet pack does, so if we want to do something similar for the DotNet.Cli module, we would also need the CliArguments.

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

The Process.withFramework also makes reuse of the NuGet publish hard, since the whole point is not to use Mono.

Arguments = args }) >> Process.withFramework) parameters.TimeOut

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

But I guess we could split the pack and push specific parts of the current NuGetParams into seperate records and have some conversion between the current NuGetParams (like for msbuild) and a new one which holds the pack and push ones? Then they would reuse the same records at least.

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Maybe we need to update DotNet.Cli.PackOptions to hold some that new pack specific record as well then. Like Authors, Tags, Description etc.

@atlemann atlemann changed the title 2148: Add dotnet nuget push command and arguments [WIP] 2148: Add dotnet nuget push command and arguments Feb 28, 2019

atlemann added some commits Feb 28, 2019

[WIP] Add NugetPushOptions record in NuGet module that can be shared …
…with DotNetCli nuget push.

Add new push function which takes this record instead of NuGetParams.
Added function for converting a NugetPushOptions to NuGet.exe style C…
…li args.

Make push function usable from DotNet.Cli module.
Remove the old publish function.
@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@matthid Not sure I'm going in the right direction here, but at least most of the code is shared between the two modules.

Another thing, the NuGet.publishSymbols function seems to call publish if it fails and not itself, is that a bug?

Make a NuGetPushOptions in DotNet.Cli module with also contains the c…
…ommon Options.

Rename options in NuGet module to NuGetPushParams.
@matthid
Copy link
Collaborator

left a comment

Because I'm lacking a better approach, I think the one proposed in this PR is fine. I have added some detail comments which we need to address/answer

@@ -39,6 +39,18 @@ type NugetSymbolPackage =
/// Build a symbol package using the nuspec file
| Nuspec = 2

type ToolOptions =

This comment has been minimized.

Copy link
@matthid

matthid Mar 16, 2019

Collaborator

Can you add short docs for this or mark it internal/private?

This comment has been minimized.

Copy link
@atlemann

atlemann Mar 16, 2019

Author Contributor

I'll make it internal, since I'm not reusing it in the DotNet module anymore.

param.Timeout |> Option.map string |> Option.map (sprintf "-Timeout %s")
]
|> List.choose id
|> String.concat " "

This comment has been minimized.

Copy link
@matthid

matthid Mar 16, 2019

Collaborator

Can you use the proper APIs for this (Arguments.ofList for example)?


module Private =
/// For internal use
let rec push (options : ToolOptions) (parameters : NuGetPushParams) (toCliArgs : NuGetPushParams -> string) nupkg =

This comment has been minimized.

Copy link
@matthid

matthid Mar 16, 2019

Collaborator

So this only currently handles dotnet nuget push we should think about other commands as well.

  • Do we add all operations by adding new functions one by one on demand?
  • Should we use a similar model as dotnet uses where you have a "low"-level DotNet.exec function where others like dotnet test build on top.
  • Something different?

Personally I'd lean to something along the second option, but if there are good reasons to do it differently I'm all ears. Note that I'm not proposing to implement all options in this PR, but whatever we decide to release should be extendible later.

This comment has been minimized.

Copy link
@atlemann

atlemann Mar 16, 2019

Author Contributor

The second option sounds like a good idea so I'll not reuse this in the DotNet module, but rather use the exec in there instead. I'll for now make this NuGet.Private.push internal instead and maybe it could be generalized to be the low level one?


try
let result =
let tracing = Process.shouldEnableProcessTracing()

This comment has been minimized.

Copy link
@matthid

matthid Mar 16, 2019

Collaborator

If we really want to disable tracing we should use the corresponding CreateProcess functionality. We should remove this class to Process.shouldEnableProcessTracing to reduce side effects.

This comment has been minimized.

Copy link
@atlemann

atlemann Mar 16, 2019

Author Contributor

Right, I just copied some other code in the same module. I can just remove it I guess.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2019

Sorry for the late reply.

But I guess we could split the pack and push specific parts of the current NuGetParams into seperate records and have some conversion between the current NuGetParams (like for msbuild) and a new one which holds the pack and push ones?

I think I have answered this in the comment regarding which API surface we want, we could try a similar approach as dotnet itself but we need good documentation as the syntax will become a bit messy (to edit nested record fields)

Another thing, the NuGet.publishSymbols function seems to call publish if it fails and not itself, is that a bug?

To be honest I don't know. I'm not really using this module myself :)

Make NuGet.ToolOptions internal
Make NuGet.Private.push private
Use DotNet.exec for DotNet.nugetPush instead of NuGet.Private.push.

@matthid matthid self-requested a review Mar 17, 2019

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Mar 17, 2019

Changes look reasonable, is there anything left as this is still marked WIP?

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Not really, I guess I was to tired to notice 😁

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

How do I edit the title from a phone...hmmm...

@atlemann atlemann changed the title [WIP] 2148: Add dotnet nuget push command and arguments 2148: Add dotnet nuget push command and arguments Mar 17, 2019

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

I kind of feel the NuGet module is obsolete now that there is dotnet CLI. But I guess it might be a bit drastic to deprecate it?

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Mar 17, 2019

But I guess it might be a bit drastic to deprecate it?

Indeed, but we could do it eventually. Sorry, just one more request, can we add a simple test for this (ie similar to others a single test checking if command line generation is OK)?

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Yes, I can add a test.

Fix bugs and add unit-tests.
- Add nupkg to arguments
- Convert timeout TimeSpan to seconds

atlemann added some commits Mar 20, 2019

@atlemann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Added some tests and fixed some bugs. I also added retry like in the NuGet module.

@matthid
Copy link
Collaborator

left a comment

Thanks again. One more minor nit (but I can do that myself once I'm ready to merge this in a couple of days, so don't worry):
I think we should add a bit of docs for NuGetPushOptions and a single example changing some parameters on top of the DotNet.nugetPush function (just like we have for the DotNet.msbuild function), because people might not realize how to change nested records in F#.

@matthid matthid merged commit c38a472 into fsharp:release/next Apr 4, 2019

4 checks passed

FAKE-CI #949 succeeded
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matthid matthid referenced this pull request Apr 8, 2019

Merged

Release 5.13.0 #2279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.