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

Azure Pipeline for Windows CI #1853

Merged
merged 22 commits into from
May 2, 2019

Conversation

lokitoth
Copy link
Member

  • Fixes the projects to build with VS2017 (without VS2015 installed or manual setup to get TextTransform tool set up)
  • Adds dependencies to ensure we do not build projects multiple times due to issues with msbuild dependency tracing
  • Adds additional testing to test.cmd and generate a test log file so results can be exported in CI setups
  • Updates packaging infrastructure to support SemVer 2.0
  • Define Azure Pipeline CI yaml for the Windows/NuGet build

@lokitoth
Copy link
Member Author

(Please do not "Update Branch" - I will rebase)

Copy link
Member

@jackgerrits jackgerrits left a comment

Choose a reason for hiding this comment

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

This looks amazing, can't wait to have it checked in!

One thing I am thinking though is, do we want to move to powershell scripts for some of this? As we build more cmd infra it gets harder to eventually change

@@ -10,7 +10,7 @@ PUSHD %~dp0

REM TODO: Figure out how to parametrize this script?! (is there a standard, or do we actually need parse args?)
ECHO Building "%vwRoot%\vowpalwabbit\vw.sln" for Release x64
"%msbuildPath%" /v:normal /m /p:Configuration=Release;Platform=x64 "%vwRoot%\vowpalwabbit\vw.sln"
"%msbuildPath%" /v:normal /m /nr:false /p:Configuration=Release;Platform=x64 "%vwRoot%\vowpalwabbit\vw.sln"
Copy link
Member

Choose a reason for hiding this comment

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

what is nr?

Copy link
Member Author

Choose a reason for hiding this comment

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

This disables node-reuse, which means it gets new msbuild "nodes" (processes) to build. It leads to more reliable builds, at the cost of a minute amount of built time, linear in number of projects.

)

IF NOT DEFINED msbuildPath (
IF NOT EXIST "%VsInstallDir%\MSBuild\15.0\Bin\MSBuild.exe" (
Copy link
Member

Choose a reason for hiding this comment

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

Consider inverting the condition so setting the variable is next to the exist check

@@ -15,8 +15,8 @@
<NuGetPackageImportStamp>
</NuGetPackageImportStamp>
<TargetFrameworkProfile />
<OutputPath>$(SolutionDir)out\target\$(Configuration)\$(PlatformShortName)\</OutputPath>
<IntermediateOutputPath>$(SolutionDir)out\int\$(Configuration)\$(PlatformShortName)\$(ProjectName)\</IntermediateOutputPath>
<OutputPath>$(SolutionDir)out\target\$(Configuration)\x64\</OutputPath>
Copy link
Member

Choose a reason for hiding this comment

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

Does this lock the solution to x64 only? #1553 seems to indicate some users build for 32bit

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, it just would introduce some oddness about building Win32 going to the x64 directory. I am not sure the 32-bit build works. Would prefer to deal with it in a separate issue, especially since we are doing the work to get Cmake builds to work.

@jackgerrits
Copy link
Member

jackgerrits commented May 2, 2019

Is there any way we can speed this build up? It's about 5 minutes slower than appveyor (27 vs 22 ish). Why does the build stage take 22 minutes? On linux the similar stage is 3 minutes. I understand it is also building C++ CLI and C#, but surely this doesn't account for all the time?

lokitoth and others added 22 commits May 2, 2019 10:16
   Solution files offer a way to code-in build dependencies which do not get resolved properly by MsBuild.

   It was necessary to add the dependencies for vw_core and libvw (and vw_clr as a precaution) so that the managed projects do not kicked off until the unmanaged dependencies were done. Otherwise the unmanaged projects get kicked off immediately when the build starts and chain to a second, parallel build of vw_core. This breaks the build when the same files (e.g. vw_core.lib, or .obj files) are being written concurrently.
Currently c_test breaks CI/CD that rely on stderror spew due to vw using cerr for info
The project was incorrectly referencing vw_core rather than libvw; unfortunately, due to explicit ordering in the soultion and an explicit reference to the lib, this was not detected until trying to fix the solution dependencies.
* Disable it for vw_clr because the rules file does not contain compatible rules for this kind of project
* Move output into a separate /package folder under /out
* Make the script agnostic of where we invoke it from
* Ensure output directory is generated before calling nuget pack
* This was removed a long time ago, but some projects did not get cleaned up properly
@lokitoth
Copy link
Member Author

lokitoth commented May 2, 2019

Is there any way we can speed this build up? It's about 5 minutes slower than appveyor (27 vs 22 ish). Why does the build stage take 22 minutes? On linux the similar stage is 3 minutes. I understand it is also building C++ CLI and C#, but surely this doesn't account for all the time?

Not sure what can really be done here, since just buliding the vw_core.vcxproj takes longer than any of the targets.

Other contributors:

  • We build a static lib, then link it in multiple times:
    • vw.exe
    • libvw.dll
    • VowpalWabbit.Core.dll
    • python? (not sure about this one)

Since I believe we are doing whole program optimization, then link is probably fairly slow.

I would like to see what cmake / clang can do for our build times, but that is beyond the score of this PR.

@lokitoth lokitoth merged commit 7161aaa into VowpalWabbit:master May 2, 2019
jackgerrits pushed a commit to jackgerrits/vowpal_wabbit that referenced this pull request May 15, 2019
* Update Clarius.TransformOnBuild to work with VS2017

* Simplify vw_core.vcxproj

* Simplify libvw.vcxproj

* Update NuGet.exe to support SemVer

* Add explicit project dependencies to the solution

   Solution files offer a way to code-in build dependencies which do not get resolved properly by MsBuild.

   It was necessary to add the dependencies for vw_core and libvw (and vw_clr as a precaution) so that the managed projects do not kicked off until the unmanaged dependencies were done. Otherwise the unmanaged projects get kicked off immediately when the build starts and chain to a second, parallel build of vw_core. This breaks the build when the same files (e.g. vw_core.lib, or .obj files) are being written concurrently.

* Enable finding VS2017 MSBuild automatically

* Fix init.cmd to also find vstest.console when it is not provided

* Enable a way to suppres c smoke test

Currently c_test breaks CI/CD that rely on stderror spew due to vw using cerr for info

* Fix c_test.vcxproj dependency

The project was incorrectly referencing vw_core rather than libvw; unfortunately, due to explicit ordering in the soultion and an explicit reference to the lib, this was not detected until trying to fix the solution dependencies.

* Disable node reuse to avoid keeping stale MSBuild.exe processes around after build

* Fix CodeAnalysis rule files location

* Disable it for vw_clr because the rules file does not contain compatible rules for this kind of project

* Unify C# outputs

* Fix NuGet pack binary search paths

* Make package.cmd more robust

* Move output into a separate /package folder under /out
* Make the script agnostic of where we invoke it from
* Ensure output directory is generated before calling nuget pack

* Remove in-build NuGet restore

* Remove DebugLeakCheck configuration

* This was removed a long time ago, but some projects did not get cleaned up properly

* Simplify cluster.vcxproj and unify output paths

* Normalize usage of $(SolutionDir)

* Simplify cs_unittest.csproj and unify outputs

* Switch to using exit codes to perform test validation

* Create Azure Pipeline configuration for Windows CI

* Bring set closer to check for envvars
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.

2 participants