chore: Add validator to check .NET SDK as minimum requirements#3194
chore: Add validator to check .NET SDK as minimum requirements#3194filzrev wants to merge 2 commits into
Conversation
e1256e8 to
44aa07e
Compare
| stringBuilder.AppendArgument($"--artifacts-path .artifacts"); | ||
|
|
||
| if (!isRestore) | ||
| stringBuilder.AppendArgument($"--output \"{artifactsPaths.BinariesDirectoryPath.ToRelativePath(artifactsPaths)}\""); |
There was a problem hiding this comment.
I thought we don't need to pass --output with artifacts path?
There was a problem hiding this comment.
I also agree --output argument should be removed from command line.
(This setting output all project dependencies output to single directory and cause confilecs)
Though, it will be handled on another PR.
artifactsPaths.BinariesDirectoryPath value is set on early stage of GenerateProjectAsync.
and this path also used on GenerateAppConfigAsync
So BinariesDirectoryPath setting itself will not be changed.
Instead, following steps are expected.
- Gets
TargetDirproperty(.artifacts\bin\BenchmarkDotNet.Autogenerated\release_net8.0) on project build phase. - Copy output files from
TargetDirtoBinariesDirectoryPathwhen build succeeded.
| } | ||
|
|
||
| if (isPublish) | ||
| stringBuilder.AppendArgument($"/p:PublishDir=\"{artifactsPaths.PublishDirectoryPath}\""); |
There was a problem hiding this comment.
I think we can also omit PublishDir since we're always using artifacts path, the toolchains that use it can update their location (.artifacts/publish instead of bin/publish).
There was a problem hiding this comment.
Publish target directory also needs MSBuild project evaluation to determine actual value.
(e.g. it's resolved to artifacts\publish\ConsoleApp7\release_net10.0)
So it seems better to use fixed path that relative to BuildArtifactsDirectoryPath.
There was a problem hiding this comment.
It seems the same issue with non-publishing toolchains for the bin directory. If we're removing --output, we should unify the logic for both. It seems pretty straight forward for us to construct that path since we're generating the project name, and we have the tfm and configuration.
There was a problem hiding this comment.
It looks like we could also add <ArtifactsPivots></ArtifactsPivots> property to the csproj to strip the release_net10.0 part without affecting dependencies, so we can just build the path from the generated project name (we could also set ArtifactsProjectName, but it'd be redundant).
There was a problem hiding this comment.
It seems the same issue with non-publishing toolchains for the bin directory. If we're removing --output, we should unify the logic for both.
It'll be expected to be handled on another PR.
It looks like we could also add property to the csproj
I'll create another PR to supports custom props/targets on benchmark project build.
/p:PublishDir setting will be removed by using this feature.
Additionally, ErrorOnDuplicatePublishOutputFiles setting also should be removed.
| this StringBuilder stringBuilder, | ||
| ArtifactsPaths artifactsPaths, | ||
| bool isRestore = false, | ||
| bool isBuild = false, |
There was a problem hiding this comment.
This parameter is being added for API consistency between commands.
It'll be removed on another PR if it's not expected to be used.
This PR include following changes that are commented on #3191
ArtifactsPathWhat's changed in this PR
1.
DotNetSdkValidator.csAdd logics to validate
dotnet --versioncommand results.When SDK major version is lower that
8. It raise validation error.This might be Breaking Changes who running benchmarks on environment that install only .NET 7 SDK or earlier.
2.
BenchmarkRunnerClean.csRemove unneeded sequencial build logics.
When using .NET 8 SDK or later,
ArtifactsPathfeature can be used for older TFMs.3.
DotNetCliCommand.cs1. Set initial capacity for StringBuilder.
2. Add
ToRelativePathhelper method. And shorten path argument.This helper method returns relative path when specified path starts with
BuildArtifactsDirectoryPath(It's used as
WorkingDirectoryon DotNetCliCommandExecutor)Note: MSBuild parameter(
/p:) is leaved as is. Because it's propagated to dependent project and path is evaluated based on project file directory.3. Modify
GetMsBuildBinLogArgumentquotes location4. Rename
MaybeAppendOutputPathstoMaybeAppendExtraArgumentsand modify logicsOutDir/OutputPathparameters.Path.AltDirectorySeparatorCharaddition.