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

Use Azure Pipelines exclusively (i.e. drop Travis, AppVeyor) #1761

Merged
merged 7 commits into from
Feb 16, 2020

Conversation

AArnott
Copy link
Contributor

@AArnott AArnott commented Jan 31, 2020

Here is a sample github action result. To see this on your own Checks tab, I think you'll have to just accept this PR.
After completing, you'll also want to go delete the hooks from this repo to Travis and AppVeyor so they stop kicking off on pushes or PRs to this repo.

Here is a sample code coverage report if we could reactivate it. That will require resolving the problem of the shadow copied assembly failing signing checks.

I don't actually deactivate Azure Pipelines with this change yet. Rather, I enhance the Azure Pipelines integration. Azure Pipelines has a built-in test and test coverage display whereas GitHub Actions does not. We should probably decide which to keep as part of this PR and disable the other.

Closes #1760

@bording
Copy link
Member

bording commented Jan 31, 2020

Can you rebase this to get rid of the Pinning and fixCompilerWarnings commits?

LibGit2Sharp/LibGit2Sharp.csproj Outdated Show resolved Hide resolved
LibGit2Sharp.Tests/StatusFixture.cs Outdated Show resolved Hide resolved
@AArnott AArnott force-pushed the validate/githubActions branch 2 times, most recently from c19a3e0 to 075cac5 Compare January 31, 2020 04:48
@AArnott
Copy link
Contributor Author

AArnott commented Jan 31, 2020

After completing, you'll also want to go delete the hooks from this repo to Travis and AppVeyor so they stop kicking off on pushes or PRs to this repo.

BTW running dotnet test on the Azure Pipelines agents anyway will run the tests against mono. But some of the tests failed, so I disabled that. It wasn't something you were already covering before anyway. But if you 'support' this library on mono, I suggest for a subsequent PR we explore those failures and see if we can fix them.

Do you know whether you want to go the Azure Pipelines route or the GitHub Actions route (or both)? You can see how Azure Pipelines now recognizes tests. If you enable analytics for your AZP account, you'll even see which tests are unstable, etc. It's pretty sweet. But GitHub Actions show logs right within GitHub which is also nice. Decisions, decisions. 🤷‍♂

@AArnott
Copy link
Contributor Author

AArnott commented Jan 31, 2020

After investigating several test failures in the last 24 hours (usually due to #1764) where both GitHub Actions and Azure Pipelines was in operation, I can heartily suggest we go with Azure Pipelines. Just searching for which test failed in the GitHub Actions log is super painful. Contrast that with Azure Pipelines (now with this PR) integrated test results and analytics (when you turn that on) is a supreme experience.

@bording
Copy link
Member

bording commented Jan 31, 2020

@AArnott That sounds good to me. Let's drop the GitHub Actions stuff in this PR then.

@AArnott AArnott force-pushed the validate/githubActions branch 4 times, most recently from 84d7213 to f9571fe Compare January 31, 2020 16:25
@AArnott AArnott changed the title Use GitHub Actions instead of Travis, AppVeyor and Azure Pipelines Use Azure Pipelines exclusively (i.e. drop Travis, AppVeyor) Jan 31, 2020
@AArnott
Copy link
Contributor Author

AArnott commented Jan 31, 2020

Don't merge yet. The net46 tests aren't running and I figured out how to get code coverage to work.

@AArnott
Copy link
Contributor Author

AArnott commented Jan 31, 2020

Ok, we're reading to merge this. Check this out:

Individual test results:

image

And integrated code coverage:

image

You can also see an even nicer coverage report if you'll visit https://codecov.io/gh/libgit2/libgit2sharp/ and just click the 'turn on' button.

@AArnott
Copy link
Contributor Author

AArnott commented Jan 31, 2020

Oh, and if you enable the codecov.io integration, your PRs will also get a nice comment like this so you know what impact the PR has on your code coverage.

@bording
Copy link
Member

bording commented Feb 1, 2020

@AArnott Thanks for taking the time to do this, but I think I'm going to need this to be split into a few steps. There are several things in here that seem like unrelated changes that are all done in a single commit with no real explanation.

Also, there are a ton of new scripts that weren't in here before. Why do we suddenly need all of these if we already had Azure Pipelines CI working and running tests? That's a lot to take on and be responsible for maintaining.

@AArnott
Copy link
Contributor Author

AArnott commented Feb 2, 2020

Many of these new files come in from my https://github.com/aarnott/library.template template repo, where I refined the art of writing build authoring that runs on all three operating systems, both locally on dev boxes and (minimal) code that only works on Azure Pipelines itself, which makes maintaining and testing build authoring as easy as possible. So yes, this came in as a significant 'chunk' because I copied and pasted it from a template. I don't really want to "re-invent the wheel" by breaking it into smaller commits as it takes a long time to figure out how to get it right, and I've already done that in that template.

This PR brings up your Azure Pipelines YML to cover everything (AFAIK) that AppVeyor and Travis were giving you, drops those other CIs, and yes, it beefs up your Azure Pipelines a bit more. I could add comments to many places in this PR to explain why it's all useful/necessary. I'd rather do that than artificially break it up into smaller chunks.

@AArnott
Copy link
Contributor Author

AArnott commented Feb 3, 2020

@bording will you accept a bunch of PR comments explaining what everything does, and then you can decide whether to reject certain parts and I'll remove those?

@bording
Copy link
Member

bording commented Feb 3, 2020

@bording will you accept a bunch of PR comments explaining what everything does, and then you can decide whether to reject certain parts and I'll remove those?

That sounds like a okay way to move forward, yes.

This removes scripts that came from Library.Template that don't really apply to libgit2sharp, or could be added later as their own focused change.
Directory.Build.props Show resolved Hide resolved
Directory.Build.props Show resolved Hide resolved
<Exclude>[xunit.*]*</Exclude>
<!-- Ensure we preserve each coverlet output file per target framework: https://github.com/tonerdo/coverlet/issues/177 -->
<CoverletOutput>$(OutputPath)/</CoverletOutput>
</PropertyGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole file enables code coverage collection when /p:CollectCoverage=true is specified, as is now done in the Azure Pipelines .yml file.

@@ -11,7 +11,8 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.0.1" />
<PackageReference Include="coverlet.msbuild" Version="2.7.0" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required for code coverage.

LibGit2Sharp.Tests/StatusFixture.cs Show resolved Hide resolved
command: push
packagesToPush: $(Pipeline.Workspace)/deployables-Windows/*.nupkg
nuGetFeedType: internal
publishVstsFeed: $(ci_feed)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you set ci_feed to the name of an Azure Artifacts feed, this will automatically light up and push your nupkg there.
If you have a public project on Azure Pipelines where this build occurs, this feed can be public as well, giving folks a place to get the latest CI builds of your library. This is a boon for folks who don't want to wait for your (I think quarterly?) scheduled releases to nuget.org

@@ -0,0 +1,2 @@
$globalJson = Get-Content -Path "$PSScriptRoot\..\..\global.json" | ConvertFrom-Json
$globalJson.sdk.version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This script is used by init.ps1 to install whatever .NET Core SDK happens to be required per your global.json file.

@echo off
SETLOCAL
set PS1UnderCmd=1
powershell.exe -NoProfile -NoLogo -ExecutionPolicy bypass -Command "try { & '%~dpn0.ps1' %*; exit $LASTEXITCODE } catch { write-host $_; exit 1 }"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For folks who prefer cmd.exe's over powershell for their CLI, this is a convenient stub that invokes init.ps1 in a natural way.

@@ -0,0 +1,67 @@
<#
.SYNOPSIS
Installs dependencies required to build and test the projects in this repository.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After running this script, any machine should be ready to build and run your tests.

This MAY not require elevation, as the SDK and runtimes are installed to a per-user location,
unless the `-InstallLocality` switch is specified directing to a per-repo or per-machine location.
See detailed help on that switch for more information.
.PARAMETER InstallLocality
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you trust OSS projects that you can't build without giving them elevated admin permissions to install stuff? I don't. This script defaults to not requiring elevation by installing dependencies in non-privileged locations. But you can opt into installing at per-machine locations for greater convenience. See docs in this script for more info.

@AArnott
Copy link
Contributor Author

AArnott commented Feb 11, 2020

So what do you think of my own code review comments above, @bording?

@bording
Copy link
Member

bording commented Feb 11, 2020

I haven't had time to look at them yet, but I hope to soon. Thanks for taking the time to do that.

@bording
Copy link
Member

bording commented Feb 15, 2020

@AArnott Looking over the stuff here, I do have some general thoughts/questions.

It's unfortunate that adding the code coverage stuff requires all the extra complexity of multiple steps and scripts, etc. I'm not really used to using code coverage tools, so it makes me wonder if it's worth the cost.

We currently have buildandtest.cmd / buildandtest.sh that were previously used by the CI scripts and could also be used locally. These are no longer used, right? How would someone locally run a build that is the same as what CI would do then?

@AArnott
Copy link
Contributor Author

AArnott commented Feb 15, 2020

It's unfortunate that adding the code coverage stuff requires all the extra complexity of multiple steps and scripts, etc. I'm not really used to using code coverage tools, so it makes me wonder if it's worth the cost.

@bording The code coverage is only half of it. The other half of the extra steps is due to simply wanting individual test run reports per-framework and per agent.

We currently have buildandtest.cmd / buildandtest.sh that were previously used by the CI scripts and could also be used locally. These are no longer used, right?

They're not used in CI any more, that's correct. They could be, but then the test results wouldn't be included nicely by Azure Pipelines' test reports. But end users can certainly use them if they're useful. Or we can remove them.

How would someone locally run a build that is the same as what CI would do then?

Build or test with dotnet build or dotnet test does effectively the same thing as CI (in a simpler way, since we don't need CI reporting when local tests run).

CI runs on all 3 agents of course, and a 4th agent to run with /p:ExtraDefine=LEAKS_IDENTIFYING. Even the original 2 buildandtest scripts didn't specify that without an extra argument passed to the scripts.

So we can decide whether to keep the scripts (which IMO don't add any value) or remove them.

@bording
Copy link
Member

bording commented Feb 16, 2020

@bording The code coverage is only half of it. The other half of the extra steps is due to simply wanting individual test run reports per-framework and per agent.

True, but there would definitely be fewer changes and steps if the coverage stuff wasn't included in this.

However, after thinking about it for a bit, I think it's not worth the effort to get rid of it since you've already got all of it working.

So we can decide whether to keep the scripts (which IMO don't add any value) or remove them.

Let's go ahead and remove them as part of this PR and see how it goes.

Once you remove those, I think I'm ready to approve and merge this, though I reserve the right to be able to ask you for help with any of these scripts if something goes wrong in the future! 😄

@bording bording merged commit ceadb47 into libgit2:master Feb 16, 2020
@AArnott AArnott deleted the validate/githubActions branch February 16, 2020 04:25
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.

Evolve CI builds
2 participants