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

Conversation

baronfel
Copy link
Contributor

Fixes #1539 by having the msbuild detection logic learn about mono versions.

For all branches we added the ability for the MSBuild environment variable to specify just a tool, which would then be searched for on the current PATH.

For mono >= 5.0.0.0 we default to msbuild on all path lookups, etc
For mono < 5.0.0.0 we default to xbuild for all similar lookups.

In addition I thought it would be a good chance to make the default msbuild functions pass through the number of cores on the machine in order to trigger parallel msbuild behavior.

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?

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

{ 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 :)

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.

Thank you :)

@baronfel
Copy link
Contributor Author

@matthid good catches on both counts, thanks!

@baronfel
Copy link
Contributor Author

That's fair. I've removed the CPU change.

@matthid
Copy link
Member

matthid commented Apr 29, 2017

https://travis-ci.org/fsharp/FAKE/builds/227224324#L6135 Good that we can get away from mono soon :)

@forki forki merged commit 35e2280 into fsprojects:master May 2, 2017
@forki
Copy link
Member

forki commented May 2, 2017

thanks for fixing this.

@nosami
Copy link
Member

nosami commented May 2, 2017

LGTM

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.

4 participants