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

[nuget] add publish warning when RuntimeIdentifier is not specified #3601

Merged

Conversation

lachmatt
Copy link
Contributor

@lachmatt lachmatt commented Sep 4, 2024

Why

Warn when an app uses autoinstrumentation NuGet package, and is published without specifying runtime identifier.
In such scenario, native profiler libraries for all of the supported platforms are copied to the publish output directory, which results in increased disk usage.

Fixes #

What

  • Add a MSBuild target that runs before the publish, and issues a warning when runtime identifier is not specified
  • Allow the warning to be suppressed by setting DisableAutoInstrumentationCheckForRuntimeIdentifier property

I considered making target run before build instead of publish, and issue an Error instead of a Warning (would be consistent with other target), but decided it would be to noisy/restrictive. Let me know if you think I should change the approach.

Tests

Included in PR.

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests.

@lachmatt lachmatt marked this pull request as ready for review September 4, 2024 13:57
@lachmatt lachmatt requested a review from a team September 4, 2024 13:57
@Kielek Kielek enabled auto-merge (squash) September 5, 2024 05:10
@Kielek Kielek disabled auto-merge September 5, 2024 05:10
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

IMO worth to add changelog entry.

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

Successfully merging this pull request may close these issues.

4 participants