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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify Windows and mono build systems #16451

Merged
merged 6 commits into from May 5, 2019

Conversation

@pchote
Copy link
Member

commented Apr 27, 2019

This PR wraps up the last major toolchain work that I wanted to complete for the next release.
This takes us another big step towards #15954 and removes a lot of legacy cruft by migrating to the VS 2017 / .NET core csproj format and configuring it to work the same way as the Makefile:

  • Assemblies are written directly to the desired location instead of copied
  • No Release/Debug distinction - we always optimize and always generate pdbs
  • Warn on error is enabled by default
  • /LARGEADDRESSAWARE is set for exes

fixheader.exe has been promoted to OpenRA.PostProcess.exe and I expect that we will use this to generate compile-time sync metadata using Mono.Cecil in the future (see #15954).

This puts the prerequisites in place for managing deps using nuget project references, but making the switch is deferred to a future PR once we can work out how to deal with the non-nuget deps.

I've tested the Makefile on macOS and Ubuntu 18.04, the make powershell works on Windows 10, and running in the debugger works with Rider and VS:Mac on macOS, and MonoDevelop on Linux. I've also done a basic prototype to test that this should work with the Mod SDK once that is updated to match.

The only problem I encountered was with debugging with vs:mac, which insists on running the game with mono32 and fails because the native deps are 64 bit, but can't compile as 64 bit because the CLR deps have a metadata flag specifying 32 bit 馃し鈥嶁檪 It turns out that bleed doesn't compile at all with VS:Mac, so this is already a win and further fixes are out of scope for this PR.

Test packages are available from : https://github.com/pchote/OpenRA/releases/tag/pkgtest-20190427

Fixes #12125
Fixes #16439

@pchote pchote added this to the Next Release milestone Apr 27, 2019

@pchote pchote force-pushed the pchote:csproj branch 2 times, most recently from 8520100 to 9b4ec9a Apr 27, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

Updated. Considering the complexity here, i'm pushing the fixes as their own commits.
We can squash these before merging if there are any objections to this approach.

@pchote pchote force-pushed the pchote:csproj branch 16 times, most recently from c5d7e2a to 65825fe Apr 28, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Works as intended on windows!

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

Changing <PlatformTarget> to AnyCPU fixes the debugging issues on macOS, but i'm assuming that this will make the windows builds prefer 64-bit and therefore fail. I'm planning to defer this for now, and a followup PR (likely for Next + 1 or later now) can set up 32/64bit-conditional dependencies in the csproj for people compiling on Windows, and a way to force a specific configuration when compiling from the packaging scripts. We can then ship specific 32 and 64 bit installers.

@pchote

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

This is going to conflict with every PR that touches the csproj files. I'd prefer not to have to rebase this after every merge, and instead wait until there are either review comments to change or this is ready to merge.

@ltem ltem added the PR: Needs +2 label May 1, 2019

@obrakmann
Copy link
Contributor

left a comment

This seems to work ok so far. Tested compilation by MD and by Makefile, unit tests by MD and Makefile, as well as StyleCop checks by both MD and Makefile. The latter fails on my system, but it has always done that.

I've also noticed that msbuild doesn't work when started in the background, it just sits around and waits until it gets back to the foreground. If there's a solution to that, that would be nice.

OpenRA.Game/OpenRA.Game.csproj Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved

@pchote pchote force-pushed the pchote:csproj branch 2 times, most recently from 07ada77 to 0d1fd7f May 2, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Updated to revert the "AnyCPU" changes in OpenRA.sln.

@teinarss

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

One regression I noticed is that after the compile, I can no longer close the window by pressing any button, I have to close the window by clicking the X.

@abcdefg30 raised this point on IRC, and unfortunately we don't know why this happens or how to fix it.

[21:27:10] | <abcdefg30> hm, make all hangs for me at the end
[21:27:19] | <abcdefg30> after the build completed (and it displays the time)
[21:27:28] | <pchote> :(
[21:27:41] | <pchote> does that line have the -nr? or whatever that arg was
[21:28:14] | <abcdefg30> let me check
[21:28:24] | <abcdefg30> it doesn't hang the second time, btw
[21:28:36] | <abcdefg30> (probably no surprise if it's the server thing)
[21:28:50] | <pchote> since there are many of those
[21:29:05] | <abcdefg30> "C:\Program Files\dotnet\dotnet.exe" "build /nr:false"
[21:29:07] | <abcdefg30> seems like it
[21:29:39] | <abcdefg30> but let's not hold the PR up onto that. imho we can fix it later if we need to
[21:30:00] | <pchote> i won't complain at that

I did, but I think you misunderstand what I meant.

Ok. I'll put this in the "joys of windows development" basket, like the freeze above.

Could add dotnet build-server shutdown to the end of function All-Command

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

That seems to work. Fixed.

@pchote

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Scratch that - the issue returned after a couple of test builds, so i'll drop that commit again.

@pchote pchote force-pushed the pchote:csproj branch from ac959a7 to 0d1fd7f May 3, 2019

make.ps1 Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Updated and made some other minor tweaks to make.ps1 - see commit message for details.

@pchote pchote force-pushed the pchote:csproj branch from 370ea8b to 30953b6 May 3, 2019

@teinarss
Copy link
Contributor

left a comment

Everything looks good, only thing i notice is that VS19 likes to reorganize the sln file

OpenRA.sln Show resolved Hide resolved
pchote added 6 commits Apr 27, 2019
Reduce minimum .NET requirement to 4.6.1.
4.7.2 causes compatibility issues with Mono 5.4
in the interim period before we migrate to netstandard2.
Unify Windows and mono build systems.
The Makefile behaviour is recreated using the new and significantly
cleaner .NET Core csproj format.

fixheader.exe is promoted to OpenRA.PostProcess.exe and now runs
on all platforms.
Remove Start-Process from make.ps1.
This also removes the hardcoded path
requirement for dotnet.exe, and simplifies
the utility and dotnet detection.

The nr flag is no longer needed for dotnet,
and nologo is added to reduce unnecessary output.

The Release configuration is set for consistency
with the real Makefile.

@pchote pchote force-pushed the pchote:csproj branch from 30953b6 to 43c4d41 May 4, 2019

@teinarss
Copy link
Contributor

left a comment

Looks good now!

@reaperrr reaperrr merged commit f0d5939 into OpenRA:bleed May 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.