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

add .Net6 support across codebase #732

Merged
merged 39 commits into from
Mar 4, 2023
Merged

Conversation

martijn00
Copy link
Contributor

This removes dependencies on old libraries: fixes #725

@martijn00
Copy link
Contributor Author

@ebozduman @harshavardhana would you mind giving this a review? I can make changes if required.

@ebozduman
Copy link
Collaborator

@martijn00
Sure. Will do.

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

@martijn00 ,

There are lint errors.
Please run dotnet regitlint and then push those lint changes

@martijn00
Copy link
Contributor Author

@ebozduman Done

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

@martijn00

Please check the build test run failure logs.
There are quite a lot of warnings and errors, which need to be addressed, for Minio.Tests.

@martijn00
Copy link
Contributor Author

@ebozduman Did you look at the error? It seems the connection get refused. Are the credentials still correct?

These test fail on master branch too!

This is currently used:

        _minioClient = new MinioClient()
            .WithEndpoint("play.min.io")
            .WithCredentials("Q3AM3UQ867SPQQA43P2F",
                "zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG")

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

@martijn00 ,

With your recent changes, it is back to lint errors in 38 files.
No need to say it, but we need to run dotnet regitlint before pushing any change.

@martijn00
Copy link
Contributor Author

@ebozduman I've updated the PR again. My question still remains: It seems the connection get refused. Are the credentials still correct?

I don't think the Test will ever work if that is not fixed on your side.

@martijn00
Copy link
Contributor Author

@ebozduman I've switch the failing test over to use ssl, and now they run. Can you have a look again?

@martijn00
Copy link
Contributor Author

@ebozduman Locally I have everything running. Can you try run the tests again with the latest change? See if that helps

I still see lots of room for improvement but i don't want to make this PR bigger than it already is. Let's work together to get this merged, and then continue from there.

@ebozduman
Copy link
Collaborator

@martijn00
Sorry for forgetting to answer your question;

My question still remains: It seems the connection get refused. Are the credentials still correct?

Yes, there was a change in play's setup. We were supposed to hit https, and you are right; this issue does not belong to your change.
So, I've created a separate PR, #749, and asked expedited review from the reviewer.

@ebozduman
Copy link
Collaborator

@martijn00,

Please rebase. PR#749 is merged.

@martijn00
Copy link
Contributor Author

@ebozduman can you try again now?

Any other comments or remarks, or things you want explanation?

@ebozduman
Copy link
Collaborator

ebozduman commented Mar 1, 2023

Hey @martijn00 ,

I've a couple of comments to share;

1. I see some warning messages about the removed packages, like System or System.Collections.Generic. But the workflow build test for 7.0 passes. There maybe something wrong in my env. I'll check it out.
Yes. It was the OmniSharp process and it was giving me wrong warning messages. It needed to be restarted.

  1. The PASS/FAIL output messages are now prettified with indentations, and etc.
    I think the default should have been the regular one-liner outputs as before for regression purposes. If a user wants to see the prettified output, s/he can turn it on.
    Could you revert it back, so that we can see one-liner test result output for each test ?

  2. I am also going to add .Net6.0 as part of the build process and testing. It is a required step.

@martijn00
Copy link
Contributor Author

@ebozduman Ok, done comment 1. When can you get to comment 2?

@ebozduman
Copy link
Collaborator

@martijn00 Done. Build tests are running right now

@ebozduman
Copy link
Collaborator

@martijn00

I forgot to ask. Why is Docs/API.md removed?

@martijn00
Copy link
Contributor Author

@ebozduman not sure why. I added it back now.

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana changed the title Update to .Net6 add .Net6 support across codebase Mar 4, 2023
@harshavardhana harshavardhana merged commit b332b78 into minio:master Mar 4, 2023
@ebozduman
Copy link
Collaborator

Hi @martijn00

I wanted to let you know that we missed to check 2 areas for this PR and there we now see regression;

  1. The release script that creates the MinIO nupackage is broken as the script expects to find a Version tag, the version of the next release information, in the Minio.csprpoj file.

  2. MinIO supports not only .Net6.0 and .Net7.0 , but also .Net4.x.
    So, we were supposed to run the functional tests against .Net4.x release(s).
    I see quite a lot of build errors when I run the functional tests against .Net4.7.2 or .Net4.8, mostly because some new features introduced in .Net6.0 are not available for .Net4.x and the versions of some packages are not comopatible with .Net4.x.

Please let us know what your opinion is about these 2 regressions and possible fixes for them.
Thanks

@martijn00
Copy link
Contributor Author

@ebozduman Where can i find this script? The version is now here: https://github.com/minio/minio-dotnet/blob/master/Directory.Build.props#L19 It should be possible to point it there?

Looking at the LTS versions of .NET it would be best to drop support for .NET4.x: https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core

Are there any compelling reasons to still support really old frameworks like that?

Also can you look at: #754

@ebozduman
Copy link
Collaborator

The version is now here: https://github.com/minio/minio-dotnet/blob/master/Directory.Build.props#L19 It should be possible to point it there?

Yes, the script'll point to Directory.Build.props.
We'll take care of the script change The release script is in a private repo.

Abour .Net4.x:
We are also discussing the drop of .Net4.x support.

I'll get to #754 as soon as I can.

@ebozduman
Copy link
Collaborator

ebozduman commented Mar 15, 2023

@martijn00

re: .Net4.x support
Even if we decide to drop the support for .Net4.x, we still need to make the changes as it will take some time to drop it after announcement.

@martijn00
Copy link
Contributor Author

What sort of changes? Also, users can still use older nuget packages for net4.

@ebozduman
Copy link
Collaborator

What sort of changes?

To make it work with the latest MinIO nuget package.

Also, users can still use older nuget packages for net4.

Yes they can, but it is a workaround, which means we don't support .Net4.x

@martijn00
Copy link
Contributor Author

martijn00 commented Mar 16, 2023

But what sort of code changes are that? I can't imagine anything... Can I help with it if you could elaborate?

@ebozduman
Copy link
Collaborator

Hi @martijn00

They are build errors, like for unknown attribute names, etc.
I run MinIO functional tests against .Net4.8 and .Net4.7.2.
Some of those build errors are listed below:

$ dotnet run --project Minio.Functional.Tests/Minio.Functional.Tests.csproj 
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Credentials/ECSCredentials.cs(18,19): error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?) [/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Minio.csproj::TargetFramework=net4.8]
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Credentials/IAMAWSProvider.cs(19,19): error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?) [/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Minio.csproj::TargetFramework=net4.8]
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Credentials/JsonWebToken.cs(18,19): error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?) [/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Minio.csproj::TargetFramework=net4.8]
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/DataModel/MinioNotification.cs(18,19): error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?) [/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Minio.csproj::TargetFramework=net4.8]
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Helper/utils.cs(23,19): error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?) [/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Minio.csproj::TargetFramework=net4.8]
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/HttpRequestMessageBuilder.cs(18,18): error CS0234: The type or namespace name 'Http' does not exist in the namespace 'System.Net' (are you missing an assembly reference?) [/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/Minio.csproj::TargetFramework=net4.8]
/home/ersan/work/src/github.com/minio/minio-dotnet/Minio/V4Authenticator.cs(19,19): error CS0234: The type or namespace name 'Json' does not exist in the namespace 'System.Text' (are you missing an assembly reference?) 
:
:
:
The build failed. Fix the build errors and run again.

You just change the target framework to .net4.7.2 and .Net4.8 in Minio.Functional.Tests.csproj file and run the functional tests. You'll see the error messages.

@martijn00
Copy link
Contributor Author

@ebozduman ok, so you want to support .net4.8? I thought you are discussing dropping the support?

There is no point in maintaining net48 support anymore. You are blocking real support for .net5, 6, 7 and 8 with this.
As said before, users still on net4.8 or lower can just use the older nuget packages. If problems come up you can still release a 4.x version with support for older frameworks, while the newer Minio 5.x versions only work on newer .net.

These things are not regressions, it's just the future.

@martijn00 martijn00 mentioned this pull request Mar 23, 2023
@ebozduman
Copy link
Collaborator

I agree with all the points you've made, but we have to follow the process.
My understanding is as long as Microsoft supports, we need to continue supporting those releases and Microsoft supports 4.7 and 4.8.
We are currently discussing it.

@martijn00
Copy link
Contributor Author

If it helps, I could do a call with you and management to talk about it. Let me know if you want that.

Urantij pushed a commit to Urantij/minio-dotnet that referenced this pull request Apr 19, 2023
Co-authored-by: ebozduman <ersan.bozduman@gmail.com>
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.

Question: Is reference to Newtonsoft.Json necessary?
3 participants