-
Notifications
You must be signed in to change notification settings - Fork 546
[build] Extend dotnet build support on unix #6585
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
Conversation
5983c2f
to
9f0a184
Compare
9f0a184
to
2cef9a1
Compare
@@ -22,7 +22,7 @@ _PREPARE_CI_MODE_ARGS = $(_PREPARE_CI_MODE_PR_ARGS) -a | |||
_PREPARE_ARGS = | |||
|
|||
all: | |||
$(call MSBUILD_BINLOG,all,$(_SLN_BUILD)) /restore $(MSBUILD_FLAGS) $(SOLUTION) | |||
$(call DOTNET_BINLOG,all) $(MSBUILD_FLAGS) $(SOLUTION) -m:1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should DOTNET_BINLOG
add the -m:1
, so this is set in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we want to set it for every dotnet
invocation as we should only need it when building Xamarin.Android.sln.
I haven't found great documentation on this parameter though. Neither the command line docs (below) or this guide mention whether or not this is a "solution only" flag, or if it also works on projects with multiple project references.
-maxCpuCount[:n] Specifies the maximum number of concurrent processes to
build with. If the switch is not used, the default
value used is 1. If the switch is used without a value
MSBuild will use up to the number of processors on the
computer. (Short form: -m[:n])
The command line docs aren't consistent with what I see locally either. The Xamarin.Android.sln build fails for me in lots of weird ways if -m:1
isn't set (even though the docs suggest the default value should be 1, which I assume would be "no parallelization" ). I know we had to set this explicitly on the Windows .sln build in the past for similar reasons as well.
The overall mac build job does seem faster in general in this PR, but I haven't done any digging into how much time is saved by 1) skipping the OSS zip creation, 2) using dotnet build more, or even the potential of 3) having more parallelization in some build commands by default.
My inclination is to keep it mostly as is, and only specify the -m
flag when we know something breaks without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, we can leave -m:1
for now.
<Target Name="_MakeMonoUnixFatBinariesOSX" | ||
AfterTargets="CopyFilesToOutputDirectory" | ||
Inputs="@(_MonoUnixDylib)" | ||
Outputs="$(_MonoUnixOutputDylib)"> | ||
<Exec | ||
Command="lipo @(_MonoUnixDylib->'"%(FullPath)"', ' ') -create -output "$(_MonoUnixOutputDylib)"" | ||
Condition=" '$(HostOS)' == 'Darwin' " | ||
/> | ||
<Copy | ||
SourceFiles="@(_MonoUnixDylib)" | ||
DestinationFolder="$(OutputPath)" | ||
Condition=" '$(HostOS)' != 'Darwin' " | ||
/> | ||
</Target> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we have this twice now:
Is there a way to put this target in its own file and import twice? If we used the same MSBuild properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my original thought, but the overhead didn't seem worth it compared to copying a few lines. I think we'd want this if we need to do something similar in a third or fourth place in the future though, so I will try to adjust this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, if the builds end up green. 👍
We shouldn't want/need to update
|
I think this is saying you still need to build I don't currently see why we would need |
@@ -316,8 +316,7 @@ | |||
<Target Name="ILRepacker" | |||
BeforeTargets="CopyFilesToOutputDirectory" | |||
Inputs="$(MSBuildAllProjects);@(IntermediateAssembly);@(InputAssemblies)" | |||
Outputs="$(IntermediateOutputPath)ILRepacker.stamp" | |||
Condition=" '$(HostOS)' != 'Linux' "> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can anyone remember the reason why we disabled this in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to crash on Linux under Mono
Commit message draft: Bumps to mono/debugger-libs@c53676b
Changes: https://github.com/mono/debugger-libs/compare/08268b730a73cca0e3b3ac9c703f0b773a3be25b...c53676b08005c806fc170b7be5cda25e52fe9054
Many instances of `msbuild` have been replaced with `dotnet build` in
our make files. The `-m:1` flag has been added to `Xamarin.Android.sln`
builds to disable parallelization. We still run into issues running
`dotnet build` on the .sln when this flag is not used. Additionally,
`msbuild` is still required when building `xabuild.csproj` and
`create-vsix.csproj`.
The `make jenkins` target has been updated to no longer create an OSS
.zip file to improve build speeds. OSS installers can be built
separately as needed.
The `Xamarin.Android.Build.Tasks` build has been updated to run ILRepack
on Linux to fix [various issues][0].
A new `MonoUnixNative.targets` file has been added that can be used by
projects that produce an assembly containing MSBuild tasks that depend
on Mono.Unix. The `<_MakeMonoUnixFatBinariesOSX/>` target will create
a multiarchitecture version of `libMono.Unix.dylib` in the projects
output directory when running on macOS. It will also copy
`runtimes\linux-x64\native\libMono.Unix.so` and
`runtimes\osx-x64\native\libMono.Unix.dylib` to the output directory
when running on Windows or Linux.
[0]: https://github.com/xamarin/xamarin-android/pull/5681#issuecomment-790017891 |
Changes:
Changes: mono/debugger-libs@08268b7...c53676b
Limitations: