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

Fix inconsistent naming of MSBuild package, add setParams to run* methods #1837

Merged
merged 4 commits into from
Apr 1, 2018
Merged

Conversation

sliepie
Copy link
Contributor

@sliepie sliepie commented Mar 27, 2018

Naming of MSBuild within module was inconsitent between MsBuild and MSBuild, since official naming is MSBuild I've renamed everything so it's consistent with official name.

Also added the setParams MSBuild.build function to the MSBuild.run* functions where applicable, so it's constent with other Fake 5 api's.
In Fake 4 this wasn't necessary because you could edit the mutable MSBuildDefaults property.

Also currently we have the following code in the MSBuild module:

 let mutable private MSBuildLoggers =
    []
    //[ ErrorLoggerName ]
    //|> List.map (fun a -> sprintf "%s,\"%s\"" a pathToLogger)

  do
    // Add MSBuildLogger to track build messages
    match BuildServer.buildServer with
    | BuildServer.AppVeyor ->
        MSBuildLoggers <- @"""C:\Program Files\AppVeyor\BuildAgent\Appveyor.MSBuildLogger.dll""" :: MSBuildLoggers
    //| BuildServer.TeamCity -> MSBuildLoggers <- sprintf "%s,\"%s\"" TeamCityLoggerName pathToLogger :: MSBuildLoggers
    | _ -> ()

Can this safely be removed? As we have now the BuildServer.* packages

The same applies to the following in MSBuildLogger:

(* TODO: Add to TeamCity
/// TeamCity Logger for MSBuild
type TeamCityLogger() =
    inherit MSBuildLogger()
    override this.RegisterEvents(eventSource) =
        eventSource.ErrorRaised.Add(fun a -> errToStr a |> TeamCityHelper.sendTeamCityError)
        *)

Finally is the NO_MSBUILD_AVAILABLE directive still necessary?
This directive is only used in Xamarin + MSBuild packages

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Looks good to me. I always wondered about the correct casing...

Thanks for taking care of this! The only thing missing is to fix the CI. Let me know if things break apart.

build.fsx Outdated
@@ -349,7 +349,7 @@ Target.create "UnskipAndRevertAssemblyInfo" (fun _ ->
)

Target.create "BuildSolution_" (fun _ ->
MsBuild.runWithDefaults "Build" ["./src/Legacy-FAKE.sln"; "./src/Legacy-FAKE.Deploy.Web.sln"]
MSBuild.runWithDefaults "Build" ["./src/Legacy-FAKE.sln"; "./src/Legacy-FAKE.Deploy.Web.sln"]
Copy link
Member

Choose a reason for hiding this comment

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

To fix the build-script you need to nest this change into a #if BOOTSTRAP block

Copy link
Contributor Author

@sliepie sliepie Mar 29, 2018

Choose a reason for hiding this comment

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

The #if BOOTSTRAP trick didn't work when building on my laptop as it expected MsBuild instead of MSBuild.
However Travis and AppVeyor builds both needed MSBuild as I expected, the cause is probably some stale artifacts on my laptop.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. Artefacts will be added to your .nuget folder (in your users directory). Maybe our build script needs to clean them...

@matthid matthid merged commit 99d493e into fsprojects:rc_1 Apr 1, 2018
@matthid
Copy link
Member

matthid commented Apr 1, 2018

Thanks!

This was referenced Apr 2, 2018
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.

3 participants