Update build project to .net 8#776
Conversation
alexandrnikitin
left a comment
There was a problem hiding this comment.
Nice! Thank you.
Let's also bump the framework in our generate project that tests code from the docs
Line 160 in e67780b
There was a problem hiding this comment.
Why do we need a separate solution file in the build directory? We already have one in the root directory NSubstitute.sln
There was a problem hiding this comment.
because build.fs have task to build main solution
this is circular dependency. we cannot build project during run project =)
There was a problem hiding this comment.
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) projPathThere was a problem hiding this comment.
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
There was a problem hiding this comment.
I still don't follow why it's required. But happy to merge as-is. 👍
Actually let's keep it as is because we ship for net6.0 version. |
|
Yea, in general, we have 2 options:
both are possible |
|
about build.fxproj project: This is very strange project, as for me, with following targets: actually I would recommend don't reinvent the wheel and use standard dotnet cli commands:
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 |
|
@Romfos I'm sorry for the late response. Now regarding the PR, I tend to think that it's better to keep the |
I have following logic:
In any case it will be updated today or tomorrow. I will not affect target framework for package |
|
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. |
dtchepak
left a comment
There was a problem hiding this comment.
LGTM 👍 Happy for me to merge?
There was a problem hiding this comment.
I still don't follow why it's required. But happy to merge as-is. 👍
changes:
note: no changes in product, no need to release new nuget package