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 msbuild being used on mono < 5.0.0.0 instead of xbuild #1540

Merged
merged 3 commits into from
May 2, 2017
Merged
Changes from 1 commit
Commits
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
53 changes: 37 additions & 16 deletions src/app/FakeLib/MSBuildHelper.fs
Original file line number Diff line number Diff line change
Expand Up @@ -45,39 +45,56 @@ let toDict items =
let getAllKnownPaths =
(knownMsBuildEntries |> List.collect (fun m -> m.Paths) |> List.rev) @ oldMsBuildLocations

/// Versions of Mono prior to this one have faulty implementations of MSBuild
let monoVersionToUseMSBuildOn = System.Version("5.0.0.0")

/// Tries to detect the right version of MSBuild.
/// - On all OS's, we check a `MSBuild` environment variable which is either
/// * a direct path to a file to use, or
/// * a directory that contains a file called `msbuild` on non-Windows systems, or `MSBuild.exe` on Windows systems
/// * a directory that contains a file called
/// * `msbuild` on non-Windows systems with mono >= 5.0.0.0, or
/// * `xbuild` on non-Windows systems with mono < 5.0.0.0,
/// * `MSBuild.exe` on Windows systems, or
/// * a tool that exists on the current PATH
/// - In addition, on non-Windows systems we check the current PATH for the following binaries, in this order:
/// * `msbuild`
/// * `xbuild`
/// * Mono >= 5.0.0.0: `msbuild`, `xbuild`
/// * Mono < 5.0.0.0: `xbuild`, `msbuild`
/// * This is due to several known issues in the Mono < 5.0 implementation of MSBuild.
/// - In addition, on Windows systems we
/// * try to read the MSBuild tool location from the AppSettings file using a parameter named `MSBuild`, and finally
/// * if a `VisualStudioVersion` environment variable is specified, we try to use the specific MSBuild version, matching that Visual Studio version.
let msBuildExe =
/// the value we're given can be a full path to a file or just a directory.
/// the value we're given can be a:
/// * full path to a file or
/// * just a directory
/// if just a directory we can make it the path to a file by Path-Combining the tool name to the directory.
let exactPathOrBinaryOnPath tool input =
if FileSystemHelper.isDirectory input && Directory.Exists input
then input </> tool
else input

let which tool = ProcessHelper.tryFindFileOnPath tool
let environVarDir = EnvironmentHelper.environVarOrNone "MSBuild"
let msbuildEnvironVar = EnvironmentHelper.environVarOrNone "MSBuild"

if isUnix
then
let unixSources = [
environVarDir |> Option.map (exactPathOrBinaryOnPath "msbuild")
match isUnix, EnvironmentHelper.monoVersion with
| true, Some(_, Some(version)) when version > monoVersionToUseMSBuildOn ->
Copy link
Member

Choose a reason for hiding this comment

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

maybe even >= but I don't think it matters

let sources = [
msbuildEnvironVar |> Option.map (exactPathOrBinaryOnPath "msbuild")
msbuildEnvironVar |> Option.bind which
which "msbuild"
which "xbuild"
]
defaultArg (unixSources |> List.choose id |> List.tryHead) "xbuild"
else
defaultArg (sources |> List.choose id |> List.tryHead) "xbuild"
Copy link
Member

Choose a reason for hiding this comment

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

Should we use msbuild here?

| true, _ ->
let sources = [
msbuildEnvironVar |> Option.map (exactPathOrBinaryOnPath "xbuild")
msbuildEnvironVar |> Option.bind which
which "xbuild"
which "msbuild"
]
defaultArg (sources |> List.choose id |> List.tryHead) "xbuild"
| false, _ ->

let envVarPath = environVarDir |> Option.map (exactPathOrBinaryOnPath "MSBuild.exe")
let configIgnoreMSBuild =
if "true".Equals(ConfigurationManager.AppSettings.["IgnoreMSBuild"], StringComparison.OrdinalIgnoreCase)
then Some ""
Expand All @@ -90,12 +107,13 @@ let msBuildExe =

ProcessHelper.tryFindFileInDirsThenPath vsVersionPaths "MSBuild.exe"

let windowsSources = [
envVarPath
let sources = [
msbuildEnvironVar |> Option.map (exactPathOrBinaryOnPath "MSBuild.exe")
msbuildEnvironVar |> Option.bind which
configIgnoreMSBuild
findOnVSPathsThenSystemPath
]
defaultArg (windowsSources |> List.choose id |> List.tryHead) "MSBuild.exe"
defaultArg (sources |> List.choose id |> List.tryHead) "MSBuild.exe"


/// [omit]
Expand Down Expand Up @@ -446,7 +464,10 @@ let MSBuildWithProjectProperties outputPath (targets : string) (properties : (st
|> Set.unionMany

let setBuildParam project projectParams =
{ projectParams with Targets = targets |> split ';' |> List.filter ((<>) ""); Properties = projectParams.Properties @ properties project }
{ projectParams with
Targets = targets |> split ';' |> List.filter ((<>) "")
Properties = projectParams.Properties @ properties project
MaxCpuCount = Some ( Some (EnvironmentHelper.getMachineEnvironment().ProcessorCount)) }
Copy link
Member

Choose a reason for hiding this comment

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

I think its ok. But doesn't msbuild figure this out when no argument is given?

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 noticed that the FAKE build itself, which uses MSBuildWithDefaults doesn't build with multiple cores because the CPU Count isn't provided on this very flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in fact, none of the MSBuildXXXX helpers set the core count, so they fall prey to this. The only way to get around it is to use raw build setParams yourself.

Copy link
Member

Choose a reason for hiding this comment

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

@baronfel To be honest I don't think we should do such changes in FAKE 4 anymore. But I guess @forki probably has a different opinion on this. If it breaks somebody we will move this to FAKE 5 anyways :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave it up to you :)


projects
|> List.filter (fun project -> not <| Set.contains project dependencies)
Expand Down