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

Farewell project json #357

Merged
merged 32 commits into from Feb 24, 2017
Merged

Farewell project json #357

merged 32 commits into from Feb 24, 2017

Conversation

@adamsitnik
Copy link
Member

adamsitnik commented Feb 3, 2017

I have ported our projects to the new csproj format (except F# for .NET Core which does not work yet).

Users don't need to do anything else than just package upgrade. We detect if they are using project.json files or the new csproj by checking the dotnet --version It means that we still support project.json files ;)

Problems:

  • today we can't easily run the test for classic .NET framework, bug reported here
  • When debugging unit tests from VS the current directory is some weird VS folder and we rely on it in few places. solution: wait for RTM
  • I could not add the new .fsproj file with samples to our .sln file because VS has another bug it should get solved for the RTM

Again big thanks to @CesarBS for help!

@dnfclas
Copy link

dnfclas commented Feb 3, 2017

Hi @adamsitnik, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 6, 2017

@adamsitnik, looks awesome! Great job!

What the current state of this PR? What else do you want to commit? Should I already try it?

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 7, 2017

@AndreyAkinshin thanks! @CesarBS helped me ;)

In general there are few issues with the tools (VS/msbuild/dotnet cli), our part (code) is ok. I thinks it's quite good (we even have now project.json/csproj auto-detection so users will not need to configure anything)

The problems:

  • VS bug solution: wait for RC4, workaround: build our projects manually in the right order
  • When debugging unit tests from VS the current directory is some weird VS folder and we rely on it in few places. solution: wait for RC4
  • our appveyor has no VS 2017 image. This will most probably require some extra configuration
  • I need to get our old classic integrations tests runnable from VS and script. This will be most probably tricky
  • can't build 100% of the solution from console. VB, F# has some wrong paths. VS can do it, probably another bug

I would like to wait for VS 2017 RC4, make sure everything works, update the docs and then release.

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 7, 2017

If you are curious you can already try it. Maybe we can add ref returns support?

using BenchmarkDotNet.Characteristics;
using BenchmarkDotNet.Environments;
using BenchmarkDotNet.Helpers;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Toolchains.DotNetCli;
using System.Reflection;
using System.Text;

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 7, 2017

nit: sort


File.WriteAllText(artifactsPaths.ProjectFilePath, content);
}

protected override string SetDependencyToExecutingAssembly(string template, Type benchmarkTarget)
{
if (!GetSolutionRootDirectory(out var solutionRootDirectory))
{
throw new NotSupportedException("Unable to find .sln or global.json file, hence can not find the csproj path");

This comment has been minimized.

Copy link
@cesarblum
var csprojs = solutionRootDirectory.GetFiles(csprojName, SearchOption.AllDirectories);
if (csprojs.Length != 1)
{
throw new NotSupportedException($"Unable to find single {csprojName} in {solutionRootDirectory} and it's subfolders. Most probably the name of output exe is different than the name of the .csproj");

This comment has been minimized.

Copy link
@cesarblum
<ItemGroup>
<ProjectReference Include="..\..\src\BenchmarkDotNet.Core\BenchmarkDotNet.Core.csproj">
<Project>{95f5d645-19e3-432f-95d4-c5ea374dd15b}</Project>
<Name>BenchmarkDotNet.Core</Name>

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 7, 2017

I don't think you need the Project and Name elements here.

<Name>BenchmarkDotNet.Core</Name>
</ProjectReference>
<ProjectReference Include="..\..\src\BenchmarkDotNet\BenchmarkDotNet.csproj">
<Project>{af1e6f8a-5c63-465f-96f4-5e5f183a33b9}</Project>

This comment has been minimized.

Copy link
@cesarblum
</PropertyGroup>
<ItemGroup>
<Content Include="xunit.runner.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 7, 2017

You can use CopyToOutputDirectory as an attribute to Content.

<GenerateDocumentationFile>true</GenerateDocumentationFile>
</PropertyGroup>

<!-- does not work ;/

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 7, 2017

You should do this in AssemblyInfo.cs - I don't think it's a good idea to share it across all projects.

<ProjectReference Include="..\..\src\BenchmarkDotNet.Diagnostics.Windows\BenchmarkDotNet.Diagnostics.Windows.csproj" />
<Reference Include="System.Reflection" />
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 7, 2017

Have you tried removing these refs to System and Microsoft.CSharp? I believe you don't need them explicitly here. But if you do, move them to common.props, since all projects have them anyways.

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 8, 2017

I would like to wait for VS 2017 RC4, make sure everything works, update the docs and then release.

@adamsitnik, than I have the following suggestion: we release v0.10.3 from the master branch now (I have some changes that I want to have in the stable version), and we release v0.10.4 with your update as soon as it is ready. Is it ok for you?

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 8, 2017

@CesarBS, you did a nice review, thanks for the help!

if (!gcMode.HasChanges)
return;

var runtimeConfigContent = new StringBuilder(80)

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 14, 2017

You don't have to generate this file anymore. See here: aspnet/KestrelHttpServer#1362 (review)

cc @natemcmaster

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 17, 2017

@CesarBS @natemcmaster thanks for the review! I adopted the code to your comments.

@AndreyAkinshin I am very close to getting it done, you can start downloading VS 2017 RC4 and give it a try ;)

I have one dilemma:

  • given that we have project called BenchmarkDotNet.Toolchains.Roslyn
  • and it contains Toolchain for Roslyn. Should it remain called RoslynToolchain or should it be just a Toolchain from BenchmarkDotNet.Toolchains.Roslyn namespace? the usage would be RoslynToolchain vs Roslyn.Toolchain
@adamsitnik adamsitnik force-pushed the farewellProjectJson branch from 4c23167 to 1a7bd42 Feb 22, 2017
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

This comment has been minimized.

Copy link
@cesarblum

cesarblum Feb 23, 2017

Any reason why this is not using the new csproj format?

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Feb 24, 2017

Author Member

@CesarBS I could not find any tool that would migrate the old csprojs for me (dotnet migrate supports only project.json => csproj) and I was too lazy to do it manually for these few testing projects

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

@adamsitnik, I installed the latest VS2017 RC4, opened the farewellProjectJson branch, and tried to run our samples. Here is the result:

// *** Generate ***
// Result = Success
// BinariesDirectoryPath = W:\Work\BenchmarkDotNet\IntroJobsFull_Sleep100_ShortRun\bin\Release\netcoreapp1.1

// *** Build ***
Errors in W:\Work\BenchmarkDotNet\IntroJobsFull_Sleep100_ShortRun\project.json
    Unable to resolve 'BenchmarkDotNet.Samples (>= 0.10.2)' for '.NETCoreApp,Version=v1.1'.
// Result = Failure
// Exception: dotnet restore has failed

Why BDN generates a project.json from our new cool csproj project? How we decide which toolchain should be chosen? Can we controll it with some job attributes?

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 24, 2017

@AndreyAkinshin we run dotnet --version and check if it contains "preview" keyword. If so it means we should use project.json files. What is the output of dotnet --version for you? Could you also run git clean -xfd and try again?

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

Yeah, I have the old version of dotnet in the path:

> dotnet --version
1.0.0-preview2-1-003177

Could we detect such situations? I think, it doesn't make any sense to run benchmarks on preview2-1-003177 from VS2017 projects.

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 24, 2017

Could we detect such situations? I think, it doesn't make any sense to run benchmarks on preview2-1-003177 from VS2017 projects.

sure, the thing is how to check it right? Maybe I could search for project.json files in solution directory? I could also read the .sln file and search for .xproj vs .csproj. @AndreyAkinshin how do you think? Which approach could be better?

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

Maybe I could search for project.json files in solution directory?

LGTM

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

Ok, I updated my PATH:

λ dotnet --version
1.0.0-rc4-004771

New result:

// Benchmark: IntroJobsFull.Sleep100: ShortRun(LaunchCount=1, TargetCount=3, WarmupCount=3)
// *** Generate ***
// Result = Success
// BinariesDirectoryPath = W:\Work\BenchmarkDotNet\IntroJobsFull_Sleep100_ShortRun\bin\Release\netcoreapp1.1

// *** Build ***
// Result = Failure

Could we print some additional information about the failed build?

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

Another concern. Here is my env info:

BenchmarkDotNet=v0.10.2.0, OS=Windows
Processor=?, ProcessorCount=8
Frequency=2531255 Hz, Resolution=395.0609 ns, Timer=TSC
dotnet cli version=1.0.0-rc4-004771
  [Host] : .NET Core 4.6.24628.01, 64bit RyuJIT

Could we print v0.10.2.0-develop instead of v0.10.2.0 when we use BDN from source?

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 24, 2017

Could we print some additional information about the failed build?

Sure

Could we print v0.10.2.0-develop instead of v0.10.2.0 when we use BDN from source?

Yes, I will restore the old behaviour (+the flag)

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

Also I spent a lot of time in the build stage (probably because of restore).
I think, it make sense to write a log line about restoring (with a hint that it could take some time) + print log info about how much time restore and build stages took.

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

I noticed, that we still generate benchmark folders (like IntroJobsFull_Sleep100_ShortRun) in the root solution folder. Is it possible to implement a workaround now (with the help of new csproj magic) and move it to the binaries folder (like bin/Release/netcoreapp1.1)?
Also, after running runCoreTests.cmd, I have C_B_Job-JZZTOX and C_B_Job-UVRVCR in the root folder which is not cool.

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 24, 2017

I noticed, that we still generate benchmark folders (like IntroJobsFull_Sleep100_ShortRun) in the root solution folder. Is it possible to implement a workaround now (with the help of new csproj magic) and move it to the binaries folder (like bin/Release/netcoreapp1.1)?

We could, but there is a chance that we would get hit by too long path exception again.
(..)solution\BDN.Generated\bin\Release\netcoreapp1.1\
vs
(..)solution\project\bin\Release\netcoreapp1.1\BDN.Generated\bin\Release\netcoreapp1.1\

Also, after running runCoreTests.cmd, I have C_B_Job-JZZTOX and C_B_Job-UVRVCR in the root folder which is not cool.

that is minor bug, I will fix it

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

We could, but there is a chance that we would get hit by too long path exception again.

Yeah, it can be a problem. But current solution (which we need only in specific situation) are too ugly. Also, we still could have some problems even with the root folder.
Maybe we can provide an API which allows to specify output directory. So, people with long paths will be easily fix it.

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

By the way, I think we are ready to merge it into master (v0.10.3 will support new csproj). I also want do to some improvements here and there. What do you think?

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 24, 2017

By the way, I think we are ready to merge it into master (v0.10.3 will support new csproj). I also want do to some improvements here and there. What do you think?

I agree. Right now I am working on getting the tests passed + adapting to your comments. Should finish today

@AndreyAkinshin AndreyAkinshin merged commit 7886ada into master Feb 24, 2017
@AndreyAkinshin AndreyAkinshin added this to the v0.10.3 milestone Feb 24, 2017
@AndreyAkinshin AndreyAkinshin deleted the farewellProjectJson branch Feb 24, 2017
@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 24, 2017

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 24, 2017

@CesarBS I removed the --output binaries option + now we are using --no-dependencies which has speeded up the build.

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Feb 26, 2017

@adamsitnik, could you move the generated folders to the binary folder before we release 0.10.3? There are still in the root solution directory.

@adamsitnik
Copy link
Member Author

adamsitnik commented Feb 26, 2017

@AndreyAkinshin Ok, I have just moved it. Personally, I am afraid of TooLongPathExceptions, but let's wait and see where it can take us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.