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

Update build project to .net 8 #776

Merged
merged 4 commits into from May 5, 2024
Merged

Update build project to .net 8 #776

merged 4 commits into from May 5, 2024

Conversation

Romfos
Copy link
Contributor

@Romfos Romfos commented Feb 4, 2024

changes:

  • Update build project to .NET 8

note: no changes in product, no need to release new nuget package

@Romfos Romfos marked this pull request as ready for review February 4, 2024 01:05
Copy link
Member

@alexandrnikitin alexandrnikitin left a comment

Choose a reason for hiding this comment

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

Nice! Thank you.

Let's also bump the framework in our generate project that tests code from the docs

<TargetFrameworks>net6.0;net462</TargetFrameworks>

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a separate solution file in the build directory? We already have one in the root directory NSubstitute.sln

Copy link
Contributor Author

@Romfos Romfos Feb 4, 2024

Choose a reason for hiding this comment

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

because build.fs have task to build main solution
this is circular dependency. we cannot build project during run project =)

Copy link
Member

Choose a reason for hiding this comment

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

build.fs uses dotnet test Docs.csproj so it shouldn't need the sln afaict. 🤔 What's the error it's giving without this?

        let projPath = outputCodePath </> "Docs.csproj"
        FileReaderWriter.Write projPath csproj
        DotNet.restore (fun p -> p) projPath
        DotNet.build (fun p -> p) projPath
        DotNet.test (fun p -> p) projPath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no error, it just stuck at build phase if add build project to main solution

let solution = (root </> "NSubstitute.sln")
....


    Target.description("Restore dependencies")
    Target.create "Restore" (fun _ ->
        DotNet.restore (fun p -> p) solution
    )

    Target.description("Compile all projects")
    Target.create "Build" (fun _ ->
        DotNet.build (fun p ->
            { p with Configuration = DotNet.BuildConfiguration.fromString configuration
                     MSBuildParams = { p.MSBuildParams with Properties = additionalArgs }
            }) solution

Copy link
Member

Choose a reason for hiding this comment

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

I still don't follow why it's required. But happy to merge as-is. 👍

@alexandrnikitin
Copy link
Member

Let's also bump the framework in our generate project that tests code from the docs

Actually let's keep it as is because we ship for net6.0 version.

@Romfos
Copy link
Contributor Author

Romfos commented Feb 4, 2024

Yea, in general, we have 2 options:

  1. use minimal supported versions in documentation as examples
  2. use the latest version in documentation as examples

both are possible

@Romfos
Copy link
Contributor Author

Romfos commented Feb 4, 2024

about build.fxproj project:

This is very strange project, as for me, with following targets:

    Target.create "-T" <| fun _ ->
        printfn "Optional config options:"
        printfn "  configuration=Debug|Release"
        printfn "  benchmark=*|<benchmark name>  (only for Benchmarks target in Release mode)"
        printfn ""
        Target.listAvailable()

    "Clean" ?=> "Build"             |> ignore
    "Clean" ?=> "Test"              |> ignore
    "Clean" ?=> "Restore"           |> ignore
    "Clean" ?=> "Documentation"     |> ignore
    "Clean" ?=> "TestCodeFromDocs"  |> ignore
    "Clean" ?=> "Package"           |> ignore
    "Clean" ?=> "Default"           |> ignore

    "Build"         <== [ "Restore" ]
    "Test"          <== [ "Build" ]
    "Documentation" <== [ "TestCodeFromDocs" ]
    "Benchmarks"    <== [ "Build" ]
    // For packaging, use a clean build and make sure all tests (inc. docs) pass.
    "Package"       <== [ "Clean"; "Build"; "Test"; "TestCodeFromDocs" ]

    "Default"       <== [ "Restore"; "Build"; "Test" ]
    "All"           <== [ "Clean"; "Default"; "Documentation"; "Package" ]

actually I would recommend don't reinvent the wheel and use standard dotnet cli commands:

  1. dotent restore for restore
  2. dotent build for build
  3. dotent test for testing
  4. dotnet pack for packaging

single value from this project, from my point of view, is documentation verification

make sense to remove it and create project for documentation verification in main solution
but it will require to make changes in nuget package release CI, that is not on github as i see

@alexandrnikitin
Copy link
Member

@Romfos I'm sorry for the late response.
I agree with your last message. I also prefer to use dotnet CLI tool for those. It didn't exist when we used FAKE for our build.

Now regarding the PR, I tend to think that it's better to keep the TargetFramework as is (net6.0) because it's the version we release and it needs to be present in the system anyway. I'm sorry I stopped working with .NET and may not see benefits of upgrading. Please convince me otherwise.

@Romfos
Copy link
Contributor Author

Romfos commented Apr 20, 2024

@Romfos I'm sorry for the late response. I agree with your last message. I also prefer to use dotnet CLI tool for those. It didn't exist when we used FAKE for our build.

Now regarding the PR, I tend to think that it's better to keep the TargetFramework as is (net6.0) because it's the version we release and it needs to be present in the system anyway. I'm sorry I stopped working with .NET and may not see benefits of upgrading. Please convince me otherwise.

I have following logic:

  1. This build project has dependency conflicts:
    image
  2. When I started investigate - update nuget packages is the easiest way to fix it
  3. New nuget packages target .NET 8 and then I decided update build project to .NET 8

image

In any case it will be updated today or tomorrow. I will not affect target framework for package

@Romfos
Copy link
Contributor Author

Romfos commented Apr 20, 2024

hm, looks like github actions no longer build for PR's in this repo. Is it expected?

@alexandrnikitin
Copy link
Member

hm, looks like github actions no longer build for PR's in this repo. Is it expected?

It is expected. It was set to "Require approval" for all fork PRs. I think for security reasons but we don't store any secrets in GH actions. I set it back to "Require approval for first-time contributors". It should build now.

Copy link
Member

@dtchepak dtchepak left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Happy for me to merge?

Copy link
Member

Choose a reason for hiding this comment

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

I still don't follow why it's required. But happy to merge as-is. 👍

@dtchepak dtchepak merged commit 4139d6a into nsubstitute:main May 5, 2024
12 checks passed
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.

None yet

3 participants