search

dotnet / BenchmarkDotNet

Powerful .NET library for benchmarking
https://benchmarkdotnet.org
MIT License
10.61k stars 972 forks source link

Change the way of generating new project file #1403

Open adamsitnik opened 4 years ago

adamsitnik commented 4 years ago

As of today, we generate the .csproj file by filling this text template.

The main disadvantage of this approach is that if there are any custom MSBuild settings in the project with a benchmark that we are not aware of, we sometimes fail to build the project and hence fail to run the benchmarks.

We used to follow a similar approach for generating the app.config files, but at some point in time we changed the approach to "rewrite everything from the source file unless it's on our list of settings". The code can be found here

I propose that we switch to a similar approach with the .csproj file:

This should fix:

and a few more issues

adamsitnik commented 4 years ago

@WojciechNagorski if you are still looking for some task then most probably this one would have the biggest impact (it's going to solve at least 10+ bugs)

WojciechNagorski commented 4 years ago

@adamsitnik Yes, I would like to solve this task. I will find time next week.

adamsitnik commented 4 years ago

Yes, I would like to solve this task. I will find time next week.

Awesome! I've assigned you to the task.

WojciechNagorski commented 4 years ago

Update: I've thought for 2 weeks and I think that I invented the final solution for all cases.

adamsitnik commented 4 years ago

@WojciechNagorski awesome :D

ilohmar commented 3 years ago

Hi, is there any progress on the implementation? I am only asking because it sounded like the problem was basically solved. :)

We also have a custom build configuration to do out-of-tree builds, and currently use the workaround of putting a stripped-down Directory.Build.props (which does not change any output paths) into the build output dir.

WojciechNagorski commented 3 years ago

@ilohmar @adamsitnik After I wanted to fix this problem I changed the project at work, then I changed work at all. So I didn't have time for this task. Currently, I have only an idea but I don't have time for it.

timcassell commented 1 year ago

@WojciechNagorski If you don't have time to work on this, could you leave your thoughts on what the solution should be so someone can work on it?

WojciechNagorski commented 1 year ago

@timcassell I had some ideas and wanted to test them one by one. Two ideas are particularly promising. But I don't want to describe them because I'd have to bring back the details and that's also work and time. I would love to do this and more, even full-time, but this was only a free contribution and fun.

timcassell commented 1 year ago

update all relative paths

I'm unsure of how to reliably do this. <Reference Include=""> could be a path, or an assembly name, and it's undocumented how to determine what it is. At least I couldn't find the documentation. https://learn.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-items?view=vs-2022 doesn't even mention the Include attribute.

[Edit] Also if the paths/assemblies include variables in them, it would be next to impossible for us to determine that without having our own mock-msbuild to resolve them.

The easiest thing to do would be to produce the generated csproj in the same directory as the original xproj. It's not ideal since it would clutter the original project directory and is separated from the generated code, but it would be able resolve all paths correctly without us having to try to guess.

Any thoughts on that @AndreyAkinshin @adamsitnik?

ketanpkolte commented 1 month ago

Hi @timcassell, @adamsitnik

This issue/change involves references to a range of both old and new issues/discussions. To clarify the scope of work, I propose creating a separate, dedicated discussion thread.

In that discussion, we should clearly define the bug/issue description and add relevant details, such as:

This will help ensure everyone has a clear understanding of the task at hand.

timcassell commented 1 month ago

Thanks for the input @ketanpkolte. I think this issue is outdated. I have already fixed most of the linked issues, and I have another PR to fix some more (#2508). Only one of the issues we haven't been able to repro so far (#1358). Also, the suggested changes introduce their own risks, and builds are already quite stable now. I think we should rather tackle the issues on a case-by-case basis. I was thinking of closing this issue after my PR is merged, or we could close it sooner if @adamsitnik is in agreement that this is no longer needed.