Fix MsBuildArguments

[timcassell]

Fixes #1377

I removed build with --no-dependencies because it apparently makes the resulting build not work properly when paired with --output. Looking at the blame history, I couldn't find a PR or issue for why it was added in the first place, only a commit (6ee21bb). Is --no-dependencies needed for a particular reason @adamsitnik?

[adamsitnik]

I removed build with --no-dependencies because it apparently makes the resulting build not work properly when paired with --output.

It was added as a fallback, for scenarios where dotnet build has failed (for example, due to the .dll/.exe being in use). By building without dependencies, we were able to succeed. It should not be executed by default, only as a fallback when full build fails.

[timcassell]

It was added as a fallback, for scenarios where dotnet build has failed (for example, due to the .dll/.exe being in use).

Okay, that's what I thought. If that's the only case for it, then we don't need it anymore, as specifying a new output path avoids the issue.

[adamsitnik]

as specifying a new output path avoids the issue.

I've tried that in the past (years ago, it could be outdated), one of the issues I've hit was the path being overwritten by some .props files.

https://github.com/dotnet/performance is a great example of repo that run into the issue. You can use this project file to tell it to use your local copy of BDN rather than a nuget package for testing and then try run the benchmarks from here

[timcassell]

https://github.com/dotnet/performance is a great example of repo that run into the issue. You can use this project file to tell it to use your local copy of BDN rather than a nuget package for testing and then try run the benchmarks from here

Cool. I tried that and it worked no problem.

I've tried that in the past (years ago, it could be outdated), one of the issues I've hit was the path being overwritten by some .props files.

I don't know what you tried, but I tried placing it in the csproj and it didn't work, it only works if I pass it to the cli. And if I only use --output without /p:(Intermediate)OutputPath, it doesn't always work. I'm pretty sure properties provided to the cli always take precedence over properties in projs.

[timcassell]

This PR fixes several issues. Could you PTAL @adamsitnik?

[nojaf]

It seems like @adamsitnik is unavailable at the moment, @timcassell would there be anyone else that can review this?

[timcassell]

@AndreyAkinshin?

[AndreyAkinshin]

@timcassell no concerns from my side, but for this issue, we really need a review from @adamsitnik who is the author of our disassembler. I don't know much about this subsystem, and I could miss some potential problems.

If you feel like this change is safe and you properly tested it so there is no actual need for a thorough review, let me know, and I will merge it as is.

[timcassell]

@AndreyAkinshin Thanks. The changes I made to the disassembler were simply to release the pdb file when it's no longer needed. Without doing that, tests were failing when it was trying to overwrite the file. There was a todo there to clean up that caching anyway. I think it's safe.

I did put this on the v0.14.0 milestone because it is changing how builds are done and breaking some public APIs, but if you want to pull it into v0.13.13, I'm okay with that.

[AndreyAkinshin]

I did put this on the v0.14.0 milestone because it is changing how builds are done and breaking some public APIs, but if you want to pull it into v0.13.13, I'm okay with that.

I don't mind releasing v0.14.0 as the next version. Could you please add some release notes to this PR then? Just create a file v0.14.0.md in BenchmarkDotNet\docs\_changelog\header\ and provide a brief status update for our users (the content of this file will be automatically included in the website changelog, the release description, and the release discussion).

If you have any other breaking changes you would like to include in v0.14.0, don't hesitate to ping me in the corresponding PRs.

[AndreyAkinshin]

@timcassell could you please provide some additional context in the changelog? There is no need for an extensive description, but it would be awesome to have several sentences describing the actual change so that it's easily understandable for our users who don't have the internal development context. The current description feels too brief and doesn't provide a lot of useful information for the users.

[AndreyAkinshin]

@timcassell now the release notes look great, thanks!
Could you please rebase the PR branch onto the master? I can't merge it automatically due to the conflicts.

[timcassell]

@AndreyAkinshin I fixed it up. For future reference, "squash and merge" should've worked, rather than "rebase and merge".