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

Add support for user-supplied project file detection #2684

Merged
merged 9 commits into from
Jan 7, 2025

Conversation

Genbox
Copy link
Contributor

@Genbox Genbox commented Jan 1, 2025

This update adds support for "Locators" in the IConfig interface. A Locator is an extensibility point where users can define path-handling logic that differs from the built-in logic.

For example, if the user sets AssemblyName in their csproj, they are forced to use the InProcess job, or else BenchmarkDotnet gives an error with:

Generate Exception: System.NotSupportedException: Unable to find in and its subfolders. Most probably the name of output exe is different than the name of the .(c/f)sproj

The error is caused by the built-in logic trying to find a project file relative to the project root, which has the same name as the DLL. Unfortunately, lots of projects use AssemblyName to generate a short name, different from the project name (Like GNU utilities) or prepend the owners name (like GitHub/Nuget prefixes) to avoid assembly name collisions.

The issue has been repeatedly raised on the BenchmarkDotnet issue tracker and StackOverlow. For issues, see #1019, #498, #1178, #1785 and #1927.

The code

ILocator is a new public interface where users can define their logic. The interface is implemented like this:

private class CustomLocator : ILocator
{
    public LocatorType LocatorType => LocatorType.ProjectFile;

    public FileInfo Locate(DirectoryInfo rootDirectory, Type type)
    {
        return new FileInfo(Path.Combine(rootDirectory.FullName, @"Src\Benchmarks\MyBenchmarks.csproj"));
    }
}

The LocatorType is not strictly necessary, as the locator is only used in a single place right now. However, if there are other places where the user could benefit from the extensibility, it is necessary to have to distinguish between locators.

If that is not wanted, let me know, and I'll remove it and rename ILocator to IProjectLocator to specialize the code for its intended purpose.

Users can add their locator to a ManualConfig like so:

// To use both ProjectLocator.Default and custom locator:
IConfig config = ManualConfig.CreateMinimumViable()
                           .AddLocator(new CustomLocator());

// To use only the custom locator:
IConfig config = ManualConfig.CreateEmpty()
                           .AddLocator(new CustomLocator());

Notes

  • ProjectLocator is a new class that contains the old logic to find the project file.
  • DefaultConfig uses ProjectLocator.Default
  • DebugConfig also uses ProjectLocator.Default
  • ManualConfig defaults to no ILocator, but CreateMinimumViable includes ProjectLocator.Default
  • I've added tests to ensure 1) duplicate locators are supported 2) that configs contains the correct locators

@@ -246,36 +247,32 @@ private static string GetIndentedXmlString(XmlDocument doc)
/// returns a path to the project file which defines the benchmarks
/// </summary>
[PublicAPI]
protected virtual FileInfo GetProjectFilePath(Type benchmarkTarget, ILogger logger)
protected virtual FileInfo GetProjectFilePath(BenchmarkCase benchmark, ILogger logger)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noting the breaking change to a public API.

@timcassell
Copy link
Collaborator

Also the DotNetCliGenerator.GetBuildArtifactsDirectoryPath may need to be updated to support the new locators.

@Genbox
Copy link
Contributor Author

Genbox commented Jan 2, 2025

Most suggestions are now implemented.

@Genbox Genbox requested a review from timcassell January 5, 2025 11:21
@Genbox
Copy link
Contributor Author

Genbox commented Jan 5, 2025

Update: ProjectLocator no longer exists. Instead, we simply give the user the ability to extend ManualConfig with a set of IFileLocators, which are executed before the default logic inside CsProjGenerator.GetProjectFilePath.

@Genbox
Copy link
Contributor Author

Genbox commented Jan 5, 2025

For future reference and to show that it works.

I have a project that sets AssemblyName like this: <AssemblyName>Genbox.$(MSBuildProjectName)</AssemblyName>

With this code:
image

I get this error (as expected):
image

After changing it to use the new Locator API:
image

It works as expected:
image

@timcassell
Copy link
Collaborator

Can you please add an integration test?

@Genbox
Copy link
Contributor Author

Genbox commented Jan 6, 2025

Can you please add an integration test?

The only solution I can think of is a separate project. Ideally, it would be a test inBenchmarkDotNet.IntegrationTests, but I cannot think of a way to do that. Do you have any ideas?

and rename LocatorArgs to FileLocatorArgs
@timcassell
Copy link
Collaborator

Why can't you do it in BenchmarkDotNet.IntegrationTests? Just do like your example above:

fileInfo = new FileInfo(Path.Combine(dir, "../../../BenchmarkDotNet.IntegrationTests.csproj"));

@Genbox
Copy link
Contributor Author

Genbox commented Jan 6, 2025

Why can't you do it in BenchmarkDotNet.IntegrationTests? Just do like your example above:

fileInfo = new FileInfo(Path.Combine(dir, "../../../BenchmarkDotNet.IntegrationTests.csproj"));

I would change the assembly name of BenchmarkDotNet.IntegrationTests.csproj, which will affect all the other tests. Adding a FileLocator that points to IntegrationTests.csproj could still work, even if FileLocators don't. The fallback will find the project instead, thereby hiding a bug.

So the alternative is to create a new integration test project like BenchmarkDotNet.IntegrationTests.ConfigPerAssembly and the others.

@timcassell
Copy link
Collaborator

Ah, that's a good point. Yes, you can create a new project and set it up like BenchmarkDotNet.IntegrationTests.ConfigPerAssembly.

@Genbox
Copy link
Contributor Author

Genbox commented Jan 6, 2025

An integration test has been implemented.

@Genbox Genbox requested a review from timcassell January 6, 2025 21:37
@Genbox
Copy link
Contributor Author

Genbox commented Jan 6, 2025

Please check that the csproj has the correct properties set. There were package ids and other stuff in the other projects that seemed irrelevant.

The tests also fails when running as debug. Not sure if this is intended or not. It is not something I've caused :)

@Genbox Genbox requested a review from timcassell January 6, 2025 22:04
Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

LGTM. We can update the GetBuildArtifactsDirectoryPath to support the new file locators separately.

Thanks @Genbox!

@timcassell timcassell merged commit 1aab1c0 into dotnet:master Jan 7, 2025
8 checks passed
@timcassell timcassell added this to the v0.14.1 milestone Jan 7, 2025
@Genbox
Copy link
Contributor Author

Genbox commented Jan 7, 2025

LGTM. We can update the GetBuildArtifactsDirectoryPath to support the new file locators separately.

Thanks @Genbox!

Likewise! Thanks for your help and contributions. Very much appreciated.

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

Successfully merging this pull request may close these issues.

2 participants