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

Build in Azure Pipelines with cached vcpkg artifacts #114

Merged
merged 16 commits into from
Sep 26, 2019

Conversation

lukka
Copy link
Member

@lukka lukka commented Sep 18, 2019

Please note that acceptance of community PRs will be delayed while we are
bringing our test and CI systems online. For more information, see the
README.md.

Description

I hope this PR is either useful and could be an inspiration on how to set up a CI system.
This PR is not meant to be merged as is, but it is created in order to show how it is possible to run a build using CMake and Ninja with cached vcpkg artifacts on Hosted Build Agents.

Hosted agents do not have the required MSVC 14.23, hence the build also install that toolset on the fly (considerably increasing build time).

The build time on Hosted agents with cached vcpkg artifacts is about:

  • restoring cache 30 seconds;
  • building 4 minutes.
  • building vcpkg artifacts 15 minutes;

See here for all build samples.

No tests are run.

Also, this PR shows how to explicitly define which vcpkg version is to be used by using a git submodule.

Checklist:

  • I understand README.md.
  • The STL builds and test harnesses have passed (must be manually verified
    by an STL maintainer before CI is online, leave this unchecked for initial
    submission).

@BillyONeal
Copy link
Member

We are absolutely interested in setting up an Azure Pipelines (or maybe GitHub Actions) script for this repo, but we are looking into solutions for hosting the build agents. The default provided "hosted" agents are little dual core boxes, and once we get our tests in here you'd likely be waiting 8+ hours for an answer.

Unfortunately, with a self hosted build agent system, there don't appear to be good options to handle untrusted pull request submissions. We may just put Azure DevOps in "manually initiated" mode but we'd like to give people more immediate feedback (before review) if possible.

I've asked the Pipelines team if they have any ideas but haven't gotten reasonable answers back yet.

Thanks for your contribution!

@BillyONeal BillyONeal self-assigned this Sep 19, 2019
@sylveon
Copy link
Contributor

sylveon commented Sep 19, 2019

Just a thought, there could be a thing running on the hosted agents which only builds the tests to do basic validity checking and then use a GitHub bot to trigger the full test suite internally.

@lukka
Copy link
Member Author

lukka commented Sep 19, 2019

@BillyONeal self hosted agents are the way to go indeed (the Microsoft hosted agents do not have the prerequisites to build anyway).
What about expendable VMs created off an existing image on Azure Compute? or the biggest size not powerful enough? If those are not and you need some extremely powerful physical machine, all they need is to not be part of corporate network and then you save any security concern.

@BillyONeal
Copy link
Member

Just a thought, there could be a thing running on the hosted agents which only builds the tests to do basic validity checking and then use a GitHub bot to trigger the full test suite internally.

That wouldn't prevent a PR from sending malicious code to our internal test infrastructure that isn't prepared to deal with that.

What about expendable VMs created off an existing image on Azure Compute?

That would work fine but I don't think there's a great way to get Azure DevOps to recycle the machine upon completion (which is necessary if we want to build and run arbitrary internet submitted code).

If those are not and you need some extremely powerful physical machine, all they need is to not be part of corporate network and then you save any security concern.

Just because the machine isn't on the corporate network doesn't mean there aren't serious problems with letting an attacker gain persistence. For example, using Azure bandwidth to attack other Internet entities, or mining bitcoins on our hardware.

Azure DevOps does have an "OK to test check" thing ( https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops&tabs=yaml#comment-triggers ) we'll probably end up using for now.

@sylveon
Copy link
Contributor

sylveon commented Sep 21, 2019

I meant a manually triggered GitHub bot, basically like the comment triggers of DevOps.

@lukka
Copy link
Member Author

lukka commented Sep 23, 2019

Running the build agent on a docker image stored in Azure Container Registry (and that contains all pre-reqs for building) could be a good fit, as long it is going to be cached locally on the host to start up quickly. Have not tried this scenario yet though.

@BillyONeal
Copy link
Member

@StephanTLavavej @CaseyCarter @barcharcraz Are you OK with the submodule to get vcpkg here? Since we're using a self-hosted agent I can just put that in the agent setup instructions but given that we already have a contribution that does that setup for us which will work if/when we can use the hosted system, I'm reluctant to throw that work away.

ado/run_build.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
boost-build:x86-windows
Copy link
Member

Choose a reason for hiding this comment

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

why do we need boost build explicitly listed?

Copy link
Member Author

Choose a reason for hiding this comment

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

in my experience, when running 'vcpkg install boost-math:x86-windows', it failed asking me to install 'boost-build:x86-windows' as well. And doing this failed with same error: 'vcpkg install boost-build:x86-windows boost-math:x86-windows'. I had to run first the installation of boost-build:x86-windows, and then run the installation of boost-math:x86-windows separately to succeed.

Copy link
Member

Choose a reason for hiding this comment

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

this might be a issue worth reporting in vcpkg, but I'm not sure.

@barcharcraz
Copy link
Member

@StephanTLavavej Stephan T. Lavavej FTE @CaseyCarter Casey Carter FTE @barcharcraz Charlie Barto FTE Are you OK with the submodule to get vcpkg here? Since we're using a self-hosted agent I can just put that in the agent setup instructions but given that we already have a contribution that does that setup for us which will work if/when we can use the hosted system, I'm reluctant to throw that work away.

Also, I'm OK with it as long as we know it can work with the new caching feature.

VCPKG itself is pretty small, it's the built artifacts that take a long time.

@BillyONeal
Copy link
Member

Hmmm this PR was created without the ability for us to edit it, so I can't try it out to see if it'll work on our self-hosted agent. Do you mind if I recreate this in my fork?

Thanks for your contribution!

@BillyONeal
Copy link
Member

/AzurePipelines run STL

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@lukka
Copy link
Member Author

lukka commented Sep 25, 2019

i can also do the requested changes here, just let me know what works better for you @BillyONeal

@CaseyCarter
Copy link
Contributor

@StephanTLavavej Stephan T. Lavavej FTE @CaseyCarter Casey Carter FTE @barcharcraz Charlie Barto FTE Are you OK with the submodule to get vcpkg here? Since we're using a self-hosted agent I can just put that in the agent setup instructions but given that we already have a contribution that does that setup for us which will work if/when we can use the hosted system, I'm reluctant to throw that work away.

Also, I'm OK with it as long as we know it can work with the new caching feature.

VCPKG itself is pretty small, it's the built artifacts that take a long time.

Despite my earlier private comments that we should avoid adding a submodule, I'm now in favor of whatever gets us up and running the quickest. We can refine things later if we find improvements, but for now we shouldn't let the perfect be the enemy of the good.

@BillyONeal
Copy link
Member

i can also do the requested changes here, just let me know what works better for you @BillyONeal

If you merge with master (please preserve the clang-format validation in at least one configuration) it should have the bits working. Note that it won't actually attempt to run the changes automatically without pre-review from us.

@BillyONeal
Copy link
Member

i can also do the requested changes here, just let me know what works better for you @BillyONeal

If you merge with origin/master it should have the bits that indicate to run using the machine on my desk. It will require pre-review from us though... since it's a self-hosted agent and we don't have a mechanism to automatically recycle it we need pre-review to ensure folks aren't submitting malware or similar.

@lukka lukka requested a review from a team as a code owner September 25, 2019 02:49
@BillyONeal
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@BillyONeal
Copy link
Member

@lukka
Copy link
Member Author

lukka commented Sep 25, 2019

@BillyONeal the tasks needs to be installed from the marketplace

@BillyONeal BillyONeal mentioned this pull request Sep 25, 2019
5 tasks
azure-devops/install_msvc_preview.ps1 Outdated Show resolved Hide resolved
azure-devops/install_msvc_preview.ps1 Show resolved Hide resolved
azure-devops/run_build.yml Show resolved Hide resolved
azure-devops/run_build.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,51 @@
parameters:
targetPlatform: 'x64'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have a parameters block here when the parameters come from azure-pipelines.yml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is declaration and default value of accepted params. The actual value is indeed in a-p.yml

Copy link
Contributor

@CaseyCarter CaseyCarter Sep 25, 2019

Choose a reason for hiding this comment

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

Rephrasing: do we need to have this declaration here, or can we leave it out so this template will fail when it's not used correctly? It would be a shame if someone accidentally introduced a typo in the parameter names (e.g., targetP1atform) in azure-pipelines.yml and we consequently build x64 four times for every PR.

Copy link
Contributor

@CaseyCarter CaseyCarter Sep 25, 2019

Choose a reason for hiding this comment

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

I suppose if it MUST be here we could give it an invalid value like potato to ensure it will fail very loudly.

azure-devops/run_build.yml Show resolved Hide resolved
azure-devops/install_msvc_preview.ps1 Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

.gitmodules Show resolved Hide resolved
[string]$bootstrapperExe = Join-Path ${env:Temp} ([System.IO.Path]::GetRandomFileName() + ".exe")
Invoke-WebRequest -Uri $VSBootstrapperURL -OutFile $bootstrapperExe

$Arguments = ('/c', $bootstrapperExe, $WorkLoads, '--quiet', '--norestart', '--wait', '--nocache' )
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Why is there a space before the final )?

}
else
{
Write-Host -Object "Non zero exit code returned by the installation process : $exitCode."
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Nonzero or Non-zero.

}

# Invalidate the standard installation of VS on the hosted agent.
Move-Item "C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/" "C:/Program Files (x86)/Microsoft Visual Studio/2019/nouse/" -Verbose
Copy link
Member

Choose a reason for hiding this comment

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

This seems highly invasive. Is it desirable? Could we simply fail if we detect an existing installation of VS?

Copy link
Member

Choose a reason for hiding this comment

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

This script is only run on the "hosted" machines which are blown away after each build, so invasive changes are fine/normal.

Copy link
Member Author

Choose a reason for hiding this comment

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

then you would always fail on the hosted one.

'--add Microsoft.VisualStudio.Component.VC.Tools.x86.x64 ' + `
'--add Microsoft.VisualStudio.Component.VC.Tools.ARM64 ' + `
'--add Microsoft.VisualStudio.Component.VC.Tools.ARM ' + `
'--add Microsoft.VisualStudio.Component.Windows10SDK.18362 '
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to request the latest Win10 SDK, instead of encoding this version number here that might need to be updated?

Copy link
Member

Choose a reason for hiding this comment

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

Not as far as I am aware.

inputs:
cmakeListsTxtPath: 'CMakeSettings.json'
useVcpkgToolchainFile: true
configurationRegexFilter: '.*${{ parameters.targetPlatform }}.*'
Copy link
Member

Choose a reason for hiding this comment

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

Is this doing a whole-string or a substring match? If the latter, the .* at the front and back are unnecessary.

Copy link
Member

@BillyONeal BillyONeal Sep 26, 2019

Choose a reason for hiding this comment

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

I'm not positive which it is but I also think it doesn't matter. (And we should merge this as-is)

# Copyright (c) Microsoft Corporation.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

# Build STL targeting x64, arm64, x86, arm.
Copy link
Member

Choose a reason for hiding this comment

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

Our usual order is x86, x64, arm, arm64. Is this order significant in some way?

boost-math:x64-windows
boost-math:x86-windows
boost-math:arm-windows
boost-math:arm64-windows
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 file have to be in the root of the repo? Would azure-devops be a better location? It's mentioned by azure-devops/run_build.yml.

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

parameters:
targetPlatform: x64


Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for there to be two newlines here?

@@ -0,0 +1,5 @@
boost-build:x86-windows
boost-math:x64-windows
boost-math:x86-windows
Copy link
Member

Choose a reason for hiding this comment

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

I observe that this mentions x64 before x86.

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal BillyONeal merged commit d7c7691 into microsoft:master Sep 26, 2019
@BillyONeal BillyONeal mentioned this pull request Sep 26, 2019
7 tasks
BillyONeal added a commit to BillyONeal/cpprestsdk that referenced this pull request Sep 27, 2019
BillyONeal added a commit to BillyONeal/cpprestsdk that referenced this pull request Sep 27, 2019
BillyONeal added a commit to BillyONeal/cpprestsdk that referenced this pull request Sep 27, 2019
BillyONeal added a commit to BillyONeal/cpprestsdk that referenced this pull request Sep 27, 2019
BillyONeal added a commit to BillyONeal/cpprestsdk that referenced this pull request Sep 27, 2019
BillyONeal added a commit to BillyONeal/cpprestsdk that referenced this pull request Sep 27, 2019
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2019
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.

6 participants