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

Fixed dotnet build in benchmarks #865

Merged
merged 3 commits into from
Jun 17, 2021
Merged

Conversation

ujjwalchadha
Copy link
Contributor

This PR fixes the benchmark.cmd errors that occur due to presence of vcxproj files in the programatical dotnet build process. It uses the dll files directly built in the msbuild step and then skips the project files inclusion by introducing IsDotnetBuild prop.

@ujjwalchadha ujjwalchadha requested a review from manodasanW June 10, 2021 02:40
<ProjectReference Include="..\Projections\Windows\Windows.csproj" Condition="$(UseWinmd) == false" />
<ProjectReference Include="..\Projections\Benchmark\Benchmark.csproj" Condition="$(UseWinmd) == false" />
<ProjectReference Include="..\TestWinRT\BenchmarkComponent\BenchmarkComponent.vcxproj" Condition="$(UseWinmd) == true" />
<ProjectReference Include="..\Projections\Windows\Windows.csproj" Condition="$(IsDotnetBuild) == false" />
Copy link
Member

@manodasanW manodasanW Jun 10, 2021

Choose a reason for hiding this comment

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

does benchmark_winmd.cmd still work? Do you still need the UseWinmd check too as testing the previous built-in winmd support doesn't use the cswinrt projections?

@@ -11,6 +11,8 @@
<ApplicationManifest Condition="$(UseWinmd) == true">Benchmarks.manifest</ApplicationManifest>
<BenchmarkTargetFramework>$(TargetFramework)</BenchmarkTargetFramework>
<BenchmarkTargetFramework Condition="'$(BenchmarkTargetFramework)' == 'netcoreapp2.0'">netstandard2.0</BenchmarkTargetFramework>
<IsDotnetBuild Condition="'$(IsDotnetBuild)' == ''">false</IsDotnetBuild>
<LangVersion Condition="$(IsDotnetBuild) == true">9.0</LangVersion>
Copy link
Member

Choose a reason for hiding this comment

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

indenting is off.

Copy link
Member

Choose a reason for hiding this comment

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

@ujjwalchadha, please change your editor setting to use spaces instead of tabs

@ujjwalchadha ujjwalchadha requested a review from manodasanW June 11, 2021 22:39
@ujjwalchadha ujjwalchadha merged commit ea43282 into master Jun 17, 2021
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.

3 participants