Skip to content

Conversation

@eerhardt
Copy link
Member

@eerhardt eerhardt commented Jan 5, 2022

When Microsoft.Extensions.Configuration.UserSecrets is referenced transitively, for example through Extensions.Hosting, the UserSecretsIdAttribute isn't getting added to the built assembly. This is because the logic is contained in a build folder which is excluded by the dependency from Extensions.Hosting.

The fix is to move the logic to buildTransitive, so it gets picked up by NuGet. One wrinkle is that the logic now conflicts with the netstandard compatibility logic added by the library infrastructure. In order to make progress, disable that logic for this package and rely on its dependencies to raise the compat error.

Contributes to #63246

When Microsoft.Extensions.Configuration.UserSecrets is referenced transitively, for example through Extensions.Hosting, the UserSecretsIdAttribute isn't getting added to the built assembly. This is because the logic is contained in a `build` folder which is excluded by the dependency from Extensions.Hosting.

The fix is to move the logic to `buildTransitive`, so it gets picked up by NuGet. One wrinkle is that the logic now conflicts with the netstandard compatibility logic added by the library infrastructure. In order to make progress, disable that logic for this package and rely on its dependencies to raise the compat error.

Contributes to dotnet#63246
@ghost
Copy link

ghost commented Jan 5, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

When Microsoft.Extensions.Configuration.UserSecrets is referenced transitively, for example through Extensions.Hosting, the UserSecretsIdAttribute isn't getting added to the built assembly. This is because the logic is contained in a build folder which is excluded by the dependency from Extensions.Hosting.

The fix is to move the logic to buildTransitive, so it gets picked up by NuGet. One wrinkle is that the logic now conflicts with the netstandard compatibility logic added by the library infrastructure. In order to make progress, disable that logic for this package and rely on its dependencies to raise the compat error.

Contributes to #63246

Author: eerhardt
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@ghost ghost assigned eerhardt Jan 5, 2022
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM. The only thing I hope @ericstj may confirm, disabling the NETStandard Compat target wouldn't have any side effect we didn't think about. The mentioned mitigation makes sense to me though.

@danmoseley
Copy link
Member

Is it feasible to have a test? (given this was significant enough to service for) But perhaps that would have to be in the SDK repo since it involves restoring packages?

@eerhardt
Copy link
Member Author

eerhardt commented Jan 6, 2022

Is it feasible to have a test? (given this was significant enough to service for) But perhaps that would have to be in the SDK repo since it involves restoring packages?

I can add a test, but as you said, it will have to be in a different repo since this is a packaging issue. We don't currently have packaging tests in this repo.

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Jan 6, 2022
@danmoseley
Copy link
Member

Your call, just curious. LMK if you would like me to merge.

@eerhardt eerhardt merged commit 392b35c into dotnet:main Jan 6, 2022
@eerhardt eerhardt deleted the Fix63246 branch January 6, 2022 20:54
@eerhardt
Copy link
Member Author

eerhardt commented Jan 6, 2022

LMK if you would like me to merge.

This one is main. I have permission to merge. #63423 is the 6.0 backport.

@ericstj
Copy link
Member

ericstj commented Jan 6, 2022

disabling the NETStandard Compat target wouldn't have any side effect we didn't think about.

I talked with @eerhardt and am OK with this.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants