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 appveyor integration by not using cake for build #6958

Merged
merged 18 commits into from
Nov 27, 2019
Merged

Conversation

huoyaoyuan
Copy link
Contributor

Fixes #6913 .
The key improvements: compiler and nuget warnings are populated, Xamarin projects are covered

I'm keeping in mind that making build configuration straightforward.
AppVeyor directly integrates to MSBuild. The configuration is quite simple.
Note that much more warnings are populated, including ones during restore and packing. It's very boring and hard to collection all them in cake.

InspectCode integrates very badly with the .NET Core world. It's strongly depending "desktop" MSBuild which is only accessible from Visual Studio Developer CLI, and supporting only Windows. Integrating it into the experience of VS will probably break command line on .NET Core. So I'm keeping the current progress with cake unchanged.

I also removed the "1 script to do what CI does at local", because:

  • Many developers doesn't have Xamarin installed. Then they will require manually skipping them in the script.
  • It's only possible to do exactly everything on Windows now.
  • It doesn't fit the workflow, especially with IDE.
  • User wants to fix violations instead of just prompting them, for example the dry run of dotnet format.

I've tried a lot at my local fork to make sure everything is working. Deployment is proved by the correct artifacts collected by AppVeyor. (https://ci.appveyor.com/project/huoyaoyuan/osu-deploy/builds/29081533/artifacts)

@huoyaoyuan
Copy link
Contributor Author

Validated at my fork. The validation of deployment is a bit out of date. It requires to deploy to a fake environment.

@huoyaoyuan
Copy link
Contributor Author

@huoyaoyuan huoyaoyuan force-pushed the ci branch 2 times, most recently from dd404f7 to c51b6f4 Compare November 25, 2019 15:54
@huoyaoyuan
Copy link
Contributor Author

Changing the tests directory organization should be suitable for another PR.

@huoyaoyuan
Copy link
Contributor Author

No: currently android test will fail on loading beatmap resource. Since no one is actually using it, let's delay it.

osu.Game/osu.Game.csproj Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@huoyaoyuan
Copy link
Contributor Author

AppVeyor does't allow mixture of propertyset and command line. Enabling WarningAsError without incluencing normal build requires explicit specifying everything.
Alternatively, I'm relatively OK with WarningAsError during normal development.
Or let AppVeyor "just work without regression" (not prompted at all -> reported to discord)?

@peppy
Copy link
Member

peppy commented Nov 27, 2019

I'm also not strongly against WarningAsError being always on.

@smoogipoo thoughts?

@huoyaoyuan
Copy link
Contributor Author

Or a condition in csproj to detect weather it's built by appveyor, similar to what we did for referencing native libs from framework.

@smoogipoo
Copy link
Contributor

I think it’s fine as always on. We’ve already had a warning in osu.Game that we’ve ignored for months.

@huoyaoyuan
Copy link
Contributor Author

Windows build for iOS provides warnings, and is skipped now via solution configuration. Once ppy/osu-framework#3044 get resolved, it can have managed side covered by Windows pool.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

let's give this a shot

@peppy peppy merged commit 7e128cb into ppy:master Nov 27, 2019
@huoyaoyuan huoyaoyuan deleted the ci branch November 27, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix AppVeyor integration
4 participants