-
Notifications
You must be signed in to change notification settings - Fork 266
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
Conversation
There was a problem hiding this 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
Line 160 in e67780b
<TargetFrameworks>net6.0;net462</TargetFrameworks> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 =)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 👍
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. |
There was a problem hiding this 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?
There was a problem hiding this comment.
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. 👍
changes:
note: no changes in product, no need to release new nuget package