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

enable using github actions for build #1061

Merged
merged 58 commits into from
Jan 18, 2020
Merged

Conversation

tocsoft
Copy link
Member

@tocsoft tocsoft commented Dec 11, 2019

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Adds a github workflow for building PRs using github actions and also build master branch on tags and merges.

On merges/tags packages are built and uploaded to github pacakge repository & for the time being myget (that can be phased out in the future are required.)

Known differences

Package number changes - due to the fact that github doesn't have build numbers we are generating the number from the date/time of the commit being built which should be unique and sequential enough for our use case.

As part of this PR i've also stopped Appveyor from publishing to the myget feed so we don't get a strange mix of packages up there.

Update

@JimBobSquarePants (me) added additional changes.

  • Build and test scripts are project agnostic and only rely on an environmental variable set in the yml for xunit (required for x86 tests).
  • Added targets netcorapp3.1, netstandard2.1 and set langversion to 8
  • Removed explicit net462 target from tests and added netcoreapp3.1
  • Replaced OpenCover with Coverlet
  • Replace custom versioning with compatible SemVer generated via GitVersion
  • Replaced obsolete ProjectIconUrl in nuspec properties
  • Stylecop is now referenced in all projects with reduced strictness in tests. (They're the Wild West of readability and we need a cleanup)
  • Cleanup of R# settings files (R# uses .editorconfig now ) and .vscode startup (It looked incorrect and if someone wants it they can PR)
  • Cleanup of other obsolete files.

@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #1061 into master will decrease coverage by 3.72%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
- Coverage   87.44%   83.71%   -3.73%     
==========================================
  Files        1131      688     -443     
  Lines       50041    28853   -21188     
  Branches     3677        0    -3677     
==========================================
- Hits        43756    24155   -19601     
+ Misses       5534     4698     -836     
+ Partials      751        0     -751
Flag Coverage Δ
#unittests 83.71% <ø> (?)
Impacted Files Coverage Δ
...ocessing/Extensions/Drawing/DrawImageExtensions.cs 95.65% <ø> (ø) ⬆️
...rocessing/Processors/Drawing/DrawImageProcessor.cs 100% <ø> (ø) ⬆️
...lorSpaces/Conversion/ColorSpaceConverterOptions.cs 57.14% <0%> (-42.86%) ⬇️
src/ImageSharp/Processing/ResizeOptions.cs 71.42% <0%> (-28.58%) ⬇️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 84% <0%> (-16%) ⬇️
src/ImageSharp/Formats/Png/PngEncoder.cs 84.61% <0%> (-15.39%) ⬇️
...sors/Convolution/Convolution2DProcessor{TPixel}.cs 84.61% <0%> (-15.39%) ⬇️
...essors/Convolution/ConvolutionProcessor{TPixel}.cs 84.72% <0%> (-15.28%) ⬇️
src/ImageSharp/Primitives/ColorMatrix.cs 77.36% <0%> (-11.58%) ⬇️
src/ImageSharp/Formats/Bmp/BmpInfoHeader.cs 84.61% <0%> (-7%) ⬇️
... and 624 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1df496...2e8f50d. Read the comment docs.

@JimBobSquarePants
Copy link
Member

I'm super excited about this! We can finally target NET Core 3+

I'll have a proper dig through the config as soon as I can. Only question I have straight away is that I can only see a subset of the original target frameworks from AppVeyor. Is that intentional?

@tocsoft
Copy link
Member Author

tocsoft commented Dec 12, 2019

It was somewhat intentional, removing the net642 targets was intentional they are so old i feel we can just drop version/target now. Totally forgot about x86 netcore and also mono.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Dec 12, 2019

It was somewhat intentional, removing the net642 targets was intentional they are so old i feel we can just drop version/target now. Totally forgot about x86 netcore and also mono.

I guess I'm ok with that considering the warning regarding NET 4.6x from the docs

While NuGet considers .NET Framework 4.6.1 as supporting .NET Standard 1.5 through 2.0, there are several issues with consuming .NET Standard libraries that were built for those versions from .NET Framework 4.6.1 projects. For .NET Framework projects that need to use such libraries, we recommend that you upgrade the project to target .NET Framework 4.7.2 or higher.

I'm curious to see how fast the builds/tests are on here though with the full suite as they seem to be rocket powered looking at the individual numbers.

build.ps1 Outdated Show resolved Hide resolved
Directory.Build.props Show resolved Hide resolved
tests/ImageSharp.Tests/ImageSharp.Tests.csproj Outdated Show resolved Hide resolved
@antonfirsov
Copy link
Member

I really want to do a deeper review on this during the weekend.

For now, only one comment:
If we drop test execution on .NET 4.6.2, we should explicitly state that that platform is no longer supported, and bugs reported against that platform will not be fixed.

build.ps1 Outdated Show resolved Hide resolved
@JimBobSquarePants JimBobSquarePants requested a review from a team January 16, 2020 13:21
.gitignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@brianpopow brianpopow left a comment

Choose a reason for hiding this comment

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

looks good to me, good job @tocsoft and @JimBobSquarePants

@antonfirsov
Copy link
Member

antonfirsov commented Jan 17, 2020

There are massive changes that may effect local dev experience. Gonna check the branch tonight with Rider.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Great job with the CI! 🎉

But I have to disagree with the sudden introduction of StyleCop in tests. Having 7632 😱 warnings completely hangs my Rider, making it impossible to edit stuff.

We should introduce rules incrementally in separate PR-s, including fixes for the rules being introduced.

@JimBobSquarePants
Copy link
Member

You and your toy IDE 😝

It’s a blanket off and on case with stylecop. You have to specify the rules and turn them off individually.

I’ll disable for now in tests and re-add them with another PR fixing them. It would only take me a couple of hours.

@brianpopow
Copy link
Collaborator

You and your toy IDE 😝

It’s a blanket off and on case with stylecop. You have to specify the rules and turn them off individually.

I’ll disable for now in tests and re-add them with another PR fixing them. It would only take me a couple of hours.

Im not sure if, we should enable stylecop rules in tests at all. Myself i would not care about it, but i had some colleagues in the past who really disliked those rules. I think writing tests is a chore by itself and we should not make it harder by enforcing style rules on them. I think its ok, if its a bit of a wild west in the tests.

@JimBobSquarePants
Copy link
Member

Im not sure if, we should enable stylecop rules in tests at all. Myself i would not care about it, but i had some colleagues in the past who really disliked those rules. I think writing tests is a chore by itself and we should not make it harder by enforcing style rules on them. I think its ok, if its a bit of a wild west in the tests.

The rules are much less strict than the main src and we can tweak them as need be. We need something though. The tests are difficult to follow at points which is dangerous.

@brianpopow
Copy link
Collaborator

I’ll disable for now in tests and re-add them with another PR fixing them. It would only take me a couple of hours.

ok then, if you want i can do that. I have some free time next week, i would not mind to do that.

@antonfirsov
Copy link
Member

I think even relaxed ruleset is worth a review to make sure they don't harm readability by eg. enforcing rules on Member Data formatting which are impractical here. (There might be no such case in practice - just an example to emphasize that we should be careful.)

Fortuanately StyleCop's automatic formatters seem to work much better today (at least with Rider). A few years ago, when I had to fix everything manually, it was a terrible & frustrating productivity killer for me. I used to fight until my last blood to keep SC far from the test code 😄

codecov: false
- os: windows-latest
framework: netcoreapp3.1
runtime: -x64
Copy link
Member

@antonfirsov antonfirsov Jan 18, 2020

Choose a reason for hiding this comment

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

It's worth to also test x86 + 3.1 because of the floating point magic we do.

@JimBobSquarePants JimBobSquarePants merged commit 00e9093 into master Jan 18, 2020
@JimBobSquarePants JimBobSquarePants deleted the sw/github-actions branch January 18, 2020 00:27
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 24, 2020
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.

4 participants