CSHARP-4385: Consider adding Directory.Build.props. #934
Conversation
DmitryLukyanov
force-pushed
the
csharp4385
branch
from
f28cb94 to
f267189
Compare
| MinimumVisualStudioVersion = 10.0.40219.1 | ||
| Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{D2012971-32BB-4C5F-BFC4-30A9994AB152}" | ||
| ProjectSection(SolutionItems) = preProject | ||
| src\Directory.Build.props = src\Directory.Build.props |
There was a problem hiding this comment.
it's possible to add this file into solution directory, but given that configuration makes sense only for src folder, probably better to leave it where it's currently placed
src/MongoDB.Bson/MongoDB.Bson.csproj
Outdated
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.6.2" PrivateAssets="All" /> |
There was a problem hiding this comment.
we can move this to directory.build.props. If we add this reference to the legacy project. Thoughts?
JamesKovacs
requested review from
BorisDog
and removed request for
JamesKovacs
src/Directory.Build.props
Outdated
| <TargetFrameworks Condition="'$(IsWindows)'!='true'">netstandard2.0;netstandard2.1</TargetFrameworks> | ||
| <LangVersion>10.0</LangVersion> | ||
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> | ||
| <WarningsAsErrors /> |
There was a problem hiding this comment.
Looks like we don't need <WarningsAsErrors> element.
There was a problem hiding this comment.
sounds like you're right based on this
src/MongoDB.Bson/MongoDB.Bson.csproj
Outdated
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.6.2" PrivateAssets="All" /> |
tests/Directory.Build.props
Outdated
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <IsOSX Condition="$([MSBuild]::IsOSPlatform('OSX'))">true</IsOSX> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
As all IsOS* are logically I think they should reside in the same PropertyGroup section?
| @@ -1,7 +1,6 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>netcoreapp2.1;netcoreapp3.1;net472;net5.0;net6.0</TargetFrameworks> | |||
| <LangVersion>9</LangVersion> | |||
| <TargetFrameworks>$(TargetFrameworks);net5.0</TargetFrameworks> | |||
There was a problem hiding this comment.
I think tests props should not be shared with smoke tests.
Smoke tests intended to be "standalone" projects, and should not share anything with driver specific code.
We might want to consider props for all smoke tests if/when we expand them with more projects.
There was a problem hiding this comment.
I think they can (should?) share some configuration like TF to ensure that all TFs we support work well in consumer application too. But I'm ok to defer this to further tickets, reverted
| @@ -0,0 +1,52 @@ | |||
| <Project> | |||
There was a problem hiding this comment.
It looks like Directory.Build.props naming is only relevant when the props needs to be applied to all project in same directory.
Maybe is should be called Tests.Build.props and optionally the containing directory BuildProps (as it's already under tests dir)?
| <DefineConstants>TRACE</DefineConstants> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition="'$(IsWindows)'=='true'"> | ||
| <DefineConstants>$(DefineConstants);WINDOWS</DefineConstants> |
There was a problem hiding this comment.
Any idea what $(DefineConstants) means and why it's not applied to TRACE?
There was a problem hiding this comment.
in this section you can define different consts (based on condition) that then will be visible in #if construction in tests
No description provided.