Framework-dependent System.Drawing.Common version by Kritner · Pull Request #1310 · ScottPlot/ScottPlot

Kritner · 2021-10-01T21:22:41Z

Purpose:

As a result of https://blog.kritner.com/2021/10/01/dotnet-centralized-package-versioning/ and chatting with Scott on slack ;)
I can't seem to build all of the projects, mostly around the pieces that require framework and/or windows based functionality. I don't know if this is something you would be able to check off @swharden

@swharden

* Think it's everything except the Version.bat tools, will get that in the next commit. * As a result of https://blog.kritner.com/2021/10/01/dotnet-centralized-package-versioning/ and chatting with Scott on slack ;) * I can't seem to build all of the projects, mostly around the pieces that require framework and/or windows based functionality. I don't know if this is something you would be able to check off @swharden

….props * Now uses a variable for the version across the other csproj files

…oke the previous xml traversal, corrected. * There's gotta be a better way for identification of property groups and/or relying on the element we're looking for to be in the correct "ordinal" property group, but eh?

swharden · 2021-10-01T23:21:59Z

Hi @Kritner, thanks for sharing this! See my notes in #1311

~~If you remove the draft status of this PR you can confirm the CI/tests pass and I won't merge it until you're ready~~

EDIT: my bad, I have to manually approve CI runs for first-time contributors

…as per ScottPlot#1311 * You can upgrade a version for specified csproj files by utilizing "OverrideVersion" on the included package for the desired csproj file.

swharden · 2021-10-01T23:32:39Z

I can't seem to build all of the projects, mostly around the pieces that require framework and/or windows based functionality

This might make this difficult 😅

We exert a lot of effort to maintain cross-targeted builds across .NET Framework (back to 4.6.1), .NET Standard (2.0), and .NET 5 so the library runs on Windows, MacOS, and Linux.

If ManagePackageVersionsCentrally isn't supported with those earlier targets, a less fancy solution like csproj variables is still an improvement. I'm interested in learning more about this topic!

swharden · 2021-10-01T23:41:18Z

a less fancy solution like csproj variables is still an improvement

If this is the case I think we really only need to use central versioning for:

ScottPlot - the four csproj files linked in Central version management #1311
Avalonia, Avalonia.Desktop, and Avalonia.Diagnostics
System.Drawing.Common (keep at 4.6.1)
- related: Which version of System.Drawing.Common should ScottPlot depend on? #1004
- related: System.Drawing.Common version downgrade #1005

The rest can remain hard-coded in the csproj files.

Kritner · 2021-10-01T23:47:13Z

I can't seem to build all of the projects, mostly around the pieces that require framework and/or windows based functionality

This might make this difficult 😅

We exert a lot of effort to maintain cross-targeted builds across .NET Framework (back to 4.6.1), .NET Standard (2.0), and .NET 5 so the library runs on Windows, MacOS, and Linux.

The only sandbox app I can't build is "WinFormsFrameworkApp". Here are outputs from the other demo apps:

WPF:

WinForms:

Console:

Avalonia Demo:

FWIW, I ran dotnet format but that seems to be failing the CI / Check Formatting step. Additionally all tests are passing locally, I'll look at which one failed in that part of the process.

System.Drawing.Common (keep at 4.6.1)

related: Which version of System.Drawing.Common should ScottPlot depend on? Which version of System.Drawing.Common should ScottPlot depend on? #1004

related: System.Drawing.Common version downgrade System.Drawing.Common version downgrade #1005

Addressed in 2ccae4f

Puts the central version to 4.6.1, individual packages can be upgraded with OverrideVersion in their respective csproj file if desired. (I'm assuming no newer features were actually required, so I just kept everything at 4.6.1)

swharden · 2021-10-01T23:48:04Z

@swharden should the package version be revved within this PR to *.25? or is that a part of the build process and/or rollup of several features into a new version?

Let's leave all the ScottPlot versions what they are. After merging this PR I'll automate the process of bumping them using GitHub Actions 👍

Kritner · 2021-10-01T23:53:20Z

@swharden should the package version be revved within this PR to *.25? or is that a part of the build process and/or rollup of several features into a new version?

Let's leave all the ScottPlot versions what they are. After merging this PR I'll automate the process of bumping them using GitHub Actions 👍

Sounds good, current branch is still set at version 4.1.24. I think that's everything, except for the CI / Check Formatting step (which I did run the formatting...) and The failing Test on Linux: https://github.com/ScottPlot/ScottPlot/pull/1310/checks?check_run_id=3773009138#step:6:23

swharden · 2021-10-01T23:56:20Z

The only sandbox app I can't build is "WinFormsFrameworkApp". Here are outputs from the other demo apps:

This is encouraging! As far as building/testing I'm happy to lean on the CI system.

Test_LettersDontRenderAsRectangles_SerifFont

Thanks again for your work on this PR! Sorry it got complicated by this 😝 This seems related to #1079 so let's disable this test to keep this PR moving and we can solve that issue before merging.

☠️ It looks like the Linux tests are failing due to font rendering issues. This may be because GitHub (the system that runs on the cloud) just today changed their default supported font set. To get this PR un-stuck you can either:

Option 1: edit the test that fails on Linux and add a statement something like:

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) Assert.Pass();

Option 2: In ci.yaml add an indented line after strategy: with fail-fast: false to allow the Windows tests to run even if the Linux tests fail.

* ScottPlot#1310 (comment)

swharden · 2021-10-02T00:17:26Z

I think the new errors indicate the package versioning isn't working as expected 😅

The font rendering error was also a result of this actually... if the wrong version of System.Drawing.Common is installed on Linux, characters render as squares, and that test fails: #1079 (comment)

Let's ignore autoformatting for now

@swharden

… framework * @swharden wants this to be an OS switch rather than framework switch, gotta get the kids to bed though! I'll try to pick this up tomorrow

when run on Windows the autoformatter didn't find any issues

Previously the format check was run on Linux which has more strict whitespace formatting requirements ScottPlot#1310

previously disabled because of System.Drawing.Common versioning issues ScottPlot#1310 ScottPlot#1311

previously the conditional logic only supported .NET 5 (not .NET 6+)

bclehmann

Looks good to me, although that test does bug me. Especially as Linux font rendering on GDI+ seems to be more fragile than other OSes, and we're already rather dependent on bug reports to catch Linux issues.

.github/workflows/ci.yaml

Directory.Build.props

src/tests/Misc/Drawing.cs

.props files were stored in /dev for when central versioning tools mature to allow mixed-package solutions and version wildcards

related ScottPlot#1310, ScottPlot#1004

an additional job builds the pages using the *.NUGET.csproj files instead of the default project files used by the solution file

swharden · 2021-10-02T04:28:24Z

Wow, this was a rollercoaster ride! 🎢🎃

I did not side with centralized dependency versioning

It couples packages more than I desire - the solution file is used for the convenience of the developer, but packages should be largely self-contained and isolated from the rest of the stuff in this repository.
Mixed projects are not supported - I wish I could centrally version just System.Drawing.Common but it doesn't look like that's possible without centrally versioning everything, or requiring building from the solution (I want the nuget packages to be independently buildable because it's often hard to build mixed-target solutions on cloud platforms)
Wildcard versioning is currently unsupported. Allowing a version like 0.10.* is pretty helpful.

I learned a lot through this experience, including not to rush to adopt preview features 😅

I moved the .props files to /dev/ so we might be able to use them later when the .NET tools mature

The CI pipeline improved

Tail-fast is now disabled, allowing more thorough assessments of whether failing tests fail on all operating systems or not
The nuget packages are built from separate csproj files than the ones used by the solution file, so a new job was added to test building those and storing them as artifacts so you can download/inspect/test them. They're only retained for 24 hours to keep storage utilization low.

Conditional `System.Drawing.Common` version

Unless I goofed and haven't realized it yet, I think this is a big win of this PR. The System.Drawing.Common versioning problems (#1004) are limited to what happens when .NET Framework users downgrade their package version. Before this PR we just targeted the lowest version, but that causes poor font rendering on linux (#1079). This new discussion led to the following in the nuget package csproj files, which I think solves the problem by allowing the latest version to be distributed to everyone except .NET Framework users (though they still have the ability to upgrade on their end if they wish).

ScottPlot/src/controls/ScottPlot.WinForms/ScottPlot.WinForms.NUGET.csproj

Lines 38 to 47 in 4895957

    
             <ItemGroup> 
        
               <!--  
        
                 Logic here ensures .NET Framework apps get the older (4.6.1) package by default. 
        
                 Upgrading may improve font rendering on Linux and MacOS, but upgrading then downgrading  
        
                 is associated with assembly issues that break .NET Framework projects on Windows. 
        
                 https://github.com/ScottPlot/ScottPlot/issues/1004 
        
               --> 
        
               <PackageVersion Condition="!$(TargetFramework.StartsWith('net4'))" Include="System.Drawing.Common" Version="5.0.2" /> 
        
               <PackageVersion Condition="$(TargetFramework.StartsWith('net4'))" Include="System.Drawing.Common" Version="4.6.1" /> 
        
             </ItemGroup>

swharden added the Hacktoberfest label Oct 1, 2021

Kritner added 2 commits October 1, 2021 19:09

Updates "Versioner" to update the version only in the Directory.Build…

a423254

….props * Now uses a variable for the version across the other csproj files

swharden linked an issue Oct 1, 2021 that may be closed by this pull request

Central version management #1311

Closed

Applies dotnet format to repository as per contribution guideline

1a75861

swharden requested a review from bclehmann October 1, 2021 23:22

Downgrades System.Drawing.Common to 4.6.1 for centralized versioning …

2ccae4f

…as per ScottPlot#1311 * You can upgrade a version for specified csproj files by utilizing "OverrideVersion" on the included package for the desired csproj file.

This comment has been minimized.

Sign in to view

Skip run of linux failing drawing test

9d7ca31

* ScottPlot#1310 (comment)

This comment has been minimized.

Sign in to view

Kritner marked this pull request as ready for review October 2, 2021 00:08

swharden and others added 2 commits October 1, 2021 20:13

GitHub Actions: disable fail fast for test matrix

5ef4c4b

Ack, my namespace import didn't save

7597170

swharden and others added 2 commits October 1, 2021 20:30

autoformat

464faef

Updates System.Drawing.Common to use a different version depending on…

c1c2488

… framework * @swharden wants this to be an OS switch rather than framework switch, gotta get the kids to bed though! I'll try to pick this up tomorrow

swharden changed the title ~~Start of centralized package versioning~~ Centralized package and dependency versioning Oct 2, 2021

swharden added 4 commits October 1, 2021 21:12

autoformat on Linux (WSL)

eef4df4

when run on Windows the autoformatter didn't find any issues

CI: validate code format using Windows

27f680a

Previously the format check was run on Linux which has more strict whitespace formatting requirements ScottPlot#1310

Tests: re-enable character validation on Linux

c34cd0c

previously disabled because of System.Drawing.Common versioning issues ScottPlot#1310 ScottPlot#1311

Packages: Improve target framework detection

55193b0

previously the conditional logic only supported .NET 5 (not .NET 6+)

bclehmann approved these changes Oct 2, 2021

View reviewed changes

.github/workflows/ci.yaml Show resolved Hide resolved

Directory.Build.props Outdated Show resolved Hide resolved

src/tests/Misc/Drawing.cs Outdated Show resolved Hide resolved

swharden added 8 commits October 1, 2021 23:51

Packages: define versions in csproj files

8cdec7d

.props files were stored in /dev for when central versioning tools mature to allow mixed-package solutions and version wildcards

dev: use original versioning tool

2694857

dev: central version props notes

373af05

winforms csproj: 4-digit assembly and file versions

06c8031

csproj: System.Drawing.Common framework dependent on framework

47eacf1

related ScottPlot#1310, ScottPlot#1004

GitHub Actions: Build NuGet packages

01e16af

an additional job builds the pages using the *.NUGET.csproj files instead of the default project files used by the solution file

Update changelog.md

f96fbd8

GitHub Actions: retain CI packages for 24hr

4895957

swharden approved these changes Oct 2, 2021

View reviewed changes

swharden changed the title ~~Centralized package and dependency versioning~~ Framework-dependent System.Drawing.Common version in NuGet packages Oct 2, 2021

swharden merged commit fd746b0 into ScottPlot:master Oct 2, 2021

swharden changed the title ~~Framework-dependent System.Drawing.Common version in NuGet packages~~ Framework-dependent System.Drawing.Common version Oct 2, 2021

swharden mentioned this pull request Feb 17, 2022

Tests: Support Docker #1677

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework-dependent System.Drawing.Common version#1310

Framework-dependent System.Drawing.Common version#1310
swharden merged 22 commits intoScottPlot:masterfrom
Kritner:FEATURE/centralPackageVersioning

Kritner commented Oct 1, 2021 •

edited

Loading

Uh oh!

swharden commented Oct 1, 2021 •

edited

Loading

Uh oh!

swharden commented Oct 1, 2021

Uh oh!

swharden commented Oct 1, 2021

Uh oh!

Kritner commented Oct 1, 2021

Uh oh!

swharden commented Oct 1, 2021

Uh oh!

Kritner commented Oct 1, 2021

Uh oh!

swharden commented Oct 1, 2021 •

edited

Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

swharden commented Oct 2, 2021

Uh oh!

bclehmann left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swharden commented Oct 2, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Kritner commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swharden commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swharden commented Oct 1, 2021

Uh oh!

swharden commented Oct 1, 2021

Uh oh!

Kritner commented Oct 1, 2021

Uh oh!

swharden commented Oct 1, 2021

Uh oh!

Kritner commented Oct 1, 2021

Uh oh!

swharden commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test_LettersDontRenderAsRectangles_SerifFont

Uh oh!

This comment has been minimized.

This comment has been minimized.

swharden commented Oct 2, 2021

Uh oh!

bclehmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swharden commented Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I did not side with centralized dependency versioning

The CI pipeline improved

Conditional System.Drawing.Common version

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kritner commented Oct 1, 2021 •

edited

Loading

swharden commented Oct 1, 2021 •

edited

Loading

swharden commented Oct 1, 2021 •

edited

Loading

swharden commented Oct 2, 2021 •

edited

Loading

Conditional `System.Drawing.Common` version