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

Add additional Squirrel parameters #2431

Merged
merged 2 commits into from
Dec 11, 2019
Merged

Add additional Squirrel parameters #2431

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bbbcbb
Add new Squirrel parameters
pchinery Nov 22, 2019
98a5031
Refactor command line to use the CreateProcess API
pchinery Dec 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions src/app/Fake.Installer.Squirrel/Squirrel.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,15 @@ type ReleasifyParams = {
/// The full path to an optional icon, which will be used for the generated installer.
SetupIcon : string option

/// Don't generate delta packages to save time
NoDelta : bool

/// Do not create an MSI file
NoMsi : bool

/// Mark the MSI as 64-bit, which is useful in Enterprise deployment scenarios
MsiWin64 : bool

/// The path to Squirrel: `squirrel.exe`
ToolPath : string

Expand All @@ -46,6 +52,12 @@ type ReleasifyParams = {

/// The secret key for the code signing certificate
SigningSecret : string option

/// Set the required .NET framework version, e.g. net461
FrameworkVersion : string option

/// Add other arguments, in case Squirrel adds new arguments in future
AdditionalArguments : string list
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we have any combination of this name already, besides the one you have chosen 💃

Expecto:

      /// Custom arguments to use in the case the helper not yet supports them
      CustomArgs : string list

DotCover:

      CustomParameters: string

OpenCover:

          /// This options is used to add additional optional arguments, could be somthing like "-returntargetcode "
          OptionalArguments : string

InnoSetup

        /// Additional parameters
        AdditionalParameters : string option

Npm/Yarn

    | Custom of string

Chocolatey

        /// A character string containing additional arguments to give to choco.
        AdditionalArgs: string

I'm not sure but at least we should be consistent and I might enforce a name via contribution guidelines. Do you like any of the existing ones?

Copy link
Member

Choose a reason for hiding this comment

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

Also I'm not sure if it should be a list or just a string (and leave escaping to the user).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Facepalm for me... I'm sorry.

I think I would prefer AdditionalParameters. The way it is implemented right now, each parameter will be string escaped and surrounded with quotes. Not doing that is easy of course. In that case, I would change it to a string option. Do you have any preference between string option and string list? I can do the change on Sunday or Monday.

Copy link
Member

Choose a reason for hiding this comment

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

I was more pointing out the bad quality of the existing code base than the name you have chosen. In fact I think one more name doesn't even matter at this point, but we should work in unifying this in the next version and I'd "lock" the one we choose now in the contribution guidelines :D

There are some tools where the "escaping" will actually break them so I tend to just provide a simple string here, but I'm open for arguments for using a list, which I guess would be more "safe".

}

let internal defaultParams = lazy(
Expand All @@ -55,12 +67,16 @@ let internal defaultParams = lazy(
BootstrapperExe = None
LoadingGif = None
SetupIcon = None
FrameworkVersion = None
NoDelta = false
NoMsi = false
MsiWin64 = false
ToolPath = Tools.findToolInSubPath toolname ( Directory.GetCurrentDirectory() </> "tools" </> "Squirrel")
TimeOut = TimeSpan.FromMinutes 10.
SignExecutable = None
SigningKeyFile = None
SigningSecret = None })
SigningSecret = None
AdditionalArguments = [] })

let private createSigningArgs (parameters : ReleasifyParams) =
new StringBuilder()
Expand All @@ -72,12 +88,19 @@ let private createSigningArgs (parameters : ReleasifyParams) =
|> StringBuilder.toText

let internal buildSquirrelArgs parameters nugetPackage =
new StringBuilder()
let sb = new StringBuilder()

parameters.AdditionalArguments |> Seq.iter (fun arg -> StringBuilder.appendIfNotNullOrEmpty arg "" sb |> ignore)

sb
|> StringBuilder.appendIfNotNullOrEmpty nugetPackage "--releasify="
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend updating this to the Arguments/CreateProcess API as the StringBuilder is a bit unsafe

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 have now added a commit that uses the CreateProcess API and uses only a string option for the args

|> StringBuilder.appendIfNotNullOrEmpty parameters.ReleaseDir "--releaseDir="
|> StringBuilder.appendIfSome parameters.FrameworkVersion (sprintf "--framework-version=%s")
|> StringBuilder.appendIfSome parameters.LoadingGif (sprintf "\"--loadingGif=%s\"")
|> StringBuilder.appendIfSome parameters.SetupIcon (sprintf "\"--setupIcon=%s\"")
|> StringBuilder.appendIfTrue parameters.NoDelta "--no-delta"
|> StringBuilder.appendIfTrue parameters.NoMsi "--no-msi"
|> StringBuilder.appendIfTrue parameters.MsiWin64 "--msi-win64"
|> StringBuilder.appendIfSome parameters.BootstrapperExe (sprintf "\"--bootstrapperExe=%s\"")
|> StringBuilder.appendIfSome parameters.SignExecutable (fun _ -> createSigningArgs parameters)
|> StringBuilder.toText
Expand Down Expand Up @@ -109,7 +132,7 @@ module internal ResultHandling =
/// Target.create "CreatePackage" (fun _ ->
/// Squirrel.releasify "./my.nupkg" (fun p -> { p with ReleaseDir = "./squirrel_release")
/// )
///
///
/// ## Defaults for setParams
///
/// - `ReleaseDir` - `""`
Expand Down Expand Up @@ -138,5 +161,5 @@ let releasify (nugetPackage: string) (setParams: ReleasifyParams -> ReleasifyPar
parameters.TimeOut

ResultHandling.failBuildIfSquirrelReportedError result

__.MarkSuccess()