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 central package management and update all references #2508

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

satano
Copy link
Contributor

@satano satano commented Nov 15, 2024

Changes in this PR:

  • Set up some code style in .editorconfig.
  • Consolidate all references into Directory.Packages.props.

Fixing tests

After switching to .NET Framework 4.7.2, MigrationToolVersionInfoTests were failing. It seems, that in this framework when running tests inside Visual Studio. The problem was in MigrationToolVersionInfo constructor, where Assembly.GetEntryAssembly() returned null and so FileVersionInfo.GetVersionInfo throwed an exception. The problem was, that FakeMigrationToolVersionInfo inherited from MigrationToolVersionInfo and so that constructor was automatically called. I believe, this logic is not needed in fake class, so I removed inheritance and implemented just the interface.

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2024

CLA assistant check
All committers have signed the CLA.

@MrHinsh
Copy link
Member

MrHinsh commented Nov 15, 2024

This one will take a little longer to review as 8.0 was introduced deliberately.

We are moving to 8.0 for all projects, and some dual build both an 8.0 and 4.7.2 output. As many projects that can use 8 should use 8, and dual build for dependancys.

A pending task for me is to merge MigrationTools.ConsoleFull and MigrationTools.ConsoleCore into a single entry that builds both 8.0 and 4.7.2!

Copy link
Member

@MrHinsh MrHinsh left a comment

Choose a reason for hiding this comment

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

Everything else apart from the TargetFramework changes are good...

Would it be better to move everything to <TargetFrameworks>net472;net8.0</TargetFrameworks> except MigrationTools.Clients.TfsObjectModel and MigrationTools.Clients.TfsObjectModel.Tests as they depend on the TFS Object Model which is net472 only.

@@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

This is a .NET Core app and must be built in 8.0

@@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

This should be 8.0

@@ -1,16 +1,16 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

The netstandard2.0 projects are shared between the net8.0 and net472... there is probably a better way to use <TargetFrameworks>net472;net8.0</TargetFrameworks> instead

@MrHinsh
Copy link
Member

MrHinsh commented Nov 15, 2024

Create a branch for the change before (Use .NET Framework 4.7.2 for almost all projects its likely that the build will succeed... and we can get those awesome changes incorporated.

@satano
Copy link
Contributor Author

satano commented Nov 15, 2024

Added two more commits:

  • Return back .NET 8.0 for projects.
  • Updated some more references with high severity vulnerabities.

@satano satano changed the title Use .NET Framework 4.7.2 for (almost) all projects, update references Use central package management and update all references Nov 15, 2024
@satano
Copy link
Contributor Author

satano commented Nov 16, 2024

I one before the las commit a47eb7a some tests failed. So I reverted back the project files (instead of netstandard i used net472), but the build checks did not finished yet. It has beet 20 our so far. Does it really takes such a long time, or is there some problem?

The other thing is, that I have a "works on my machine" problem. Because I always run tests and all of them pass. But some of them faile in CI pipeline.

@MrHinsh
Copy link
Member

MrHinsh commented Nov 16, 2024

@satano I need to approve external build runs...

@MrHinsh
Copy link
Member

MrHinsh commented Nov 16, 2024

@satano
Copy link
Contributor Author

satano commented Nov 19, 2024

Well. I do not know why some tests are failing on CI build, so I returned back netstandard2.0 and all pacakge versions (so packages are not updated). The only thing is, that the project uses centra package management for better handling references and unneeded direct references are removed. Hope it will pass.

@satano
Copy link
Contributor Author

satano commented Nov 19, 2024

OK. Last attempt. I reverted everything (even using of non-needed dependencies), just used central package management.

@satano
Copy link
Contributor Author

satano commented Nov 19, 2024

main branch has changed, so I rebased this one.

@MrHinsh
Copy link
Member

MrHinsh commented Nov 20, 2024

OK. Last attempt. I reverted everything (even using of non-needed dependencies), just used central package management.

Its always a good idea to have separate discreet changes :) ! This codebase is quite complex and has a number of quirks.

@MrHinsh MrHinsh merged commit c8278f2 into nkdAgility:main Nov 20, 2024
6 checks passed
@satano satano deleted the net472 branch November 20, 2024 12:27
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.

3 participants