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

Forward msbuild properties to project reference #1377

Closed
dotMorten opened this issue Feb 25, 2020 · 8 comments · Fixed by #2393
Closed

Forward msbuild properties to project reference #1377

dotMorten opened this issue Feb 25, 2020 · 8 comments · Fixed by #2393
Assignees
Milestone

Comments

@dotMorten
Copy link

Currently when you set MSBuild properties, they don't get forwarded to the project reference from the autogenerated projects.

In https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/Templates/CsProj.txt#L26 it would be nice if we could provide addition properties on the reference. For example if this was added to that template::

 <ItemGroup>
    <ProjectReference Include="$CSPROJPATH$" AdditionalProperties="$ADDITIONAL_PROPERTIES$"/>
  </ItemGroup>

And then a way to set the $ADDITIONAL_PROPERTIES$ (or extract the /p properties from the msbuild properties.
Otherwise the msbuild properties are rather useless if the referenced project isn't rebuilt with the correct properties (it appears it just pulls in the project reference and not building it if it was already built, so multiple jobs with different settings won't get applies).

@adamsitnik
Copy link
Member

Hello @dotMorten

I've run into similar issues with MsBuild properties. I was wondering what would be the best way of passing all kinds of custom MsBuild properties to the auto-generated project. Perhaps we should add a possibility to make the auto-generated project include provided .props file?

Could you try using the MsBuildArgument type as a workaround for now?

job.With(new Argument[] { new MsBuildArgument("/p:SomeProperty=Value")})

@dotMorten
Copy link
Author

I tried that. I even tried hacking the above into a custom version of BenchmarkDotNet. Still didn't work so this issue might have been a bit premature.

The main issue I have is that the benchmark assembly isn't recompiled for each job, so forwarding this parameter doesn't seem to do anything.
That creates a problem when using different versions as a dependency, but I can't add benchmarks that uses newer APIs or use #if-def to switch between old and new more efficient methods to do a compare across versions.

I basically would like to be able to create tests like this:

#if VERSION2_0
   myList.AddRange(items);
#else
  foreach(var item in items)
   myList.Add(item);
#endif

Another scenario is a case of a braking API change (in our case we had to change the color type when adding .NET Standard support). For example:
#if VERSION2_0
myInstance.Color = System.Drawing.Color.Red;
#else
myInstance.Color = System.Windows.Media.Colors.Red;
#endif

I got it so for each version I can create a set of defines (ie Version 3 will have Version2_0 and Version3_0 defined)

However it doesn't work, as the benchmark assembly is compiled with whatever version is defined in that project, and then the benchmark executable that's created for each job just copies the new DLL on top. That means you get into "MethodNotFound" exceptions when the test executes.

@adamsitnik
Copy link
Member

@dotMorten I have filled #1403 which should solve this problem and a few more similar issues ;)

@dotMorten
Copy link
Author

Thank you!

@timcassell

This comment was marked as outdated.

@timcassell

This comment was marked as outdated.

@timcassell
Copy link
Collaborator

More chatter about this issue in #1773.

@timcassell timcassell assigned timcassell and unassigned adamsitnik Aug 5, 2023
@timcassell
Copy link
Collaborator

I'm onto a fix for Core, I just need to make it also work for NativeAot, etc.

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

Successfully merging a pull request may close this issue.

4 participants