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

Added support for .NET 8 #1137

Merged
merged 5 commits into from
Feb 3, 2024
Merged

Added support for .NET 8 #1137

merged 5 commits into from
Feb 3, 2024

Conversation

paulushub
Copy link
Contributor

@paulushub paulushub commented Jan 25, 2024

Description

  • Updated test and sample projects to support .NET 8
  • Fixed compile warnings and errors including CS8352, CA1416
  • Initial update to the tests workflow (runtests.yml)
  • Removed previously deleted items from the solution file.

Type of change

  • Fixing the CS8352 compiler error requires breaking changes to CoordinateParser (StringParser is internal)
  • This change does require a documentation update
  • Updates to runtest.yml, includes
    • Dotnet runtime upgrade to .NET 8 for benchmark.
    • For the unit-tests, .NetCore 3.1 runtime is maintained in addition to .NET 8
    • actions/checkout version v2 to v4
    • actions/cache version v1 to v4
    • actions/setup-dotnet versions v3 to v4
    • NUnit.ConsoleRunner version 3.10.0 to 3.17.0
  • The unit-tests now run in release configuration similar to the benchmark
  • Update revision notes
  • Resolved review issues.

How Has This Been Tested?

  • SvgW3CTestRunner is tested for each supported platform
  • Svg.UnitTests tested on command line for each supported platform
  • Svg.Benchmarks tested on command line for each supported platform, warnings are as before will be addressed later.

- Updated test and sample projects to supports .NET 8
- Fixed compile warnings and errors including CS8352, CA1416
- Initial update to the tests workflow (runtests.yml)
- Removed previously deleted items from the solution file.
- Added .NET 3.1
- Removed include-prerelease: false
- Upgraded actions/cache to v4 (from v1)
- Run tests in release mode (as the benchmark)
- Upgraded the actions/checkout (v2 to v4)
- nuget restore is currently the longest running work on the workflow.
- initial attempt to improve it
Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Thanks - this looks good to me, though release notes (especially related to the API change) are missing. I think the API change will not have a large impact and is ok, though would like to hear another opinion (@H1Gdev ?).

steps:
- name: Checkout source
uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

We could configure the dependabot to take care of these, similar to this one.

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 actions we only need to specify the major version change, so I think the dependabot might not be needed.

Copy link
Member

Choose a reason for hiding this comment

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

Not needed, but helpful... otherwise you have to manually follow these version changes (this was just an observation, not related to this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, the idea is good and it is a useful service.
Just evaluating how useful it will be in this case since all minor and patch/revision are automatically picked up.
Maybe a monthly or quarterly time span might be useful - will look into it when we add release workflow.

@paulushub paulushub self-assigned this Jan 25, 2024
@paulushub
Copy link
Contributor Author

I think the API change will not have a large impact and is ok

Bump the release version to 3.5.0 (from 3.4.6), and anyone upgrading would understand.

@mrbean-bremen
Copy link
Member

Bump the release version to 3.5.0 (from 3.4.6), and anyone upgrading would understand.

Sure, but it shall appear in the release notes nevertheless.

<Configurations>Debug;Release</Configurations>
</PropertyGroup>

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it difficult to avoid it with implementation??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please can you elaborate? Cannot understand what you meant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to avoid CS0618 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to avoid CS0618 ?

Maybe stop marking codes as obsolete. Better still, state the version by which such will be removed,
and then remove the obsolete codes and tests accordingly.

On the library side (SVG-NET), we do not need the warnings.
The end-user needs the warning and should care.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, is this <NoWarn> unnecessary ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning was showing up in the workflow tests, so I turned it off in the projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how many warnings will show, but should we hide them ?

I think this is information that developers should know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having moved the tests to Release configuration, we can release the warning in the Debug mode.
Knowing is not sufficient, there must be a plan and documentation of obsolete codes.

@paulushub paulushub merged commit 02518c4 into svg-net:master Feb 3, 2024
8 checks passed
@paulushub paulushub mentioned this pull request Feb 3, 2024
github-actions bot pushed a commit that referenced this pull request Feb 3, 2024
… Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Added support for .NET 8 - Updated test and sample projects to supports .NET 8 - Fixed compile warnings and errors including CS8352, CA1416 - Initial update to the tests workflow (runtests.yml) - Removed previously deleted items from the solution file. CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Fixing workflow errors and warnings - Added .NET 3.1 - Removed include-prerelease: false - Upgraded actions/cache to v4 (from v1) CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Update runtests.yml - Run tests in release mode (as the benchmark) - Upgraded the actions/checkout (v2 to v4) CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Add test solutions to reduce unnecessary nuget package restore - nuget restore is currently the longest running work on the workflow. - initial attempt to improve it CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Removed CS0618 in debug mode, updated release notes.
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.

3 participants