-
Notifications
You must be signed in to change notification settings - Fork 898
Use Azure Pipelines exclusively (i.e. drop Travis, AppVeyor) #1761
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
Conversation
80c6968
to
e2aeae8
Compare
Can you rebase this to get rid of the Pinning and fixCompilerWarnings commits? |
c19a3e0
to
075cac5
Compare
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 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. 🤷♂ |
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. |
@AArnott That sounds good to me. Let's drop the GitHub Actions stuff in this PR then. |
84d7213
to
f9571fe
Compare
Don't merge yet. The net46 tests aren't running and I figured out how to get code coverage to work. |
f9571fe
to
30a8e57
Compare
It was sorting in an undefined way that led it to fail on Ubuntu.
30a8e57
to
ddd4998
Compare
Ok, we're reading to merge this. Check this out: Individual test results: And integrated code coverage: 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. |
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. |
@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. |
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. |
@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.
<Exclude>[xunit.*]*</Exclude> | ||
<!-- Ensure we preserve each coverlet output file per target framework: https://github.com/tonerdo/coverlet/issues/177 --> | ||
<CoverletOutput>$(OutputPath)/</CoverletOutput> | ||
</PropertyGroup> |
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
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.
command: push | ||
packagesToPush: $(Pipeline.Workspace)/deployables-Windows/*.nupkg | ||
nuGetFeedType: internal | ||
publishVstsFeed: $(ci_feed) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 }" |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
So what do you think of my own code review comments above, @bording? |
I haven't had time to look at them yet, but I hope to soon. Thanks for taking the time to do that. |
@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? |
@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.
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.
Build or test with CI runs on all 3 agents of course, and a 4th agent to run with So we can decide whether to keep the scripts (which IMO don't add any value) or remove them. |
ce97c4b
to
f55e1fe
Compare
f55e1fe
to
5f47dac
Compare
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.
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! 😄 |
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