Should the DotNet.ReproducibleBuilds package be an SDK instead? #45
Comments
MattKotsenas
changed the title
|
I think if we did fix this we'd need to do a staged migration - keeping the Sdk.props and Sdk.targets but delegating those to import props and targets from the 'normal' Then we could break after enough time for folks to migrate. |
MattKotsenas
added a commit
to rjmurillo/moq.analyzers
that referenced
this issue
Jul 29, 2024
#91 intended to enable warnings as errors in CI. However, `$(ContinuousIntegrationBuild)` is set by `DotNet.ReproducibleBuilds`, and that package's .props file is imported _after_ our own Directory.Build.props. As a result, the properties were evaluated before they were set. This change moves them into the .targets file as "computed properties" in order for them to pick up the configuration from .props files. This aligns with the [documented guidance](https://learn.microsoft.com/en-us/visualstudio/msbuild/customize-your-build?view=vs-2022#choose-between-adding-properties-to-a-props-or-targets-file) "Set dependent properties in .targets files, because they pick up customizations from individual projects.". I've also filed dotnet/reproducible-builds#45 to see if we can make the reproducible-builds package more intuitive. Lastly, this cleans up existing warnings: - Fixes whitespace issues - Suppresses StyleCop warnings about documenting public members in tests, which already had the built-in rules suppressed - Reorders static members to be above instance members - Adds a single suppression for a long method to keep the change small
|
Yes, I think SDK instead is better. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
DotNet.ReproducibleBuilds.Isolatedpackage is an SDK,DotNet.ReproducibleBuildsis not. Should they both be?The specific issue that prompts this issue is that I see devs using properties set by RB like
$(ContinuousIntegrationBuild)inDirectory.Build.props, but the D.B.props file is imported first per dotnet/msbuild#994 (comment).Since RB is essentially an extension to the SDK, this feels like a defensible change to me. Open question about how much of a breaking change this would be. We likely could leave a stub .props file behind that emits a warning to explain the upgrade path.
The text was updated successfully, but these errors were encountered: